Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 8, 2012, 20:22 UTC
Message-ID: <20120108202216.GL1942@burratino>
In-Reply-To: <CALkWK0=-AWy7HnVASB1rt8njavTYOhV7Zxsdq4TE+VShVZmEzQ@mail.gmail.com>

Ramkumar Ramachandra wrote:

> Jonathan Nieder wrote:

Show 9 quoted lines

>> I don't know --- it's not confusing to me.  Could you explain further
>> what harm the current behavior does?  E.g., could it cause me to
>> misunderstand some basic concepts, or could it lead me to run commands
>> that cause me to scratch my head or lose data?
>
> Junio explained this to me in [1].  It's very unnatural for a user to
> want to execute "git cherry-pick --continue" when the previous command
> was a "git revert": it probably means that she forgot about the
> in-progress "git revert".

[...]

> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355

I don't think that's what Junio said.

Did this actually happen, or is it a theoretical worry? I think I would be more likely to run "git cherry-pick <foo>..<bar>" than "git cherry-pick --continue" if I had forgotten about an in-progress revert. The former already errors out with a sensible message.

Or is the problem that I might run:

	git revert foo..bar
	git reset --merge; # conflict --- let's clean this up

	# ah, I remember reverting the patch that conflicted before;
	# let's reuse the resolution.
	git cherry-pick baz
	edit file.c; # another conflict, sigh
	git add file.c
	git cherry-pick --continue; # oops!

? That seems like a real worry, but the same problem could happen with cherry-pick used both for the multipick and single-pick, so I don't think your patch fundamentally addresses it.

In other words, this is a problem caused by the overloading of the same cherry-pick command for single-pick and multi-pick. I think it should be preventable by remembering which action failed when stopping a sequence and doing only a single-pick resume if CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.

The "oops" is bad since the operator might have been intending to run some more tests and amend as necessary before continuing the multi-pick. It is not _that_ bad, since more typically one would have already run some tests before running cherry-pick --continue to commit the resolution. Still probably worth fixing.

Show 5 quoted lines

> The problem becomes more serious when the
> sequencer grows more capabilities: a "git merge --continue" to
> continue a "git am" sounds much more absurd.  Ofcourse, we will
> provide a way to continue any sequencer operation in the future: "git
> continue" seems to be a good candidate.

I don't understand why "cherry-pick --continue" resuming a revert sequence implies that "merge --continue" would have to as well.

All that said, forbidding cherry-pick --continue from resuming a revert sequence would be fine with me, _as long as the semantics are clearly spelled out in the commit message and documentation_. What happens when there is a mixture of picks and reverts?

Thanks. Jonathan