Rebase regression in v1.7.9?

Rebase regression in v1.7.9?

[Felipe Contreras]

Hi,

Try the following steps:

% git checkout -b tmp v1.7.9
% git cherry-pick 6e1c9bb^2^
% git rebase -i --onto 6e1c9bb HEAD^
% git rebase --continue

The rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.

This doesn't happen if you run rebase without -i, or do rebase --skip instead.

IIRC correctly rebase --continue used to work fine.

Cheers.

-- 
Felipe Contreras

Re: Rebase regression in v1.7.9?

[Andrew Wong]

On 01/31/2012 05:56 PM, Felipe Contreras wrote:
> The rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.
>   
Ah, good catch. I can reproduce the issue. This is only happening in
"rebase -i" because interactive rebase relies on cherry-pick, but not
regular rebase. And now cherry-pick creates a state when there's a
conflict (since 1.7.5?), which "rebase -i" didn't expect before. We
probably just need to do a manual clean up before "rebase -i" continues.

I'll try to come up with a patch for this. In the mean time, doing a
"git reset" will remove that dangling file. Of course, you could always
manually remove it. Does the dangling file cause a subsequent git
command to fail?

Re: Rebase regression in v1.7.9?

[Felipe Contreras]

On Wed, Feb 1, 2012 at 7:27 PM, Andrew Wong <andrew.w@sohovfx.com> wrote:
> On 01/31/2012 05:56 PM, Felipe Contreras wrote:
>> The rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.
>>
> Ah, good catch. I can reproduce the issue. This is only happening in
> "rebase -i" because interactive rebase relies on cherry-pick, but not
> regular rebase. And now cherry-pick creates a state when there's a
> conflict (since 1.7.5?), which "rebase -i" didn't expect before. We
> probably just need to do a manual clean up before "rebase -i" continues.
>
> I'll try to come up with a patch for this. In the mean time, doing a
> "git reset" will remove that dangling file. Of course, you could always
> manually remove it. Does the dangling file cause a subsequent git
> command to fail?

No, it's just annoying with the __git_ps1 prompt stuff. Yeah, 'git
reset' solves the problem, but it's much better to type 'git skip'
instead... for now.

-- 
Felipe Contreras

[PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD
for its caller's sake, let its caller do the work. This way, the
sequencer doesn't have to check all points of failures where its caller
doesn't want CHERRY_PICK_HEAD.

For example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if
'commit' failed due to an empty commit. Letting 'rebase -i' deal with
removing CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--interactive.sh |   10 +++++++++-
 sequencer.c                |    6 ------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5812222..061248c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -196,7 +196,12 @@ pick_one () {
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $ff "$@"
+	output git cherry-pick $ff "$@" ||
+	{
+		status=$?
+		rm -f "$GIT_DIR"/CHERRY_PICK_HEAD
+		return $status
+	}
 }
 
 pick_one_preserving_merges () {
@@ -308,7 +313,10 @@ pick_one_preserving_merges () {
 			;;
 		*)
 			output git cherry-pick "$@" ||
+			{
+				rm -f "$GIT_DIR"/CHERRY_PICK_HEAD
 				die_with_patch $sha1 "Could not pick $sha1"
+			}
 			;;
 		esac
 		;;
diff --git a/sequencer.c b/sequencer.c
index a37846a..c2eceb5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -129,12 +129,6 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 
 	if (msg) {
 		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occured but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
-		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
 		return;
 	}
 
-- 
1.7.10.rc1.22.gf5241

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Junio C Hamano]

Andrew Wong <andrew.kw.w@gmail.com> writes:

> For example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if
> 'commit' failed due to an empty commit. Letting 'rebase -i' deal with
> removing CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.

Hmmm.

Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the
sequencer when it is not know if it is needed, instead of the current code
which seems to create first and then selectively try to unlink() it?

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 03/19/2012 12:51 PM, Junio C Hamano wrote:
> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the
> sequencer when it is not know if it is needed, instead of the current code
> which seems to create first and then selectively try to unlink() it?
>   
I do agree that it's not pretty to create the file, then quickly delete
it. I did consider putting a condition around the code that creates
CHERRY_PICK_HEAD and REVERT_HEAD.

A possible condition would be checking the env var GIT_CHERRY_PICK_HELP,
which is only set if 'cherry-pick' is called under 'rebase -i'. I never
liked how we're passing in a help message using an env var, so I don't
feel like introducing another dependency on this env var is a good idea.

Another possible condition would be to add another flag to
"cherry-pick". But a proper implementation would not only involve adding
code to parse the flag in 'cherry-pick', but also adding code to
save/restore the option in sequencer, even though 'rebase -i' only need
it for single_pick. It's not that adding these codes are difficult, but
it seems like we're adding a lot of code just to add a behavior that
only 'rebase -i' needs.

Though if the additional flag in "cherry-pick" and additional option in
sequencer could be useful elsewhere, I could do it that way too.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 12-03-19 5:00 PM, Andrew Wong wrote:
> On 03/19/2012 12:51 PM, Junio C Hamano wrote:
>> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the
>> sequencer when it is not know if it is needed, instead of the current code
>> which seems to create first and then selectively try to unlink() it?
>>
> Though if the additional flag in "cherry-pick" and additional option in
> sequencer could be useful elsewhere, I could do it that way too.
I looked into adding a "no-state" flag in 'cherry-pick' to not create 
the CHERRY_PICK_HEAD, but 'commit' actually has several dependencies on 
CHERRY_PICK_HEAD, such as recording reflog message, 'prepare-commit-msg' 
hook, and formatting a user message. So if we want to continue to pursue 
this path, we'd have to preserve those behaviors in 'commit' as well. 
It's probably not a good idea to make all these changes in  
'cherry-pick' and 'commit' just to avoid a simple cleanup in 'rebase 
-i'. So I still prefer the patch I submitted earlier.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 03/24/2012 04:03 PM, Andrew Wong wrote:
> On 12-03-19 5:00 PM, Andrew Wong wrote:
>> On 03/19/2012 12:51 PM, Junio C Hamano wrote:
>>> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the
>>> sequencer when it is not know if it is needed, instead of the
>>> current code
>>> which seems to create first and then selectively try to unlink() it?
>>>
>> Though if the additional flag in "cherry-pick" and additional option in
>> sequencer could be useful elsewhere, I could do it that way too.
> I looked into adding a "no-state" flag in 'cherry-pick' to not create
> the CHERRY_PICK_HEAD, but 'commit' actually has several dependencies
> on CHERRY_PICK_HEAD, such as recording reflog message,
> 'prepare-commit-msg' hook, and formatting a user message. So if we
> want to continue to pursue this path, we'd have to preserve those
> behaviors in 'commit' as well. It's probably not a good idea to make
> all these changes in  'cherry-pick' and 'commit' just to avoid a
> simple cleanup in 'rebase -i'. So I still prefer the patch I submitted
> earlier.
Can we look into queuing this patch? Or does anyone have any thoughts on
this?

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Junio C Hamano]

Andrew Wong <andrew.w-lists@sohovfx.com> writes:

> Can we look into queuing this patch? Or does anyone have any thoughts on
> this?

I do not recall if I convinced myself that the patch was fixing the right
problem, or it does not look like it would break other cases; reviews from
interested parties are very much appreciated.

This fell through the crack. Thanks for sending a reminder.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Andrew Wong <andrew.w-lists@sohovfx.com> writes:
>
>> Can we look into queuing this patch? Or does anyone have any thoughts on
>> this?
>
> I do not recall if I convinced myself that the patch was fixing the right
> problem, or it does not look like it would break other cases; reviews from
> interested parties are very much appreciated.
>
> This fell through the crack. Thanks for sending a reminder.

And I forgot to Cc who is responsible for that CHERRY_PICK_HEAD part...

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Ramkumar Ramachandra]

Hi Andrew,

[+CC: Jonathan Nieder]

Andrew Wong wrote:
> Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD
> for its caller's sake, let its caller do the work. This way, the
> sequencer doesn't have to check all points of failures where its caller
> doesn't want CHERRY_PICK_HEAD.

This part makes sense.

> For example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if
> 'commit' failed due to an empty commit. Letting 'rebase -i' deal with
> removing CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.

Yes, that's because git-commit is spawned.  The sequencer has no way
to tell if the commit was actually successful.  Incidentally, what is
your motivation for this patch?  Did the "rebase -i" or the sequencer
misbehave in some scenario?  Wouldn't it make sense to add a failing
test for that scenario first?

Also, note that in a previous iteration, we considered the possibility
of making git-commit remove CHERRY_PICK_HEAD, but decided that it
would be ugly subsequently.

> A possible condition would be checking the env var GIT_CHERRY_PICK_HELP,
> which is only set if 'cherry-pick' is called under 'rebase -i'. I never
> liked how we're passing in a help message using an env var, so I don't
> feel like introducing another dependency on this env var is a good idea.

True; this is ugly.  It detracts us from the purpose of the patch
which is to shift the responsibility of cleaning up CHERRY_PICK_HEAD
to the caller.

> Another possible condition would be to add another flag to
> "cherry-pick". But a proper implementation would not only involve adding
> code to parse the flag in 'cherry-pick', but also adding code to
> save/restore the option in sequencer, even though 'rebase -i' only need
> it for single_pick. It's not that adding these codes are difficult, but
> it seems like we're adding a lot of code just to add a behavior that
> only 'rebase -i' needs.

Another ugly solution.

> Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
> [...]

Bonus: After this patch, the sequencer code is symmetric in
CHERY_PICK_HEAD and REVERT_HEAD.  How do we convince ourselves that
we're not breaking some corner case though?  I'd be more comfortable
with the patch if you can present a failing test first.

Thanks.

    Ram

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

(cc-ing Jay, expert on the CHERRY_PICK_HEAD facility)
Hi all,

Ramkumar Ramachandra wrote:
> Andrew Wong wrote:

>> Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD
>> for its caller's sake, let its caller do the work. This way, the
>> sequencer doesn't have to check all points of failures where its caller
>> doesn't want CHERRY_PICK_HEAD.
>
> This part makes sense.

Sorry, I think I've missed the point.  Can you explain to me what
problem this is solving, aside from somehow dividing responsibility
for the CHERRY_PICK_HEAD file among different tools?

>From the surrounding thread, it looks like the following sequence of
commands is at stake:

	git checkout -b tmp v1.7.9
	git cherry-pick 6e1c9bb^2^
	git rebase -i --onto 6e1c9bb HEAD^
	git rebase --continue

The pick works fine and is just part of the setup.  The rebase produces

	The previous cherry-pick is now empty, possibly due to conflict resolution.
	If you wish to commit it anyway, use:

	    git commit --allow-empty

	Otherwise, please use 'git reset'

CHERRY_PICK_HEAD points to "run-command: optionally kill children on
exit" to help the user understand how to resolve the conflict.
Normally print_advice() would remove it because the caller has set
GIT_CHERRY_PICK_HELP to indicate that it wants to use some other
mechanism than "git commit" to deal with resolved conflicts.
Unfortunately the GIT_CHERRY_PICK_HELP facility does not give the
caller a way to specify an alternative message for this case, like:

	The previous cherry-pick is now empty, possibly due to conflict resolution.

	When you have resolved this problem run "git rebase --continue".
	If you would prefer to skip this patch, instead run "git rebase --skip".
	To check out the original branch and stop rebasing run "git rebase --abort".

In fact, "git cherry-pick" does not handle this case at all.  It lets
"git commit" notice the lack of change.  "git commit" emits a message
and follows the usual rules for a failed commit, including preserving
CHERRY_PICK_HEAD to help the operator clean up.

Ok.  Now the user (sensibly) ignores the message from cherry-pick and
just runs "git rebase --continue".  The rebase finishes but nobody
feels it's his responsibility to remove the .git/CHERRY_PICK_HEAD file
and it gets left behind.

For symptom relief, your patch makes sense, though I haven't checked
it in detail yet.  The description distracted me --- it would be
better to say "this sequence of commands has this bad consequence;
this patch papers over the problem to make people happier until the
underlying problem can be addressed" instead of pretending the design
was almost sane and we are just fixing the last detail. ;-)

I suspect a more appropriate long-term fix would involve "git
cherry-pick" noticing when a patch has resolved to nothing instead of
leaving it to "git commit" to detect that.

Sensible?
Jonathan

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 04/03/2012 10:45 AM, Jonathan Nieder wrote:
> Ok.  Now the user (sensibly) ignores the message from cherry-pick and
> just runs "git rebase --continue".  The rebase finishes but nobody
> feels it's his responsibility to remove the .git/CHERRY_PICK_HEAD file
> and it gets left behind.
>   
Yes, that's exactly what's happening. That particular rebase will leave
behind CHERRY_PICK_HEAD, which have bad consequences such as:
1. Confuses __git_ps1 (from git-completion.bash) into thinking a
cherry-pick is still in progress. (which is what started this discussion)
2. Cause "cherry-pick --continue" to think a cherry-pick is still in
progress.
3. Similarly, if a user then continue on to modifying their files, and
do a "add" and "commit", "commit" would reuse the message from the
CHERRY_PICK_HEAD.

> I suspect a more appropriate long-term fix would involve "git
> cherry-pick" noticing when a patch has resolved to nothing instead of
> leaving it to "git commit" to detect that.
>   
I actually tried implementing a fix like that too. But then I thought
there might be other scenarios where "commit" could fail, and it doesn't
seem to make sense for "cherry-pick" to have to detect all possible
"commit" failures. Though it also feels like the question of whether or
not "cherry-pick" should detects the "empty commit" is a separate issue
altogether.

Besides the "empty commit" failure, "cherry-pick" can still run into
various errors, such as merge conflict. So it will have to keep a state
somehow. And instead of having "cherry-pick" make special cases for
"rebase -i" to remove the state, it makes more sense to teach "rebase
-i" that "cherry-pick" now keeps a state on failure. So if "cherry-pick"
fails, "rebase -i" is responsible for clearing that state. And that's
what this patch is supposed to do.

Perhaps I should rephrase my description to reflect this better?
Something along the line of: "cherry-pick" now keeps a state on failure.
Instead of having a special case inside the sequencer to remove the
state, we teach "rebase -i" that we need to clear the state.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Andrew Wong wrote:

> Besides the "empty commit" failure, "cherry-pick" can still run into
> various errors, such as merge conflict.

Cherry-pick does the merge, so it is what notices the merge conflict.
If you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant
code should show up.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Jonathan Nieder wrote:
> Andrew Wong wrote:

>> Besides the "empty commit" failure, "cherry-pick" can still run into
>> various errors, such as merge conflict.
>
> Cherry-pick does the merge, so it is what notices the merge conflict.
> If you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant
> code should show up.

I was looking at an older git version.  In newer gits, the code path
in question lives at print_advice() in sequencer.c.

Sorry for the noise.
Jonathan

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 04/03/2012 05:12 PM, Jonathan Nieder wrote:
> Jonathan Nieder wrote:
>   
>> Cherry-pick does the merge, so it is what notices the merge conflict.
>> If you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant
>> code should show up.
>>     
> I was looking at an older git version.  In newer gits, the code path
> in question lives at print_advice() in sequencer.c.
>   
Yes, the code has been moved into sequencer now.

But what I meant was, regardless of who's calling "cherry-pick", if
"cherry-pick" runs into an error and needs to stop, it needs to save a
state so that it can do a "--continue". And this behavior should stay
the same regardless of who the caller is. And that means its callers
(e.g. "rebase -i") should know about this and do a cleanup when
"cherry-pick" failed.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Andrew Wong wrote:

> But what I meant was, regardless of who's calling "cherry-pick", if
> "cherry-pick" runs into an error and needs to stop, it needs to save a
> state so that it can do a "--continue". And this behavior should stay
> the same regardless of who the caller is. And that means its callers
> (e.g. "rebase -i") should know about this and do a cleanup when
> "cherry-pick" failed.

The current CHERRY_PICK_HELP codepath removes CHERRY_PICK_HEAD to let
its caller take care of the appropriate "commit -c" magic for
historical reasons.  I'd be happy to see "rebase -i" stop relying on
that.

Unfortunately, outside scripts from before CHERRY_PICK_HEAD existed
are also allowed to use CHERRY_PICK_HELP, so the incomplete
implementation that leaves behind a CHERRY_PICK_HEAD when the commit
being cherry-picked resolves into nothingness is still a bug.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 04/03/2012 05:26 PM, Jonathan Nieder wrote:
> The current CHERRY_PICK_HELP codepath removes CHERRY_PICK_HEAD to let
> its caller take care of the appropriate "commit -c" magic for
> historical reasons.  I'd be happy to see "rebase -i" stop relying on
> that.
>   
"rebase -i" doesn't rely on "commit -c". It stores the author info
inside its state dir, so when the user does a "rebase --continue", the
author info from the state dir is used.

Also, "commit -c" does override CHERRY_PICK_HEAD for author info and
message. So having CHERRY_PICK_HEAD around shouldn't affect scripts that
rely on "commit -c".

> Unfortunately, outside scripts from before CHERRY_PICK_HEAD existed
> are also allowed to use CHERRY_PICK_HELP,so the incomplete
> implementation that leaves behind a CHERRY_PICK_HEAD when the commit
> being cherry-picked resolves into nothingness is still a bug.
>   
CHERRY_PICK_HELP was introduced as a hack to allow "rebase -i" to
override what message "cherry-pick" shows, and not as something that
affects the behavior of "cherry-pick". Since it is also not documented,
it feels more like an internal implementation detail. So I suspect not
many people know about this env var and even fewer would actually use it.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Andrew Wong wrote:

> CHERRY_PICK_HELP was introduced as a hack

Do you mean we should get rid of CHERRY_PICK_HELP?

Note that you don't have to convince me of anything --- I was just
trying to help by providing my reactions and some background.  If the
Right Thing to Do as revealed by list consensus or Junio's decree ;-)
involves keeping CHERRY_PICK_HELP difficult to use and fragile, I
won't mind much as long as rebase -i works, since I don't have any
private scripts that use CHERRY_PICK_HELP personally.

Sorry for the lack of clarity.
Jonathan

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Andrew Wong]

On 04/04/2012 02:11 PM, Jonathan Nieder wrote:
> Do you mean we should get rid of CHERRY_PICK_HELP?
>   
No. I thought you meant that if "cherry-pick" runs into an error, it
should not leave behind a state (i.e. CHERRY_PICK_HEAD) when
CHERRY_PICK_HELP is defined. And I was arguing that defining
CHERRY_PICK_HELP shouldn't affect the behavior of "cherry-pick" at all.
So it shouldn't be trying to remove the state in the first place. The
cleanup responsibility should fall into caller of "cherry-pick". i.e.
"rebase -i"

Though I now think that my original patch description could be improved
to better reflect that.

And you also mention earlier that the patch is more of a symptom relief,
and that
> a more appropriate long-term fix would involve "git
> cherry-pick" noticing when a patch has resolved to nothing instead of
> leaving it to "git commit" to detect that.
And I was arguing that "cherry-pick" doesn't have to detect scenarios
where "commit" could fail. Since there could be other scenarios where
"commit" could fail and "cherry-pick" is already handling "commit"
failing, I think there's no need for "cherry-pick" to handle an empty
commit specifically.

So if the list or Junio thinks that the patch is the right thing to do,
I should improve on the patch description before we queue it.

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Andrew Wong wrote:

>                            Since there could be other scenarios where
> "commit" could fail

As far as I can tell, there just aren't any such other scenarios,
unless you mean like running out of memory or disk space.  "git
cherry-pick" disables hooks when running "git commit" so the
pre-commit hook can't block the commit.

So the scenarios fall into three or so categories.

 - when "git cherry-pick" performs a merge and encounters conflicts,
   it prints a message and exits, writing CHERRY_PICK_HEAD to tell
   the operator what command to use (instead of "git commit" or
   "git cherry-pick --continue") when the problem is resolved.

   If my script sets GIT_CHERRY_PICK_HELP, it will print a different
   message and does not write CHERRY_PICK_HEAD because the operator
   is going to run "myscript --resume" and not "git commit" or "git
   cherry-pick --continue" when the problem is resolved.

 - when "git cherry-pick" performs a clean merge that produces no
   change, "git commit" prints a message about a missing --allow-empty
   argument and exits.

   My GIT_CHERRY_PICK_HELP setting is not respected, so the user is
   likely to run "git commit" or "git cherry-pick --continue" instead
   of the command I wanted.

 - when "git cherry-pick" performs a clean merge that produces a
   change but "git commit" fails to record it due to a stray signal or
   running out of disk space, git does not print any advice for the
   operator.

   In particular, my GIT_CHERRY_PICK_HELP setting is not respected.
   Also, CHERRY_PICK_HEAD is written even though my wrapper script
   that sets GIT_CHERRY_PICK_HELP didn't expect that.

   The operator can return to a familar state with "git reset --hard"
   followed by "git checkout" of some familiar branch, except that my
   script may be keeping some state of its own that lingers until the
   operator tries to use it again...

I was focusing on the second category.  Using a stock message instead
of the custom message from GIT_CHERRY_PICK_HELP certainly seems to me
like a bug or incomplete feature.

When you say that there are other ways for "git commit" to fail and
Junio says that in some cases "git cherry-pick" should not write
CHERRY_PICK_HEAD at all, you are probably thinking of the third
category.

Hope that helps,
Jonathan

Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed

[Jonathan Nieder]

Jonathan Nieder wrote:

>  - when "git cherry-pick" performs a merge and encounters conflicts,
>    it prints a message and exits, writing CHERRY_PICK_HEAD to tell
>    the operator what command to use (instead of "git commit" or
>    "git cherry-pick --continue") when the problem is resolved.

The above paragraph doesn't make any sense.  I meant:

	When cherry-pick encounters conflicts, it prints a message
	telling the operator what command to use (namely "git commit"
	or "git cherry-pick --continue") once the problem is resolved
	and writes CHERRY_PICK_HEAD to make that command work.

Sorry for the noise.

>    If my script sets GIT_CHERRY_PICK_HELP, it will print a different
>    message and cherry-pick does not write CHERRY_PICK_HEAD because
>    the operator is going to run "myscript --resume" instead of
>    "git commit" or "git cherry-pick --continue" when the problem is
>    resolved.

Jonathan