* [BUG] cherry-pick ignores some arguments @ 2012-06-14 9:44 Yann Dirson 2012-06-14 16:29 ` Carlos Martín Nieto 0 siblings, 1 reply; 11+ messages in thread From: Yann Dirson @ 2012-06-14 9:44 UTC (permalink / raw) To: git list 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-14 9:44 [BUG] cherry-pick ignores some arguments Yann Dirson @ 2012-06-14 16:29 ` Carlos Martín Nieto 2012-06-15 7:14 ` Yann Dirson 2012-06-15 15:03 ` Junio C Hamano 0 siblings, 2 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2012-06-14 16:29 UTC (permalink / raw) To: Yann Dirson; +Cc: git list [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-14 16:29 ` Carlos Martín Nieto @ 2012-06-15 7:14 ` Yann Dirson 2012-06-15 13:12 ` Carlos Martín Nieto 2012-06-15 15:06 ` Junio C Hamano 2012-06-15 15:03 ` Junio C Hamano 1 sibling, 2 replies; 11+ messages in thread From: Yann Dirson @ 2012-06-15 7:14 UTC (permalink / raw) To: git list; +Cc: Carlos Martín Nieto 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-15 7:14 ` Yann Dirson @ 2012-06-15 13:12 ` Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Carlos Martín Nieto 2012-06-15 14:39 ` [BUG] cherry-pick ignores some arguments Carlos Martín Nieto 2012-06-15 15:06 ` Junio C Hamano 1 sibling, 2 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2012-06-15 13:12 UTC (permalink / raw) To: Yann Dirson; +Cc: git list [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] Documentation: --no-walk is no-op if range is specified 2012-06-15 13:12 ` Carlos Martín Nieto @ 2012-06-15 14:33 ` Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated Carlos Martín Nieto 2012-06-15 17:37 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Junio C Hamano 2012-06-15 14:39 ` [BUG] cherry-pick ignores some arguments Carlos Martín Nieto 1 sibling, 2 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2012-06-15 14:33 UTC (permalink / raw) To: git; +Cc: Yann Dirson 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated 2012-06-15 14:33 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Carlos Martín Nieto @ 2012-06-15 14:33 ` Carlos Martín Nieto 2012-06-15 17:52 ` Junio C Hamano 2012-06-15 17:37 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Junio C Hamano 1 sibling, 1 reply; 11+ messages in thread From: Carlos Martín Nieto @ 2012-06-15 14:33 UTC (permalink / raw) To: git; +Cc: Yann Dirson 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated 2012-06-15 14:33 ` [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated Carlos Martín Nieto @ 2012-06-15 17:52 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2012-06-15 17:52 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git, Yann Dirson 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`. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] Documentation: --no-walk is no-op if range is specified 2012-06-15 14:33 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated Carlos Martín Nieto @ 2012-06-15 17:37 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2012-06-15 17:37 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: git, Yann Dirson 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-15 13:12 ` Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Carlos Martín Nieto @ 2012-06-15 14:39 ` Carlos Martín Nieto 1 sibling, 0 replies; 11+ messages in thread From: Carlos Martín Nieto @ 2012-06-15 14:39 UTC (permalink / raw) To: Yann Dirson; +Cc: git list [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-15 7:14 ` Yann Dirson 2012-06-15 13:12 ` Carlos Martín Nieto @ 2012-06-15 15:06 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2012-06-15 15:06 UTC (permalink / raw) To: Yann Dirson; +Cc: git list, Carlos Martín Nieto 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] cherry-pick ignores some arguments 2012-06-14 16:29 ` Carlos Martín Nieto 2012-06-15 7:14 ` Yann Dirson @ 2012-06-15 15:03 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2012-06-15 15:03 UTC (permalink / raw) To: Carlos Martín Nieto; +Cc: Yann Dirson, git list 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-06-15 17:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-06-14 9:44 [BUG] cherry-pick ignores some arguments Yann Dirson 2012-06-14 16:29 ` Carlos Martín Nieto 2012-06-15 7:14 ` Yann Dirson 2012-06-15 13:12 ` Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Carlos Martín Nieto 2012-06-15 14:33 ` [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated Carlos Martín Nieto 2012-06-15 17:52 ` Junio C Hamano 2012-06-15 17:37 ` [PATCH 1/2] Documentation: --no-walk is no-op if range is specified Junio C Hamano 2012-06-15 14:39 ` [BUG] cherry-pick ignores some arguments Carlos Martín Nieto 2012-06-15 15:06 ` Junio C Hamano 2012-06-15 15:03 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).