{"thread":{"id":"30806","subject":"[BUG] cherry-pick ignores some arguments","startedAt":"2012-06-14T09:44:15Z","lastAt":"2012-06-15T17:52:44Z","messageCount":11,"participants":["Yann Dirson","Carlos Martín Nieto","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"193641","messageId":"20120614114415.39cbb64c@chalon.bertin.fr","threadId":"30806","inReplyTo":null,"subject":"[BUG] cherry-pick ignores some arguments","fromName":"Yann Dirson","fromEmail":"dirson@bertin.fr","sentAt":"2012-06-14T09:44:15Z","receivedAt":"2012-06-14T09:44:15Z","isPatch":false,"sender":{"key":"dirson@bertin.fr","avatar":null},"body":"Hello list,\n\nI just did a \"git cherry-pick AAA BBB..CCC\" using 1.7.10.3, and was surprised\nthat only the BBB..CCC range got picked - AAA was silently ignored.\n\n-- \nYann Dirson - Bertin Technologies\n"},{"id":"193660","messageId":"1339691389.4625.9.camel@beez.lab.cmartin.tk","threadId":"30806","inReplyTo":"20120614114415.39cbb64c@chalon.bertin.fr","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-06-14T16:29:49Z","receivedAt":"2012-06-14T16:29:49Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:\n> Hello list,\n> \n> I just did a \"git cherry-pick AAA BBB..CCC\" using 1.7.10.3, and was surprised\n> that only the BBB..CCC range got picked - AAA was silently ignored.\n> \n\nThere is no way to know whether this is a bug without knowing how AAA,\nBBB and ccc are related? From the names, can we assume that AAA is a\n(grand)parent of BBB? If that is the case, cherry-pick is behaving as\nexpected.\n\nSee the DESCRIPTION in http://git-scm.com/docs/git-rev-list for further\nexplanation, but the short of the story is that the second argument told\nit to ignore any commit before BBB, so AAA is not in the list of commits\nto be applied.\n\n cmn\n\n"},{"id":"193684","messageId":"20120615091425.20e40af9@chalon.bertin.fr","threadId":"30806","inReplyTo":"1339691389.4625.9.camel@beez.lab.cmartin.tk","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Yann Dirson","fromEmail":"dirson@bertin.fr","sentAt":"2012-06-15T07:14:25Z","receivedAt":"2012-06-15T07:14:25Z","isPatch":false,"sender":{"key":"dirson@bertin.fr","avatar":null},"body":"On Thu, 14 Jun 2012 18:29:49 +0200 Carlos Martín Nieto wrote:\n> On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:\n> > Hello list,\n> > \n> > I just did a \"git cherry-pick AAA BBB..CCC\" using 1.7.10.3, and was surprised\n> > that only the BBB..CCC range got picked - AAA was silently ignored.\n> > \n> \n> There is no way to know whether this is a bug without knowing how AAA,\n> BBB and ccc are related? From the names, can we assume that AAA is a\n> (grand)parent of BBB? If that is the case, cherry-pick is behaving as\n> expected.\n>\n> See the DESCRIPTION in http://git-scm.com/docs/git-rev-list for further\n> explanation, but the short of the story is that the second argument told\n> it to ignore any commit before BBB, so AAA is not in the list of commits\n> to be applied.\n\nOK, this is exactly the case. Looking back at the cherry-pick manpage, I'd say that\nwhat confused me is the implicit --no-walk: the standard \"git cherry-pick AAA\" does\nnot look like a rev-list spec at all!\n\nAt least for this command, it would seem more natural (to me at least) to take\neach arg one by one and feed it to \"rev-list --no-walk\" or similar. Maybe some\nspecial rev-list flag could trigger such a particular behaviour, pretty much like\nwhat --no-walk does ?\n\n\nAnother orthogonal UI issue I see, is that rev-list could be more user-friendly to warn\nthe user when one element of a rev list is ignored because of another one. Not sure\nwhether this would be useful for all explicit rev lists specified by the user - maybe a\nconfig var and associated option would be needed too.\n\n-- \nYann Dirson - Bertin Technologies\n"},{"id":"193692","messageId":"1339765943.4625.57.camel@beez.lab.cmartin.tk","threadId":"30806","inReplyTo":"20120615091425.20e40af9@chalon.bertin.fr","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-06-15T13:12:23Z","receivedAt":"2012-06-15T13:12:23Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Fri, 2012-06-15 at 09:14 +0200, Yann Dirson wrote:\n> On Thu, 14 Jun 2012 18:29:49 +0200 Carlos Martín Nieto wrote:\n> > On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:\n> > > Hello list,\n> > > \n> > > I just did a \"git cherry-pick AAA BBB..CCC\" using 1.7.10.3, and was surprised\n> > > that only the BBB..CCC range got picked - AAA was silently ignored.\n> > > \n> > \n> > There is no way to know whether this is a bug without knowing how AAA,\n> > BBB and ccc are related? From the names, can we assume that AAA is a\n> > (grand)parent of BBB? If that is the case, cherry-pick is behaving as\n> > expected.\n> >\n> > See the DESCRIPTION in http://git-scm.com/docs/git-rev-list for further\n> > explanation, but the short of the story is that the second argument told\n> > it to ignore any commit before BBB, so AAA is not in the list of commits\n> > to be applied.\n> \n> OK, this is exactly the case. Looking back at the cherry-pick manpage, I'd say that\n> what confused me is the implicit --no-walk: the standard \"git cherry-pick AAA\" does\n> not look like a rev-list spec at all!\n\nThe typical cherry-pick usage is for a few select commits out of a\ndifferent branch. The manpage itself only started explaining the ranges\nin 2010 and they may be more of a side-effect than a conscious design\ndecision. But that's neither here nor there.\n\n> \n> At least for this command, it would seem more natural (to me at least) to take\n> each arg one by one and feed it to \"rev-list --no-walk\" or similar. Maybe some\n> special rev-list flag could trigger such a particular behaviour, pretty much like\n> what --no-walk does ?\n\nThis would cause a regression, as passing it \"A..B\" is the same as \"B\n^A\" which is spellt as two different arguments. Making\n\n git cherry-pick B ^A\n\ninternally cause\n\n git cherry-pick B\n git cherry-pick ^A\n\nto be called would cause the wrong thing to happen. Instead of\ncherry-picking the commits between B and A, it would cherry-pick B and\nthen do nothing in the second run (as there were no positive commits\nspecified).\n\n> \n> \n> Another orthogonal UI issue I see, is that rev-list could be more user-friendly to warn\n> the user when one element of a rev list is ignored because of another one. Not sure\n> whether this would be useful for all explicit rev lists specified by the user - maybe a\n> config var and associated option would be needed too.\n\nDoing it by default is not an option, as that would start causing all\nsorts of commands and scripts to start warning during normal operation\nwith an error message that comes completely out of the blue from the\nuser's perspective. It's a perfectly valid thing to give it positive\nreferences that are hidden by other arguments.\n\nAnother thing is that rev-list is plumbing so it's not allowed to change\n(and it's not something users would generally be using). What I see\nlooking at the cherry-pick manpage is that it doesn't mention what\nhappens when you do ask rev-list to walk (which is what you do by giving\nit a range). Though it does say that no traversal is done by default, it\ndoesn't say how you override that default. The EXAMPLES section isn't\nthat clear either, and the explanation for rev-list's --no-walk isn't\nmuch help either. I'll try to create a couple of patches to make the\nbehaviour clearer.\n\n cmn\n\n"},{"id":"193696","messageId":"1339770796-542-1-git-send-email-cmn@elego.de","threadId":"30806","inReplyTo":"1339765943.4625.57.camel@beez.lab.cmartin.tk","subject":"[PATCH 1/2] Documentation: --no-walk is no-op if range is specified","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-06-15T14:33:15Z","receivedAt":"2012-06-15T14:33:15Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The existing description can be misleading and cause the reader to\nthink that --no-walk will do something if they specify a range in the\ncommand line instead of a set of revs.\n\nSigned-off-by: Carlos Martín Nieto \n---\n Documentation/rev-list-options.txt | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 1ae3c89..84e34b1 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -622,6 +622,7 @@ These options are mostly targeted for packing of git repositories.\n --no-walk::\n \n \tOnly show the given revs, but do not traverse their ancestors.\n+\tThis has no effect if a range is specified.\n \n --do-walk::\n \n-- \n1.7.10.2.520.g6a4a482\n"},{"id":"193695","messageId":"1339770796-542-2-git-send-email-cmn@elego.de","threadId":"30806","inReplyTo":"1339770796-542-1-git-send-email-cmn@elego.de","subject":"[PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-06-15T14:33:16Z","receivedAt":"2012-06-15T14:33:16Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"When given a set of commits, cherry-pick will apply the changes for\nall of them. Specifying a simple range will also work as\nexpected. This can cause the user to think that\n\n git cherry-pick A B..C\n\nwill apply A and then B..C. This is not what happens. Instead the revs\nare given to rev-list which will consider A and C as positive revs and\nB as a negative one. Add a note about this and add an example with\nthis particular syntax, which has shown up on the list a few times.\n\nSigned-off-by: Carlos Martín Nieto \n---\n Documentation/git-cherry-pick.txt | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex 06a0bfd..10abfbf 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -48,6 +48,7 @@ OPTIONS\n \tSets of commits can be passed but no traversal is done by\n \tdefault, as if the '--no-walk' option was specified, see\n \tlinkgit:git-rev-list[1].\n+\tNote that specifying a range will activate revision walking.\n \n -e::\n --edit::\n@@ -130,6 +131,15 @@ EXAMPLES\n \tApply the changes introduced by all commits that are ancestors\n \tof master but not of HEAD to produce new commits.\n \n+`git cherry-pick master next ^maint`::\n+`git cherry-pick master maint..next`::\n+\n+\tApply the changes introduced by all commits that are ancestors\n+\tof master or next, but not maint or any of its ancestors. The\n+\tsecond spelling is often a misunderstanding of revision\n+\twalking works when trying to apply a range plus a particular\n+\tcommit and included for completeness.\n+\n `git cherry-pick master~4 master~2`::\n \n \tApply the changes introduced by the fifth and third last\n-- \n1.7.10.2.520.g6a4a482\n"},{"id":"193697","messageId":"1339771158.4625.59.camel@beez.lab.cmartin.tk","threadId":"30806","inReplyTo":"1339765943.4625.57.camel@beez.lab.cmartin.tk","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-06-15T14:39:18Z","receivedAt":"2012-06-15T14:39:18Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Fri, 2012-06-15 at 15:12 +0200, Carlos Martín Nieto wrote:\n> The typical cherry-pick usage is for a few select commits out of a\n> different branch. The manpage itself only started explaining the ranges\n> in 2010 and they may be more of a side-effect than a conscious design\n> decision. But that's neither here nor there.\n\nDisregard this part. I just found the patches. For some reason I thought\nthat the capability was there much earlier.\n\n cmn\n\n"},{"id":"193699","messageId":"7vaa04d3c8.fsf@alter.siamese.dyndns.org","threadId":"30806","inReplyTo":"1339691389.4625.9.camel@beez.lab.cmartin.tk","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T15:03:51Z","receivedAt":"2012-06-15T15:03:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> On Thu, 2012-06-14 at 11:44 +0200, Yann Dirson wrote:\n>> Hello list,\n>> \n>> I just did a \"git cherry-pick AAA BBB..CCC\" using 1.7.10.3, and was surprised\n>> that only the BBB..CCC range got picked - AAA was silently ignored.\n>> \n>\n> There is no way to know whether this is a bug without knowing how AAA,\n> BBB and ccc are related? From the names, can we assume that AAA is a\n> (grand)parent of BBB? If that is the case, cherry-pick is behaving as\n> expected.\n\nThat is correct from the \"rev-list\" point of view. The request is\ntelling us, by having BBB on the LHS of \"..\" (which is the same as\nsaying \"^BBB\"), that nothing that is an ancestor of BBB should be\nused, so if AAA happens to be behind BBB, it won't be picked.\n\nIn the context of \"cherry-pick\", \"show\", and \"format-patch\",\nhowever, \"I want AAA and things *between* BBB and CCC\" is not an\nunreasonable thing to ask [*1*]. You may be trying to port the\nfeature implemented on your 'master' branch by commits in the\nconsecutive range BBB..CCC to your 'maint' branch, but the\nimplementation may happen to depend on an unrelated fix AAA that\nalso is on your 'master' branch that came before BBB.\n\nIt's just that the existing \"AAA BBB..CCC\" syntax is *not* a way to\nask for that semantics, as it has an established \"rev-list\" meaning\nyou explained. Obviously you could say\n\n\tgit cherry-pick AAA $(git rev-list BBB..CCC)\n\nto get that semantics, but it is a mouthful to say.\n\nI am OK if somebody comes up with a different syntax to allow users\nto say \"I have multiple range expressions. Please grab sets of\ncommits from them *separately*, and give me a *union* of them\".\n\nIt is OK to add such a feature---it will have to be a lot more\nexpensive from latency point of view (i.e. such a query cannot\nstream and always have to be \"limited\" in rev-list sense)---as long\nas such a change will not hurt performance and semantics of simpler\ncases.\n\n\n[Footnote]\n\n*1* I've said this a few times here, but the way \"show --do-walk\"\nwalks the history is an ugly hack that merely happens to appear to\nwork sometimes but is done in a wrong way.\n"},{"id":"193700","messageId":"7v62asd379.fsf@alter.siamese.dyndns.org","threadId":"30806","inReplyTo":"20120615091425.20e40af9@chalon.bertin.fr","subject":"Re: [BUG] cherry-pick ignores some arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T15:06:50Z","receivedAt":"2012-06-15T15:06:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson writes:\n\n> Another orthogonal UI issue I see, is that rev-list could be more\n> user-friendly to warn the user when one element of a rev list is\n> ignored because of another one.\n\nSometimes my maint-1.7.8 branch may still have commits that are not\nin maint branch, sometimes maint-1.7.9 branch is a true subset of\nmaint branch. \"rev-list ^maint maint-1.7.8 maint-1.7.9\" is a very\nsensible thing to ask to find out what is still not in 'maint' out\nof stuff that are _usually_ but not always part of 'maint'.\n\nComplaining on such a query is not user-friendly at all.\n"},{"id":"193709","messageId":"7vk3z8bhn2.fsf@alter.siamese.dyndns.org","threadId":"30806","inReplyTo":"1339770796-542-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH 1/2] Documentation: --no-walk is no-op if range is specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T17:37:53Z","receivedAt":"2012-06-15T17:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> The existing description can be misleading and cause the reader to\n> think that --no-walk will do something if they specify a range in the\n> command line instead of a set of revs.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n> Documentation/rev-list-options.txt | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 1ae3c89..84e34b1 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -622,6 +622,7 @@ These options are mostly targeted for packing of git repositories.\n> --no-walk::\n> \n> \tOnly show the given revs, but do not traverse their ancestors.\n> +\tThis has no effect if a range is specified.\n> \n> --do-walk::\n\nThis is correct as a description of the current behaviour, but I\nhave to wonder if we should error out when the user explicitly\n(i.e. the implicit uses of --no-walk by \"show\" and \"cherry-pick\"\nneed to be treated differently) gives --no-walk and a negative\ncommit (either by A..B range, or a separate ^A).\n\nWould that break a valid script, and if not, how involved would such\na fix be?\n"},{"id":"193711","messageId":"7vehpgbgyb.fsf@alter.siamese.dyndns.org","threadId":"30806","inReplyTo":"1339770796-542-2-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/2] git-cherry-pick.txt: make clearer when revision walking gets activated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T17:52:44Z","receivedAt":"2012-06-15T17:52:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> When given a set of commits, cherry-pick will apply the changes for\n> all of them. Specifying a simple range will also work as\n> expected. This can cause the user to think that\n>\n> git cherry-pick A B..C\n>\n> will apply A and then B..C. This is not what happens. Instead the revs\n> are given to rev-list which will consider A and C as positive revs and\n> B as a negative one. Add a note about this and add an example with\n> this particular syntax, which has shown up on the list a few times.\n>\n> Signed-off-by: Carlos Martín Nieto \n> ---\n> Documentation/git-cherry-pick.txt | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\n> index 06a0bfd..10abfbf 100644\n> --- a/Documentation/git-cherry-pick.txt\n> +++ b/Documentation/git-cherry-pick.txt\n> @@ -48,6 +48,7 @@ OPTIONS\n> \tSets of commits can be passed but no traversal is done by\n> \tdefault, as if the '--no-walk' option was specified, see\n> \tlinkgit:git-rev-list[1].\n> +\tNote that specifying a range will activate revision walking.\n\nThat is not wrong per-se, but I do not think it would have helped\nYann. How about phrasing it this way?\n\n\tNote that specifying a range will feed all\n\t... arguments to a single revision walk (see a later\n\texample that uses 'maint master..next').\n\n> @@ -130,6 +131,15 @@ EXAMPLES\n> \tApply the changes introduced by all commits that are ancestors\n> \tof master but not of HEAD to produce new commits.\n> \n> +`git cherry-pick master next ^maint`::\n> +`git cherry-pick master maint..next`::\n> +\n> +\tApply the changes introduced by all commits that are ancestors\n> +\tof master or next, but not maint or any of its ancestors. The\n> +\tsecond spelling is often a misunderstanding of revision\n> +\twalking works when trying to apply a range plus a particular\n> +\tcommit and included for completeness.\n\nIf you are using these three branches because you expect familiarity\nwith the convention of maint < master < next on the reader's side, I\nthink it should be rewritten like this.\n\n`git cherry-pick maint next ^master`::\n`git cherry-pick maint master..next`::\n\n\tApply the changes introduced by all commits that are\n\tancestors of maint or next, but not master or any of its\n\tancestors. Note that the latter does not mean `maint` and\n\teverything between `master` and `next`; specifically,\n\t`maint` will not be used if it is included in `master`.\n"}]}