Re: [BUG] cherry-pick ignores some arguments

["Carlos Martín Nieto" <cmn@elego.de>]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

On Fri, 2012-06-15 at 09:14 +0200, Yann Dirson wrote:
> On Thu, 14 Jun 2012 18:29:49 +0200 Carlos Martín Nieto <cmn@elego.de> wrote:
> > On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:
> > > 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.
> > > 
> > 
> > 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.
> 
> 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!

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.

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

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

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

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]