Rebase regression in v1.7.9?

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.

On 01/31/2012 05:56 PM, Felipe Contreras wrote:

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?

On Wed, Feb 1, 2012 at 7:27 PM, Andrew Wong <andrew.w@sohovfx.com> wrote:

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.

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(-)

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

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?

On 03/19/2012 12:51 PM, Junio C Hamano wrote:

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.

On 12-03-19 5:00 PM, Andrew Wong wrote:

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.

On 03/24/2012 04:03 PM, Andrew Wong wrote:

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

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

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.

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

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

Hi Andrew,

[+CC: Jonathan Nieder]

Andrew Wong wrote:

This part makes sense.

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.

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 ugly solution.

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

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

Ramkumar Ramachandra wrote:

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?

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

On 04/03/2012 10:45 AM, Jonathan Nieder wrote:

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 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.

Andrew Wong 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.

Jonathan Nieder wrote:

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

On 04/03/2012 05:12 PM, Jonathan Nieder wrote:

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.

Andrew Wong 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.

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.

On 04/03/2012 05:26 PM, Jonathan Nieder wrote:

"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".

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.

Andrew Wong wrote:

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

On 04/04/2012 02:11 PM, Jonathan Nieder wrote:

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

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.

Andrew Wong wrote:

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

Jonathan Nieder wrote:

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.

Jonathan