git-cherry-pick and git-commit --amend in version 1.7.6.4

Hi,

still with version 1.7.6.4, when I do a cherry-pick, that succeeded, I cannot do a commit --amend after:

# git cherry-pick 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
[dev 1a04a23] drivers/net/usb/asix.c: Fix unaligned accesses
 1 files changed, 33 insertions(+), 1 deletions(-)
# echo $?
0
# git commit --amend
fatal: You are in the middle of a cherry-pick -- cannot amend.
#

The same operations (with the same patch), with version 1.7.3.4 is ok.

Regards,
Nicolas

Pinging Jay who may have know a thing or two from the history leading to 37f7a85 (Teach commit about CHERRY_PICK_HEAD, 2011-02-19).

Yep, I replied to Nicolas' other message.

j.

This is probably different fro Nicolas's use case, but you can easily trigger:

 $ edit foo.c
 $ EDITOR=: git commit --amend ;# forget to add foo.c
 $ git cherry-pick other
 error: Your local changes to the following files would be overwritten by
 merge:
        foo.c
 Please, commit your changes or stash them before you can merge.
 Aborting
 $ EDITOR=: git commit --amend foo.c
 fatal: You are in the middle of a cherry-pick -- cannot amend.

I think the sequencer state needs to be removed when the command aborts.

This needs to be fixed before 1.7.7.1.

Or written later in do_pick_commit().

Something like this?

index 3117776c2c..f7fcc88871 100644
@@ -384,6 +384,7 @@ static int do_pick_commit(void)
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
+	int record_cherry_pick_head = 0;
 
 	if (no_commit) {
 		/*

@@ -477,7 +478,7 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			record_cherry_pick_head = 1;
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {

@@ -514,6 +515,9 @@ static int do_pick_commit(void)
 	free_message(&msg);
 	free(defmsg);
 
+	if (record_cherry_pick_head)
+		write_cherry_pick_head();
+
 	return res;
 }

As a general direction, I think it makes tons of sense ot delay writing out these state files before you really commit that the user will be in the cherry-pick (or revert) sequence.

I am not sure if do_pick_commit() is the best place to do so. Wouldn't it be necessary to special case the first round at least? The pick can fail in one of two ways:

- It does not even start. This is the case I illustrated in the earlier message, and we do not want to leave sequencer state.

- It stops with conflict. At this point, it probably is OK to say that the user is committed to go with the sequencer flow and the next step would be to help Git resolve conflicts and proceed, and in this case we do want the sequencer state.

And once we picked/reverted at least one commit, if there are more, the user knows the sequencer flow is in progress, and it is perfectly fine to see the error message from "commit --amend".

It's just the "commit --amend" message that says I cannot amend felt utterly out of place, immediately after seeing "cherry-pick" that tried to pick only one commit did _not_ even start.

After thinking about it a bit more, I am starting to think that it may just be the error message given by "commit --amend".

If the sequence were like this:

    $ edit foo.c ;# I want to fix foo.c in the current branch "master"
    $ EDITOR=: git commit --amend ;# forgot to say "foo.c"
    $ git cherry-pick other~2 other
    [master 48882c9] frotz: update xyzzy
     Author: Jay Soffian <[email protected]>
     1 files changed, 2 insertions(+), 2 deletions(-)
    error: Your local changes to the following files would be overwritten by merge:
            foo.c
    Please, commit your changes or stash them before you can merge.
    Aborting

Then at this point, amending the commit at HEAD^ is not possible anyway, as it is not at the tip anymore. It is perfectly fine that

    $ git commit --amend foo.c

fails at this point.

It is just that it initially felt irritatingly wrong if I was picking only a single commit "other" that wanted to touch foo.c, like this:

    $ edit foo.c ;# I want to fix foo.c in the current branch "master"
    $ EDITOR=: git commit --amend ;# forgot to say "foo.c"
    $ git cherry-pick other
    error: Your local changes to the following files would be overwritten by merge:
            foo.c
    Please, commit your changes or stash them before you can merge.
    Aborting

At this point, as it says "Please commit your changes", and it is very clear that cherry-pick _correctly_ errored out without touching any of my work, it is natural for me to expect that I can "commit --amend" to fix my eariler mistake.

    $ EDITOR=: git commit --amend foo.c
    fatal: You are in the middle of a cherry-pick -- cannot amend.

This can only worked around halfway:

    $ rm .git/CHERRY_PICK_HEAD
    $ EDITOR=: git commit --amend foo.c

Things look OK so far, but then restarting the cherry-pick I wanted to do after I fixed foo.c would fail like this:

    $ git cherry-pick other
    error: .git/sequencer already exists.
    error: A cherry-pick or revert is in progress.
    hint: Use --continue to continue the operation
    hint: or --reset to forget about it
    fatal: cherry-pick failed

Perhaps it would be a possible solution to teach "cherry-pick --reset" to remove CHERRY_PICK_HEAD and the sequencer state, so that the above transcript would become:

    $ edit foo.c ;# I want to fix foo.c in the current branch "master"
    $ EDITOR=: git commit --amend ;# forgot to say "foo.c"
    $ git cherry-pick other
    error: Your local changes to the following files would be overwritten by merge:
            foo.c
    Please, commit your changes or stash them before you can merge.
    Aborting
    $ EDITOR=: git commit --amend foo.c
    fatal: You are in the middle of a cherry-pick -- cannot amend.
    hint: use "git cherry-pick --reset" to discard the previous cherry-pick.
    $ git cherry-pick --reset
    $ EDITOR=: git commit --amend foo.c
    $ git cherry-pick other
    [master 48882c9] frotz: update nitfol
     Author: Jay Soffian <[email protected]>
     1 files changed, 2 insertions(+), 2 deletions(-)

At least, that looks like something we _could_ explain to the end users.

Does it also refrain from creating sequencer state directory?

I'm not familiar with the sequencer code. It's not in master is it?

What's happening here is that do_pick_commit() was creating CHERRY_PICK_HEAD, but then git aborts several call sites away (do_recursive_merge -> merge_trees -> git_merge_trees -> unpack_trees -> display_error_msgs).

So I think do_pick_commit() needs to defer creating CHERRY_PICK_HEAD till after the possible abort.

I don't know if that's the right fix for next or not, but it seems correct for master.

j.

Please do the following with 1.7.6.4:

# ls .git
# git cherry-pick 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
# ls .git
# git cat-file -p 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
# git cat-file -p HEAD

And send the transcript.

Thanks.

j.

Here is:

# ls .git
branches  COMMIT_EDITMSG  config  description  FETCH_HEAD  HEAD  hooks  index info  logs  objects  ORIG_HEAD  packed-refs  refs
# git cherry-pick 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
[dev 4cca2c2] drivers/net/usb/asix.c: Fix unaligned accesses
 1 files changed, 33 insertions(+), 1 deletions(-)
# ls .git
branches  CHERRY_PICK_HEAD  COMMIT_EDITMSG  config  description  FETCH_HEAD HEAD  hooks  index  info  logs  objects  ORIG_HEAD  packed-refs  refs
# git cat-file -p 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
tree f29742a1a73c27a88c7ac701a7a06ac1c2f7973a
parent e7a3af5d8cd782b84e6ca4e4dcc8613be1a809f0
author Neil Jones <[email protected]> 1274141908 -0700
committer David S. Miller <[email protected]> 1274141908 -0700

drivers/net/usb/asix.c: Fix unaligned accesses

Using this driver can cause unaligned accesses in the IP layer This has been fixed by aligning the skb data correctly using the spare room left over by the 4 byte header inserted between packets by the device.

# git cat-file -p HEAD
tree 282b6492d9d5bcf1c3718420c6f31ca2033ca5cb
parent c8054f854773e65d8592f2ef35939ec2ae8b01df
author Nicolas Dichtel <[email protected]> 1317886553 +0200
committer Nicolas Dichtel <[email protected]> 1317886553 +0200

drivers/net/usb/asix.c: Fix unaligned accesses

Using this driver can cause unaligned accesses in the IP layer This has been fixed by aligning the skb data correctly using the spare room left over by the 4 byte header inserted between packets by the device.

#

Regards,
Nicolas

There is another symptom, describe in this thread:
http://comments.gmane.org/gmane.comp.version-control.git/182852

Maybe the two problems are related.

Regards,
Nicolas

No CHERRY_PICK_HEAD, so far so good.

cherry-pick completes successfully.

This is bad. CHERRY_PICK_HEAD should only exist if the cherry-pick failed.

I really don't know what could cause this. Possibly a hook in your repo?

Using "GIT_TRACE=1 git cherry-pick 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9" will tell you whether git is running any hooks.

I can't think of anything config-wise that would cause this behavior.

I'll peer at the code some more...

j.

No hooks:

# ls .git/hooks/
applypatch-msg.sample  post-commit.sample   post-update.sample pre-commit.sample          pre-rebase.sample
commit-msg.sample      post-receive.sample  pre-applypatch.sample prepare-commit-msg.sample  update.sample

Here is the output:
# GIT_TRACE=1 git cherry-pick 3f78d1f210ff89af77f042ab7f4a8fee39feb1c9
trace: built-in: git 'cherry-pick' '3f78d1f210ff89af77f042ab7f4a8fee39feb1c9'
trace: run_command: 'commit' '-n' '-F' '.git/MERGE_MSG'
trace: exec: 'git' 'commit' '-n' '-F' '.git/MERGE_MSG'
setup: git_dir: .git
setup: worktree: /home/dichtel/DEV/linux-2.6
setup: cwd: /home/dichtel/DEV/linux-2.6
setup: prefix: (null)
trace: built-in: git 'commit' '-n' '-F' '.git/MERGE_MSG'
[master 8372873] drivers/net/usb/asix.c: Fix unaligned accesses

 1 files changed, 33 insertions(+), 1 deletions(-)

#

With a very basic config, the pb he still here:
cat ~/.gitconfig
[user]
name = Nicolas Dichtel
email = [email protected]

I will try to do some other tests.

Regards,
Nicolas

I have a theory that determine_whence() inside commit.c isn't finding .git/CHERRY_PICK_HEAD:

	else if (file_exists(git_path("CHERRY_PICK_HEAD")))
		whence = FROM_CHERRY_PICK;

That would cause the mis-attributed cherry-picked commit. commit.c is also responsible for removing CHERRY_PICK_HEAD, which is not happening correctly:

	unlink(git_path("CHERRY_PICK_HEAD"));

Maybe git_path("CHERRY_PICK_HEAD") is returning something unexpected. But the trace output looks fine.

Aside, I'm a little confused by the "setup:" output appearing above. In 1.7.5 and later, it requires setting GIT_TRACE_SETUP=1 to appear, but you reported you're having this problem with 1.7.6.4.

j.