[BUG] cherry-pick ignores some arguments

[BUG] cherry-pick ignores some arguments

[Yann Dirson]

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.

-- 
Yann Dirson - Bertin Technologies

Re: [BUG] cherry-pick ignores some arguments

[Carlos Martín Nieto]

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

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.

   cmn


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

Re: [BUG] cherry-pick ignores some arguments

[Yann Dirson]

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!

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.

-- 
Yann Dirson - Bertin Technologies

Re: [BUG] cherry-pick ignores some arguments

[Carlos Martín Nieto]

[-- 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 --]

[PATCH 1/2] Documentation: --no-walk is no-op if range is specified

[Carlos Martín Nieto]

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

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1ae3c89..84e34b1 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -622,6 +622,7 @@ These options are mostly targeted for packing of git repositories.
 --no-walk::
 
 	Only show the given revs, but do not traverse their ancestors.
+	This has no effect if a range is specified.
 
 --do-walk::
 
-- 
1.7.10.2.520.g6a4a482

[PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated

[Carlos Martín Nieto]

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

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 06a0bfd..10abfbf 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -48,6 +48,7 @@ OPTIONS
 	Sets of commits can be passed but no traversal is done by
 	default, as if the '--no-walk' option was specified, see
 	linkgit:git-rev-list[1].
+	Note that specifying a range will activate revision walking.
 
 -e::
 --edit::
@@ -130,6 +131,15 @@ EXAMPLES
 	Apply the changes introduced by all commits that are ancestors
 	of master but not of HEAD to produce new commits.
 
+`git cherry-pick master next ^maint`::
+`git cherry-pick master maint..next`::
+
+	Apply the changes introduced by all commits that are ancestors
+	of master or next, but not maint or any of its ancestors. The
+	second spelling is often a misunderstanding of revision
+	walking works when trying to apply a range plus a particular
+	commit and included for completeness.
+
 `git cherry-pick master~4 master~2`::
 
 	Apply the changes introduced by the fifth and third last
-- 
1.7.10.2.520.g6a4a482

Re: [BUG] cherry-pick ignores some arguments

[Carlos Martín Nieto]

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

On Fri, 2012-06-15 at 15:12 +0200, Carlos Martín Nieto 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.

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

   cmn


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

Re: [BUG] cherry-pick ignores some arguments

[Junio C Hamano]

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

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

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.

Re: [BUG] cherry-pick ignores some arguments

[Junio C Hamano]

Yann Dirson <dirson@bertin.fr> writes:

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

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.

Re: [PATCH 1/2] Documentation: --no-walk is no-op if range is specified

[Junio C Hamano]

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

> 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(+)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 1ae3c89..84e34b1 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -622,6 +622,7 @@ These options are mostly targeted for packing of git repositories.
>  --no-walk::
>  
>  	Only show the given revs, but do not traverse their ancestors.
> +	This has no effect if a range is specified.
>  
>  --do-walk::

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?

Re: [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated

[Junio C Hamano]

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

> 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(+)
>
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 06a0bfd..10abfbf 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -48,6 +48,7 @@ OPTIONS
>  	Sets of commits can be passed but no traversal is done by
>  	default, as if the '--no-walk' option was specified, see
>  	linkgit:git-rev-list[1].
> +	Note that specifying a range will activate revision walking.

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

> @@ -130,6 +131,15 @@ EXAMPLES
>  	Apply the changes introduced by all commits that are ancestors
>  	of master but not of HEAD to produce new commits.
>  
> +`git cherry-pick master next ^maint`::
> +`git cherry-pick master maint..next`::
> +
> +	Apply the changes introduced by all commits that are ancestors
> +	of master or next, but not maint or any of its ancestors. The
> +	second spelling is often a misunderstanding of revision
> +	walking works when trying to apply a range plus a particular
> +	commit and included for completeness.

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