[BUG] cherry-pick ignores some arguments

Hello list,

I just did a "git cherry-pick AAA BBB..CCC" using 1.7.10.3, and was surprised that only the BBB..CCC range got picked - AAA was silently ignored.

On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:

There is no way to know whether this is a bug without knowing how AAA, BBB and ccc are related? From the names, can we assume that AAA is a (grand)parent of BBB? If that is the case, cherry-pick is behaving as expected.

See the DESCRIPTION in http://git-scm.com/docs/git-rev-list for further explanation, but the short of the story is that the second argument told it to ignore any commit before BBB, so AAA is not in the list of commits to be applied.

   cmn

On Thu, 14 Jun 2012 18:29:49 +0200 Carlos Martín Nieto <cmn@elego.de> wrote:

OK, this is exactly the case. Looking back at the cherry-pick manpage, I'd say that what confused me is the implicit --no-walk: the standard "git cherry-pick AAA" does not look like a rev-list spec at all!

At least for this command, it would seem more natural (to me at least) to take each arg one by one and feed it to "rev-list --no-walk" or similar. Maybe some special rev-list flag could trigger such a particular behaviour, pretty much like what --no-walk does ?

Another orthogonal UI issue I see, is that rev-list could be more user-friendly to warn the user when one element of a rev list is ignored because of another one. Not sure whether this would be useful for all explicit rev lists specified by the user - maybe a config var and associated option would be needed too.

On Fri, 2012-06-15 at 09:14 +0200, Yann Dirson wrote:

The typical cherry-pick usage is for a few select commits out of a different branch. The manpage itself only started explaining the ranges in 2010 and they may be more of a side-effect than a conscious design decision. But that's neither here nor there.

This would cause a regression, as passing it "A..B" is the same as "B ^A" which is spellt as two different arguments. Making

    git cherry-pick B ^A

internally cause

    git cherry-pick B
    git cherry-pick ^A

to be called would cause the wrong thing to happen. Instead of cherry-picking the commits between B and A, it would cherry-pick B and then do nothing in the second run (as there were no positive commits specified).

Doing it by default is not an option, as that would start causing all sorts of commands and scripts to start warning during normal operation with an error message that comes completely out of the blue from the user's perspective. It's a perfectly valid thing to give it positive references that are hidden by other arguments.

Another thing is that rev-list is plumbing so it's not allowed to change (and it's not something users would generally be using). What I see looking at the cherry-pick manpage is that it doesn't mention what happens when you do ask rev-list to walk (which is what you do by giving it a range). Though it does say that no traversal is done by default, it doesn't say how you override that default. The EXAMPLES section isn't that clear either, and the explanation for rev-list's --no-walk isn't much help either. I'll try to create a couple of patches to make the behaviour clearer.

   cmn

The existing description can be misleading and cause the reader to think that --no-walk will do something if they specify a range in the command line instead of a set of revs.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 Documentation/rev-list-options.txt | 1 +
 1 file changed, 1 insertion(+)

When given a set of commits, cherry-pick will apply the changes for all of them. Specifying a simple range will also work as expected. This can cause the user to think that

    git cherry-pick A B..C

will apply A and then B..C. This is not what happens. Instead the revs are given to rev-list which will consider A and C as positive revs and B as a negative one. Add a note about this and add an example with this particular syntax, which has shown up on the list a few times.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 Documentation/git-cherry-pick.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

Carlos Martín Nieto <cmn@elego.de> writes:

That is not wrong per-se, but I do not think it would have helped Yann. How about phrasing it this way?

	Note that specifying a range will feed all
	<commit>... arguments to a single revision walk (see a later
	example that uses 'maint master..next').

If you are using these three branches because you expect familiarity with the convention of maint < master < next on the reader's side, I think it should be rewritten like this.

`git cherry-pick maint next ^master`:: `git cherry-pick maint master..next`::

	Apply the changes introduced by all commits that are
	ancestors of maint or next, but not master or any of its
	ancestors.  Note that the latter does not mean `maint` and
	everything between `master` and `next`; specifically,
	`maint` will not be used if it is included in `master`.

Carlos Martín Nieto <cmn@elego.de> writes:

This is correct as a description of the current behaviour, but I have to wonder if we should error out when the user explicitly (i.e. the implicit uses of --no-walk by "show" and "cherry-pick" need to be treated differently) gives --no-walk and a negative commit (either by A..B range, or a separate ^A).

Would that break a valid script, and if not, how involved would such a fix be?

On Fri, 2012-06-15 at 15:12 +0200, Carlos Martín Nieto wrote:

Disregard this part. I just found the patches. For some reason I thought that the capability was there much earlier.

   cmn

Yann Dirson <dirson@bertin.fr> writes:

Sometimes my maint-1.7.8 branch may still have commits that are not in maint branch, sometimes maint-1.7.9 branch is a true subset of maint branch. "rev-list ^maint maint-1.7.8 maint-1.7.9" is a very sensible thing to ask to find out what is still not in 'maint' out of stuff that are _usually_ but not always part of 'maint'.

Complaining on such a query is not user-friendly at all.

Carlos Martín Nieto <cmn@elego.de> writes:

That is correct from the "rev-list" point of view. The request is telling us, by having BBB on the LHS of ".." (which is the same as saying "^BBB"), that nothing that is an ancestor of BBB should be used, so if AAA happens to be behind BBB, it won't be picked.

In the context of "cherry-pick", "show", and "format-patch", however, "I want AAA and things *between* BBB and CCC" is not an unreasonable thing to ask [*1*]. You may be trying to port the feature implemented on your 'master' branch by commits in the consecutive range BBB..CCC to your 'maint' branch, but the implementation may happen to depend on an unrelated fix AAA that also is on your 'master' branch that came before BBB.

It's just that the existing "AAA BBB..CCC" syntax is *not* a way to ask for that semantics, as it has an established "rev-list" meaning you explained. Obviously you could say

	git cherry-pick AAA $(git rev-list BBB..CCC)

to get that semantics, but it is a mouthful to say.

I am OK if somebody comes up with a different syntax to allow users to say "I have multiple range expressions. Please grab sets of commits from them *separately*, and give me a *union* of them".

It is OK to add such a feature---it will have to be a lot more expensive from latency point of view (i.e. such a query cannot stream and always have to be "limited" in rev-list sense)---as long as such a change will not hurt performance and semantics of simpler cases.

[Footnote]

*1* I've said this a few times here, but the way "show --do-walk" walks the history is an ugly hack that merely happens to appear to work sometimes but is done in a wrong way.