git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Apr 3, 2012, 06:32 UTC
Message-ID
<CALkWK0nmNWaOKcyGH2N0s3B1AFD-+3vHz1BBc3U=RMEFLNuc7A@mail.gmail.com>
In-Reply-To
<1332106632-31882-1-git-send-email-andrew.kw.w@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.

Show 7 quoted lines
> 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
Previous: Junio C HamanoNext: Jonathan Nieder
Message 11 of 22 in “Rebase regression in v1.7.9?”
  1. Felipe ContrerasJan 31, 2012
  2. Andrew WongFeb 1, 2012
  3. Felipe ContrerasFeb 1, 2012
  4. rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failedAndrew Wong, Mar 18, 2012
  5. Junio C HamanoMar 19, 2012
  6. Andrew WongMar 19, 2012
  7. Andrew WongMar 24, 2012
  8. Andrew WongApr 2, 2012
  9. Junio C HamanoApr 2, 2012
  10. Junio C HamanoApr 3, 2012
  11. Ramkumar RamachandraApr 3, 2012
  12. Jonathan NiederApr 3, 2012
  13. Andrew WongApr 3, 2012
  14. Jonathan NiederApr 3, 2012
  15. Jonathan NiederApr 3, 2012
  16. Andrew WongApr 3, 2012
  17. Jonathan NiederApr 3, 2012
  18. Andrew WongApr 3, 2012
  19. Jonathan NiederApr 4, 2012
  20. Andrew WongApr 4, 2012
  21. Jonathan NiederApr 4, 2012
  22. Jonathan NiederApr 4, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.