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

[Ramkumar Ramachandra <artagnon@gmail.com>]

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