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@xxxxxxxxx>
> [...]
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html