Splitting a patch

To make things easier for your code reviewer, each patch should be small, and hold one well defined change. I break this rule all the time, and it comes back to bite me. What happens is that I get heads down coding, and I have a solution that involves changes to wide number of files and subsystems, new abstractions, etc. Here is how I am currently dealing with breaking down a big patch.

I started with a branch named revocation-events. On it I had one big commit. To make sure I could always get back to this commit, I started by creating a new branch:

git checkout -b revocation-events-split

Note I want I want to be able to commit later with the commit message from my original comment.

git log --format=%B -n 1 HEAD > /tmp/commitmsg.txt

I can later use that commit message with

git commit -F /tmp/commitmsg.txt

Now, the scary part. Remove everything from the commit:

git reset –soft HEAD~1

Leaves all of your files staged for review. You can either leave some in and selectively remove others, or remove them all. For my case, I need to inspect each. Since all of my files are in either the etc or the keystone subdir:

git status
# On branch throwaway
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
#	modified:   etc/keystone-paste.ini
#	modified:   etc/keystone.conf.sample
#	modified:   keystone/assignment/core.py

and so on, I run

git reset HEAD keystone
git reset HEAD etc

Now, I want to selectively add back in lines from a single file. To see the changes in a single file:

 git diff HEAD keystone/tests/test_notifications.py

To start off the new review, I can select certain lines from this change using

git add -i --patch keystone/tests/test_notifications.py

(I’ve just been informed that the -i is redundant with –patch)

This puts git add in interactive mode it will query on each chunk of the patch. You are given a list of options:
Stage this hunk [y,n,q,a,d,/,e,?]? y = yes and n = no. ? Will show what the others mean.

What if a given chunk is mixed? Use the e for edit option. The below hunk has two tests. I want each in a separate commit.

diff --git a/keystone/tests/test_notifications.py b/keystone/tests/test_notifications.py
index 57741fe..d724dd4 100644
--- a/keystone/tests/test_notifications.py
+++ b/keystone/tests/test_notifications.py
@@ -228,6 +228,21 @@ class NotificationsForEntities(test_v3.RestfulTestCase):
         self.identity_api.delete_user(user_ref['id'])
         self._assertLastNotify(user_ref['id'], 'deleted', 'user')
 
+    def test_delete_domain(self):
+        domain_ref = self.new_domain_ref()
+        self.identity_api.create_domain(domain_ref['id'], domain_ref)
+        domain_ref['enabled'] = False
+        self.identity_api.update_domain(domain_ref['id'], domain_ref)
+        self.identity_api.delete_domain(domain_ref['id'])
+        self._assertLastNotify(domain_ref['id'], 'deleted', 'domain')
+
+    def test_disable_domain(self):
+        domain_ref = self.new_domain_ref()
+        self.identity_api.create_domain(domain_ref['id'], domain_ref)
+        domain_ref['enabled'] = False
+        self.identity_api.update_domain(domain_ref['id'], domain_ref)
+        self._assertLastNotify(domain_ref['id'], 'disabled', 'domain')
+
     def test_update_group(self):
         group_ref = self.new_group_ref(domain_id=self.domain_id)
         self.identity_api.create_group(group_ref['id'], group_ref)
Stage this hunk [y,n,q,a,d,/,e,?]? 

e gives me the message at the top of the screen:

 # Manual hunk edit mode -- see bottom for a quick guide
 @@ -228,6 +228,21 @@ class NotificationsForEntities(test_v3.RestfulTestCase):

And instructions at the bottom:

 # To remove '-' lines, make them ' ' lines (context).
 # To remove '+' lines, delete them.
 # Lines starting with # will be removed.

To remove the second test, I remove the lines with the + next to them:

 # Manual hunk edit mode -- see bottom for a quick guide
 @@ -228,6 +228,21 @@ class NotificationsForEntities(test_v3.RestfulTestCase):
          self.identity_api.delete_user(user_ref['id'])
          self._assertLastNotify(user_ref['id'], 'deleted', 'user')
 
 +    def test_delete_domain(self):
 +        domain_ref = self.new_domain_ref()
 +        self.identity_api.create_domain(domain_ref['id'], domain_ref)
 +        domain_ref['enabled'] = False
 +        self.identity_api.update_domain(domain_ref['id'], domain_ref)
 +        self.identity_api.delete_domain(domain_ref['id'])
 +        self._assertLastNotify(domain_ref['id'], 'deleted', 'domain')
 +
      def test_update_group(self):
          group_ref = self.new_group_ref(domain_id=self.domain_id)
          self.identity_api.create_group(group_ref['id'], group_ref)
 # ---
 # To remove '-' lines, make them ' ' lines (context).
 # To remove '+' lines, delete them.
 # Lines starting with # will be removed.
 #
 # If the patch applies cleanly, the edited hunk will immediately be
 # marked for staging. If it does not apply cleanly, you will be given
 # an opportunity to edit again. If all lines of the hunk are removed,
 # then the edit is aborted and the hunk is left unchanged.

This is in vi mode for me. To exit

:wq

Then , git commit to create a smaller patch.
(update: you can run git log -p to confirm only the changes you want made it in to the commit)

Once I have a patch basically ready to go, and I want to test it, I have a couple choices. The simplest is to do git stash and run the unit tests, but I always get distracted. Instead of forgetting my stash, I tend to create each of the commits in turn. Once I have all three commits created, I can do git checkout HEAD~3 To checkout the first commit, run the tests, and see that things are OK. To go back to the head of the branch

git checkout revocation-events-split

If all three patches are good, and ready for review, git review will submit all three of them.

git reflog is your friend. It shows you everything you have done to change the state of your repo. I refer to the reflog all the time to reorient myself.

4 thoughts on “Splitting a patch

  1. Matěj Yeah, I should have named this “Splitting a commit” as it really is only a patch during the edit stage. Still many people from older projects think of commits in terms of patches. Hadn’t even realized it until you responded.

  2. Thanks for sharing, but I would have thought that for people who haven’t learnt how to do this yet, the most straightforward recommendation would be to use “git gui” to amend the commit? It’s *much* simpler than the CLI-based approach, and can easily operate at file-, hunk- or even line-based granularity.

    The steps are as follows:

    1. Run git gui
    2. Click “Amend Last Commit”
    3. For each file in the “Staged Changes” pane from which you wish to remove items from the commit, select the file, and then unstage the whole file via the Commit menu, or individual hunks/lines by right-clicking
    4. Click “Commit”
    5. Stage and commit the remaining changes.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>