{"thread":{"id":"29345","subject":"[BUG] multi-commit cherry-pick messes up the order of commits","startedAt":"2012-01-11T17:31:01Z","lastAt":"2012-01-12T20:17:10Z","messageCount":24,"participants":["SZEDER Gábor","Christian Couder","Ramkumar Ramachandra","Jeff King","Jonathan Nieder","Junio C Hamano","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182364","messageId":"20120111173101.GQ30469@goldbirke","threadId":"29345","inReplyTo":null,"subject":"[BUG] multi-commit cherry-pick messes up the order of commits","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-01-11T17:31:01Z","receivedAt":"2012-01-11T17:31:01Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nI did some multi-commit cherry-picks lately, and noticed that\nsometimes cherry-pick applied the commits in different order than I\nspecified on the command line.  After some debugging, today I could\nfinally come up with a receipe to reproduce:\n\n  git init\n  echo 1 >a && git add a\n  git commit -m a\n  echo 2 >b && git add b\n  git commit -m b\n  sleep 2 && echo 3 >c && git add c\n  git commit -m c\n  git checkout -b branch HEAD^^\n  git cherry-pick master master^    # the later commit first\n\nwhere the 'git cherry-pick' command produces the following output:\n\n[branch ef5b86e0] b\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 b\n[branch 6a74f934] c\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 c\n\nNotice that master^, i.e. the commit adding the file 'b', is picked\nbefore master, i.e. the commit adding 'c', although the order on the\ncommand line was the reverse.\n\nThis is because\n\n  cmd_cherry_pick()\n    pick_revisions()\n      walk_revs_populate_todo()\n        prepare_revs()\n\ncalls prepare_revision_walk(), which parses the commits from the\ncommand line in the order they were specified, but inserts them into a\nlist ordered by date, and commits will be picked in the order they\nappear in this list.  So if you specify commits in a different order\nthan their committer date or commits with the same commiter date\n(which are often produced by am, rebase, and multi-commit\ncherry-pick), then they will be picked in wrong order.\n\nAs far as I can tell, this buggy behavior is as old as multi-commit\ncherry-pick itself, i.e. 7e2bfd3f (revert: allow cherry-picking more\nthan one commit, 2010-06-02).\n\n\nBest,\nGábor\n"},{"id":"182429","messageId":"CAP8UFD2uLoqzXRxssjwwW1Vk8RuNF_5OT1d7Z7hiRQ+Rq=UM1A@mail.gmail.com","threadId":"29345","inReplyTo":"20120111173101.GQ30469@goldbirke","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-01-12T13:31:30Z","receivedAt":"2012-01-12T13:31:30Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi all,\n\n2012/1/11 SZEDER Gábor <szeder@ira.uka.de>:\n>\n> As far as I can tell, this buggy behavior is as old as multi-commit\n> cherry-pick itself, i.e. 7e2bfd3f (revert: allow cherry-picking more\n> than one commit, 2010-06-02).\n\nThanks for the very detailed report!\n\nI didn't test nor even compiled anything but maybe this can be fixed\nby adding something like:\n\nopts->revs->topo_order = 1;\n\nin parse_args() or in prepare_revs()\n\nI will try to have a look tonight.\n\nThanks again,\nChristian.\n"},{"id":"182433","messageId":"20120112144409.GV30469@goldbirke","threadId":"29345","inReplyTo":"CAP8UFD2uLoqzXRxssjwwW1Vk8RuNF_5OT1d7Z7hiRQ+Rq=UM1A@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2012-01-12T14:44:09Z","receivedAt":"2012-01-12T14:44:09Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\n\nOn Thu, Jan 12, 2012 at 02:31:30PM +0100, Christian Couder wrote:\n> Hi all,\n> \n> 2012/1/11 SZEDER Gábor <szeder@ira.uka.de>:\n> >\n> > As far as I can tell, this buggy behavior is as old as multi-commit\n> > cherry-pick itself, i.e. 7e2bfd3f (revert: allow cherry-picking more\n> > than one commit, 2010-06-02).\n> \n> Thanks for the very detailed report!\n> \n> I didn't test nor even compiled anything but maybe this can be fixed\n> by adding something like:\n> \n> opts->revs->topo_order = 1;\n> \n> in parse_args() or in prepare_revs()\n> \n> I will try to have a look tonight.\n\n[Beware, I'm mostly clueless about git internals.]\n\nI don't think that any commit reordering, whether it's based on\ncommitter date, topology, or whatever, is acceptable.  Commits must be\npicked in the exact order they are specified on the command line.\n\nBesides, AFAICT, parse_args() sets opts->revs->no_walk = 1, which will\ncause prepare_revision_walk() to return before it would reach the\ntopo_order condition, so opts->revs->topo_order = 1 wouldn't have any\neffect.\n\n\nBest,\nGábor\n"},{"id":"182437","messageId":"CALkWK0kTog-rPLPj3fcFDEzDSVt1USZqnVdakW=+DK3C6gADoA@mail.gmail.com","threadId":"29345","inReplyTo":"20120112144409.GV30469@goldbirke","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T16:35:21Z","receivedAt":"2012-01-12T16:35:21Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Gábor,\n\nSZEDER Gábor wrote:\n> I don't think that any commit reordering, whether it's based on\n> committer date, topology, or whatever, is acceptable.  Commits must be\n> picked in the exact order they are specified on the command line.\n\nThanks for the excellent report.  I'm trying to figure out how to get\nthe revision API to do no ordering.\n\n-- Ram\n"},{"id":"182439","messageId":"20120112165329.GA17173@sigill.intra.peff.net","threadId":"29345","inReplyTo":"20120112144409.GV30469@goldbirke","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T16:53:29Z","receivedAt":"2012-01-12T16:53:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2012 at 03:44:09PM +0100, SZEDER Gábor wrote:\n\n> > Thanks for the very detailed report!\n> > \n> > I didn't test nor even compiled anything but maybe this can be fixed\n> > by adding something like:\n> > \n> > opts->revs->topo_order = 1;\n> > \n> > in parse_args() or in prepare_revs()\n> > \n> > I will try to have a look tonight.\n> \n> [Beware, I'm mostly clueless about git internals.]\n> \n> I don't think that any commit reordering, whether it's based on\n> committer date, topology, or whatever, is acceptable.  Commits must be\n> picked in the exact order they are specified on the command line.\n\nI thought the multi-commit cherry-pick was supposed to take arbitrary\nrevision arguments, so you can do:\n\n  git cherry-pick master..topic\n\nand likewise you can spell it:\n\n  git cherry-pick topic ^master\n\nor:\n\n  git cherry-pick ^master topic\n\nSo the order of arguments isn't relevant in those cases; the graph\nordering is. I agree it would be nice to make:\n\n  git cherry-pick commit1 commit3 commit2\n\nwork in the order specified, but how does that interact with existing\ncases that provide more traditional revision arguments?\n\n-Peff\n"},{"id":"182441","messageId":"CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com","threadId":"29345","inReplyTo":"20120112165329.GA17173@sigill.intra.peff.net","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T17:09:48Z","receivedAt":"2012-01-12T17:09:48Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Peff,\n\nJeff King wrote:\n>                  I agree it would be nice to make:\n>  git cherry-pick commit1 commit3 commit2\n>\n> work in the order specified, but how does that interact with existing\n> cases that provide more traditional revision arguments?\n\nWhat are your thoughts on making it a flag in the revision API to be\nactivated with \"cherry-pick --literal-order commit1 commit3 commit2\"\nor similar?  I'm not sure how to get it to reconcile with the more\ntraditional revision arguments yet. My current worktree (WIP):\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0d8020c..47da41b 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -228,6 +228,7 @@ static void parse_args(int argc, const char\n**argv, struct re\n                opts->revs = xmalloc(sizeof(*opts->revs));\n                init_revisions(opts->revs, NULL);\n                opts->revs->no_walk = 1;\n+               opts->revs->literal_order = 1;\n                if (argc < 2)\n                        usage_with_options(usage_str, options);\n                argc = setup_revisions(argc, argv, opts->revs, NULL);\ndiff --git a/revision.c b/revision.c\nindex 064e351..301ef58 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2054,7 +2054,10 @@ int prepare_revision_walk(struct rev_info *revs)\n                if (commit) {\n                        if (!(commit->object.flags & SEEN)) {\n                                commit->object.flags |= SEEN;\n-                               commit_list_insert_by_date(commit,\n&revs->commits\n+                               if (revs->literal_order)\n+                                       commit_list_insert(commit,\n&revs->commits\n+                               else\n+\ncommit_list_insert_by_date(commit, &revs-\n                        }\n                }\n                e++;\ndiff --git a/revision.h b/revision.h\nindex b8e9223..65c3dc3 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -67,6 +67,7 @@ struct rev_info {\n                        remove_empty_trees:1,\n                        simplify_history:1,\n                        lifo:1,\n+                       literal_order:1,\n                        topo_order:1,\n                        simplify_merges:1,\n                        simplify_by_decoration:1,\n"},{"id":"182443","messageId":"1326388458-5785-1-git-send-email-artagnon@gmail.com","threadId":"29345","inReplyTo":"CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T17:14:18Z","receivedAt":"2012-01-12T17:14:18Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> My current worktree (WIP):\n> [...]\n\nClassic whitespace breakage.  How many times am I going to fall for\nthe same joke?\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0d8020c..47da41b 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -228,6 +228,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\topts->revs = xmalloc(sizeof(*opts->revs));\n \t\tinit_revisions(opts->revs, NULL);\n \t\topts->revs->no_walk = 1;\n+\t\topts->revs->literal_order = 1;\n \t\tif (argc < 2)\n \t\t\tusage_with_options(usage_str, options);\n \t\targc = setup_revisions(argc, argv, opts->revs, NULL);\ndiff --git a/revision.c b/revision.c\nindex 064e351..301ef58 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2054,7 +2054,10 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n \t\t\t\tcommit->object.flags |= SEEN;\n-\t\t\t\tcommit_list_insert_by_date(commit, &revs->commits);\n+\t\t\t\tif (revs->literal_order)\n+\t\t\t\t\tcommit_list_insert(commit, &revs->commits);\n+\t\t\t\telse\n+\t\t\t\t\tcommit_list_insert_by_date(commit, &revs->commits);\n \t\t\t}\n \t\t}\n \t\te++;\ndiff --git a/revision.h b/revision.h\nindex b8e9223..65c3dc3 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -67,6 +67,7 @@ struct rev_info {\n \t\t\tremove_empty_trees:1,\n \t\t\tsimplify_history:1,\n \t\t\tlifo:1,\n+\t\t\tliteral_order:1,\n \t\t\ttopo_order:1,\n \t\t\tsimplify_merges:1,\n \t\t\tsimplify_by_decoration:1,\n"},{"id":"182442","messageId":"20120112171536.GA18102@sigill.intra.peff.net","threadId":"29345","inReplyTo":"CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T17:15:36Z","receivedAt":"2012-01-12T17:15:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2012 at 10:39:48PM +0530, Ramkumar Ramachandra wrote:\n\n> Jeff King wrote:\n> >                  I agree it would be nice to make:\n> >  git cherry-pick commit1 commit3 commit2\n> >\n> > work in the order specified, but how does that interact with existing\n> > cases that provide more traditional revision arguments?\n> \n> What are your thoughts on making it a flag in the revision API to be\n> activated with \"cherry-pick --literal-order commit1 commit3 commit2\"\n> or similar?  I'm not sure how to get it to reconcile with the more\n> traditional revision arguments yet. My current worktree (WIP):\n\nI think that is a sensible first-cut. It may even be possible to use\nheuristics to identify when --literal-order is needed, and eventually it\ncould go away. But that is a much riskier feature that can be built on\ntop of the much safer proposal you are making.\n\n> diff --git a/revision.c b/revision.c\n> index 064e351..301ef58 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2054,7 +2054,10 @@ int prepare_revision_walk(struct rev_info *revs)\n>                 if (commit) {\n>                         if (!(commit->object.flags & SEEN)) {\n>                                 commit->object.flags |= SEEN;\n> -                               commit_list_insert_by_date(commit,\n> &revs->commits\n> +                               if (revs->literal_order)\n> +                                       commit_list_insert(commit,\n> &revs->commits\n> +                               else\n> +\n> commit_list_insert_by_date(commit, &revs-\n\nMy only concern is that there are other parts of the revision machinery\nthat depend on the date-ordering of the commit list. What would happen,\nfor example, with:\n\n  git rev-list --literal-order --do-walk foo\n\nIt probably doesn't make sense to allow literal-order without no-walk,\nanyway (which of course is the default in cherry-pick anyway, so it's\nnot a big deal here).\n\nI'm also not sure what:\n\n  git rev-list --literal-order foo..bar\n\nwould or should do.\n\n-Peff\n"},{"id":"182444","messageId":"CALkWK0nJM2wUE9qzp38qjFFqCdwX9w0Jckxi1G=1=7adMxg2rw@mail.gmail.com","threadId":"29345","inReplyTo":"20120112171536.GA18102@sigill.intra.peff.net","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T17:26:23Z","receivedAt":"2012-01-12T17:26:23Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Peff,\n\nJeff King wrote:\n> It may even be possible to use\n> heuristics to identify when --literal-order is needed, and eventually it\n> could go away.\n\nYeah, that would be really nice.\n\n> My only concern is that there are other parts of the revision machinery\n> that depend on the date-ordering of the commit list.\n\nAgreed.  Looking at it another way, it's an opportunity to read\nrevision.c, learn, and modernize some older parts :)\n\n> What would happen,\n> for example, with:\n>\n>  git rev-list --literal-order --do-walk foo\n>\n> It probably doesn't make sense to allow literal-order without no-walk,\n> anyway (which of course is the default in cherry-pick anyway, so it's\n> not a big deal here).\n\nI don't know if that particular case is a problem: there are some\nmutually exclusive options in revision.c already like:\n\n  cannot combine --reverse with --graph\n\nThis'll just be another one of them.\n\n> I'm also not sure what:\n>\n>  git rev-list --literal-order foo..bar\n>\n> would or should do.\n\nInstead of classifying it as an \"ordering\" option (as defined in\nDocumentation/rev-list-options.txt), I think we should give it some\nsort of special status for now -- it can be combined with ordering\noptions (of which date ordering is default anyway).  For this specific\nquestion, I suspect that revision.c does a topo-ordering for commit\nranges (I haven't read the code), so we have to make sure that\nwhatever extra logic we add doesn't disrupt the existing logic in\nrevision.c.\n\n-- Ram\n"},{"id":"182447","messageId":"20120112174731.GA6038@burratino","threadId":"29345","inReplyTo":"20120112165329.GA17173@sigill.intra.peff.net","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T17:47:32Z","receivedAt":"2012-01-12T17:47:32Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I agree it would be nice to make:\n>\n>   git cherry-pick commit1 commit3 commit2\n>\n> work in the order specified, but how does that interact with existing\n> cases that provide more traditional revision arguments?\n\nYes, exactly.  Another question: what should\n\n\tgit cherry-pick master..next maint..master\n\ndo?\n"},{"id":"182448","messageId":"1326390647-21446-1-git-send-email-artagnon@gmail.com","threadId":"29345","inReplyTo":"CALkWK0nJM2wUE9qzp38qjFFqCdwX9w0Jckxi1G=1=7adMxg2rw@mail.gmail.com","subject":"[PATCH] cherry-pick: add failing test for out-of-order pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T17:50:47Z","receivedAt":"2012-01-12T17:50:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Due to the way traditional revision arguments work, the following\ninvocations of 'git cherry-pick' are equivalent:\n\n  $ git cherry-pick master..topic\n  $ git cherry-pick topic ^master\n  $ git cherry-pick ^master topic\n\nSo the order of the arguments specified on the command-line is\nirrelevant in these cases.  However, there are cases where it is worth\npaying attention to the order.  For instance:\n\n  $ git cherry-pick commit3 commit1 commit2\n\npicks commits after sorting by date order, which is counter-intuitive.\nAdd a failing test to t3508 (cherry-pick-many-commits) documenting\nthis behavior.\n\nReported-by: SZEDER Gábor <szeder@ira.uka.de>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n Irrespective of how far we get with the '--literal-order' idea, I\n think this quirk is worth documenting.\n\n t/t3508-cherry-pick-many-commits.sh |   25 +++++++++++++++++++++++++\n 1 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 8e09fd0..dd65835 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -59,6 +59,31 @@ test_expect_success 'cherry-pick first..fourth works' '\n \tcheck_head_differs_from fourth\n '\n \n+test_expect_failure 'cherry-pick picks commits in the right order' '\n+\tcat <<-\\EOF >expected &&\n+\t[master OBJID] fourth\n+\t Author: A U Thor <author@example.com>\n+\t 1 files changed, 1 insertions(+), 0 deletions(-)\n+\t[master OBJID] second\n+\t Author: A U Thor <author@example.com>\n+\t 1 files changed, 1 insertions(+), 0 deletions(-)\n+\t[master OBJID] third\n+\t Author: A U Thor <author@example.com>\n+\t 1 files changed, 1 insertions(+), 0 deletions(-)\n+\tEOF\n+\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_tick &&\n+\tgit cherry-pick fourth second third >actual &&\n+\tgit diff --quiet other &&\n+\tgit diff --quiet HEAD other &&\n+\n+\tsed -e \"s/$_x05[0-9a-f][0-9a-f]/OBJID/\" <actual >actual.fuzzy &&\n+\ttest_cmp expected actual.fuzzy &&\n+\tcheck_head_differs_from second\n+'\n+\n test_expect_success 'cherry-pick --strategy resolve first..fourth works' '\n \tcat <<-\\EOF >expected &&\n \tTrying simple merge.\n-- \n1.7.8.2\n"},{"id":"182450","messageId":"7vaa5s3hiq.fsf@alter.siamese.dyndns.org","threadId":"29345","inReplyTo":"CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T18:25:17Z","receivedAt":"2012-01-12T18:25:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> What are your thoughts on making it a flag in the revision API to be\n> activated with \"cherry-pick --literal-order commit1 commit3 commit2\"\n> or similar?\n\nThat is an insane UI for the sake of flexibility.\n\nYou should be able to look at revs->cmdline and tell if you need to let\ncherry-pick walk (i.e. \"cherry-pick master..next\"), or if the user wants\nindividual commits (i.e. \"cherry-pick A B C\").\n\nAnd you do prepare_revision_walk() only when you need to walk; otherwise\nyou use the contents of revs->pending in order.\n"},{"id":"182452","messageId":"20120112183246.GB6038@burratino","threadId":"29345","inReplyTo":"1326390647-21446-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] cherry-pick: add failing test for out-of-order pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T18:32:46Z","receivedAt":"2012-01-12T18:32:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n>   $ git cherry-pick master..topic\n>   $ git cherry-pick topic ^master\n>   $ git cherry-pick ^master topic\n>\n> So the order of the arguments specified on the command-line is\n> irrelevant in these cases.  However, there are cases where it is worth\n> paying attention to the order.  For instance:\n>\n\nThis segue feels a bit unnatural.  I think the relevant point was that\nearly output from revision traversal (and perhaps some other things\n--- I haven't checked) relies on commits having been inserted in a\ntopologically sorted order.\n\nAnyway, I don't think the background is necessary --- the\none-paragraph description below stands well enough alone.\n\n>   $ git cherry-pick commit3 commit1 commit2\n> \n> picks commits after sorting by date order, which is counter-intuitive.\n> Add a failing test to t3508 (cherry-pick-many-commits) documenting\n> this behavior.\n> \n> Reported-by: SZEDER Gábor <szeder@ira.uka.de>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n[...]\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -59,6 +59,31 @@ test_expect_success 'cherry-pick first..fourth works' '\n>  \tcheck_head_differs_from fourth\n>  '\n>  \n> +test_expect_failure 'cherry-pick picks commits in the right order' '\n\nI would say \"in the order requested\" instead of the right order, since\nit is not completely obvious to me what the right order is.\n\n> +\tcat <<-\\EOF >expected &&\n> +\t[master OBJID] fourth\n> +\t Author: A U Thor <author@example.com>\n> +\t 1 files changed, 1 insertions(+), 0 deletions(-)\n> +\t[master OBJID] second\n> +\t Author: A U Thor <author@example.com>\n> +\t 1 files changed, 1 insertions(+), 0 deletions(-)\n> +\t[master OBJID] third\n> +\t Author: A U Thor <author@example.com>\n> +\t 1 files changed, 1 insertions(+), 0 deletions(-)\n> +\tEOF\n\nWhy check all these details of formatting, instead of e.g. using \"git\nrev-list | git diff-tree -s --format=%s\"?\n\n[...]\n> +\ttest_cmp expected actual.fuzzy &&\n> +\tcheck_head_differs_from second\n\nWhy make the same check twice?\n\nHope that helps,\nJonathan\n"},{"id":"182453","messageId":"7v62gg3grq.fsf@alter.siamese.dyndns.org","threadId":"29345","inReplyTo":"20120112174731.GA6038@burratino","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T18:41:29Z","receivedAt":"2012-01-12T18:41:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Yes, exactly.  Another question: what should\n>\n> \tgit cherry-pick master..next maint..master\n>\n> do?\n\nRevision ranges are not defined as a union of sets, but a single set as a\nrange between zero or more bottom (UNINTERESTING) commits and zero or more\ntop commits, the ones reachable from the top but not from the bottom, so\nthe above will work as if you said \"^master ^maint master next\", which is\nthe same as \"master..next\" (if you assume all of \"maint\" is contained in\n\"master\" all of which is contained in \"next\", of course).\n\nAnd it is not likely to change soon.\n\nIn the longer term (or in an alternate universe where we were inventing\nGit from scratch today without any existing users), we may want to revamp\nthe revision machinery, taking advantage of the recent addition of the\n\"cmdline\" facility to it, so that we would walk ranges \"master..next\" and\n\"maint..master\" independently, clearing the object flags as needed between\nthe separate traversals as needed, and then take a union of these ranges,\nbefore returning results from get_revision() calls.\n"},{"id":"182455","messageId":"1326395132-18947-1-git-send-email-artagnon@gmail.com","threadId":"29345","inReplyTo":"20120112183246.GB6038@burratino","subject":"[PATCH v2] cherry-pick: add failing test for out-of-order pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T19:05:31Z","receivedAt":"2012-01-12T19:05:31Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The invocation\n\n  $ git cherry-pick commit3 commit1 commit2\n\npicks commits after sorting by date order, which is counter-intuitive.\nAdd a failing test to t3508 (cherry-pick-many-commits) documenting\nthis behavior.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n Had some weird compulsion to conform to the style of the other tests\n in the previous iteration.\n\n t/t3508-cherry-pick-many-commits.sh |   17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex 8e09fd0..d9d632d 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -59,6 +59,23 @@ test_expect_success 'cherry-pick first..fourth works' '\n \tcheck_head_differs_from fourth\n '\n \n+test_expect_failure 'cherry-pick picks commits in the order requested' '\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_tick &&\n+\tgit cherry-pick fourth second third &&\n+\t{\n+\t\tgit rev-list --reverse HEAD |\n+\t\tgit diff-tree --stdin -s --format=%s\n+\t} >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfourth\n+\tsecond\n+\tthird\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick --strategy resolve first..fourth works' '\n \tcat <<-\\EOF >expected &&\n \tTrying simple merge.\n-- \n1.7.8.2\n"},{"id":"182457","messageId":"4F0F32CC.8040404@kdbg.org","threadId":"29345","inReplyTo":"CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-01-12T19:21:48Z","receivedAt":"2012-01-12T19:21:48Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.01.2012 18:09, schrieb Ramkumar Ramachandra:\n> @@ -2054,7 +2054,10 @@ int prepare_revision_walk(struct rev_info *revs)\n>                 if (commit) {\n>                         if (!(commit->object.flags & SEEN)) {\n>                                 commit->object.flags |= SEEN;\n> -                               commit_list_insert_by_date(commit,\n> &revs->commits\n> +                               if (revs->literal_order)\n> +                                       commit_list_insert(commit,\n> &revs->commits\n> +                               else\n> +\n> commit_list_insert_by_date(commit, &revs-\n\nWhy do we need a new flag?\n\n  git show origin/master origin/maint\n  git show origin/maint origin/master\n\nshow the revisions in different order, in particular, in the order\nrequested on the command line. Shoudn't cherry-pick be able to do the\nsame without new hacks?\n\n-- Hannes\n"},{"id":"182459","messageId":"CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com","threadId":"29345","inReplyTo":"7vaa5s3hiq.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T19:25:58Z","receivedAt":"2012-01-12T19:25:58Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> What are your thoughts on making it a flag in the revision API to be\n>> activated with \"cherry-pick --literal-order commit1 commit3 commit2\"\n>> or similar?\n>\n> That is an insane UI for the sake of flexibility.\n>\n> You should be able to look at revs->cmdline and tell if you need to let\n> cherry-pick walk (i.e. \"cherry-pick master..next\"), or if the user wants\n> individual commits (i.e. \"cherry-pick A B C\").\n>\n> And you do prepare_revision_walk() only when you need to walk; otherwise\n> you use the contents of revs->pending in order.\n\nOkay, just to make sure I understand this correctly: if more than one\nargument is literally specified, I should not set up the revision\nwalker and pick the commits listed in revs->pending, correct?  Then,\nwhen I encounter the following command,\n\n  $ git cherry-pick maint ^master\n\nI should just pick two commits: maint, and ^master.  But won't this\nintroduce some kind of regression for those who expect me to pick the\nmaster..maint range instead?  Has this double-interpretation issue\ncome up in other commands?  Have we documented this somewhere?\n\nThanks.\n\n-- Ram\n"},{"id":"182460","messageId":"CALkWK0=NVUd629FgkPfgi8ZgTuO+a10t+iwbSrAvONCSmeq2rQ@mail.gmail.com","threadId":"29345","inReplyTo":"4F0F32CC.8040404@kdbg.org","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-01-12T19:29:53Z","receivedAt":"2012-01-12T19:29:53Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Johannes,\n\nJohannes Sixt wrote:\n> Why do we need a new flag?\n>\n>  git show origin/master origin/maint\n>  git show origin/maint origin/master\n>\n> show the revisions in different order, in particular, in the order\n> requested on the command line. Shoudn't cherry-pick be able to do the\n> same without new hacks?\n\nThat was my first reaction too -- then I saw builtin/push.c (the\nbuiltin show is quite similar), and found out that it doesn't use the\nrevision walker at all.  It operates on refs, which has different\nsemantics altogether (called \"refspec\" in some places I think).\n\n-- Ram\n"},{"id":"182461","messageId":"20120112193312.GC6038@burratino","threadId":"29345","inReplyTo":"1326395132-18947-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2] cherry-pick: add failing test for out-of-order pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T19:33:12Z","receivedAt":"2012-01-12T19:33:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n>  Had some weird compulsion to conform to the style of the other tests\n>  in the previous iteration.\n\nThe tests you're talking about were introduced in commit 7b53b92f to\ncheck for a buglet that made --strategy suppress the progress\nreporting (\"Finished one cherry-pick.\") output cherry-pick normally\nwould emit.  \n\nSo no inconsistency here --- those tests are _intending_ to check the\noutput format and that cherry-pick, unlike cherry-pick --ff, produces\nnew commits (though it would probably be clearer to put checks for\nthese behaviors in separate test assertions), while the new failing\ntest you are introducing is not about those things.\n\nStriving for a consistent style is certainly not weird.\n\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -59,6 +59,23 @@ test_expect_success 'cherry-pick first..fourth works' '\n[...]\n> +\tgit cherry-pick fourth second third &&\n> +\t{\n> +\t\tgit rev-list --reverse HEAD |\n> +\t\tgit diff-tree --stdin -s --format=%s\n> +\t} >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfourth\n> +\tsecond\n> +\tthird\n> +\tEOF\n> +\ttest_cmp expect actual\n\nThis still feels more convoluted than expected (e.g., why --reverse?).\nSomething like\n\n\tprintf \"%s\\n\" third second fourth >expect &&\n\t...\n\tgit log --format=%s >actual &&\n\ttest_cmp expect actual\n\nshould be plenty.\n"},{"id":"182462","messageId":"20120112193426.GD6038@burratino","threadId":"29345","inReplyTo":"CALkWK0=NVUd629FgkPfgi8ZgTuO+a10t+iwbSrAvONCSmeq2rQ@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T19:34:26Z","receivedAt":"2012-01-12T19:34:26Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> That was my first reaction too -- then I saw builtin/push.c (the\n> builtin show is quite similar), and found out that it doesn't use the\n> revision walker at all.\n\n\"git push\", unlike \"git show\", does not accept arguments like\nmaint..master.\n"},{"id":"182463","messageId":"20120112194710.GA28148@sigill.intra.peff.net","threadId":"29345","inReplyTo":"CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T19:47:10Z","receivedAt":"2012-01-12T19:47:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 13, 2012 at 12:55:58AM +0530, Ramkumar Ramachandra wrote:\n\n> Junio C Hamano wrote:\n> > You should be able to look at revs->cmdline and tell if you need to let\n> > cherry-pick walk (i.e. \"cherry-pick master..next\"), or if the user wants\n> > individual commits (i.e. \"cherry-pick A B C\").\n> >\n> > And you do prepare_revision_walk() only when you need to walk; otherwise\n> > you use the contents of revs->pending in order.\n\nI am tempted to suggest that cherry-pick should not feed its arguments\nto the revision machinery in the first place, but instead accept a set\nof arguments, each argument of which is either a single commit\n(interpreted by get_sha1) or a range specifier (which can be fed to\nsetup-revisions). And then get a linearized set of commits for _each_\nargument independently and concatenate them (possibly eliminating\nduplicates). That would make all of these work as most people would\nexpect:\n\n  git cherry-pick A B C\n  git cherry-pick A..B\n  git cherry-pick A..B B..C\n\nbut would be a regression for:\n\n  git cherry-pick B ^A\n\nversus the current code. I suspect that the latter form is not all that\ncommonly used, though, and certainly I would accept it as a casualty of\nmaking the \"A B C\" form work. My only hesitation is that it is in fact a\nregression.\n\n> Okay, just to make sure I understand this correctly: if more than one\n> argument is literally specified, I should not set up the revision\n> walker and pick the commits listed in revs->pending, correct?  Then,\n> when I encounter the following command,\n> \n>   $ git cherry-pick maint ^master\n> \n> I should just pick two commits: maint, and ^master.\n\nBut ^master is not a commit, it is a negation. So it is nonsensical if\nthe arguments are considered independent of each other (you _could_\nuse a heuristic to guess that they are not independent, but I'd rather\nnot go there). So you'd probably end up just rejecting the arguments.\n\n-Peff\n"},{"id":"182464","messageId":"20120112201122.GE6038@burratino","threadId":"29345","inReplyTo":"20120112194710.GA28148@sigill.intra.peff.net","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-12T20:11:22Z","receivedAt":"2012-01-12T20:11:22Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I am tempted to suggest\n[...]\n>              That would make all of these work as most people would\n> expect:\n>\n>   git cherry-pick A B C\n>   git cherry-pick A..B\n>   git cherry-pick A..B B..C\n>\n> but would be a regression for:\n>\n>   git cherry-pick B ^A\n>\n> versus the current code. I suspect that the latter form is not all that\n> commonly used, though, and certainly I would accept it as a casualty of\n> making the \"A B C\" form work. My only hesitation is that it is in fact a\n> regression.\n\nI find myself using such complicated expressions as\n\n\tlist-revs-to-skip |\n\txargs git cherry-pick --cherry-pick --right-only HEAD...topic --not\n\nso yeah, that would be a pretty serious loss in functionality.\n\nHowever, moving to something like the far future semantics that Junio\nhinted at, for cherry-pick/revert and other --no-walk style commands\nonly, would not be a regression for me.  The multi-pick feature is\nstill young, and I _suspect_ changing the meaning of A..B B..C for it\nwould not inconvenience anybody.\n\nI would even welcome a change in the meaning of B ^A: the most\nintuitive thing for it to do would be to cherry-pick the single commit B\nwhen and only when it is not an ancestor of A.\n\nJonathan\n"},{"id":"182465","messageId":"7vpqeo1y0i.fsf@alter.siamese.dyndns.org","threadId":"29345","inReplyTo":"CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T20:11:57Z","receivedAt":"2012-01-12T20:11:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Okay, just to make sure I understand this correctly: if more than one\n> argument is literally specified, I should not set up the revision\n> walker and pick the commits listed in revs->pending, correct?\n\nNot really.\n\nA rough approximation would be that if you see any negative ones (either\ncoming from A..B or ^A), you would always want to walk, giving everything\nto prepare_revision_walk()-and-then-get_revision() machiery.\n\nOtherwise you have only zero [*1*] or more positive ones, and you pick\nthem in the order you find in the pending list, without bothering the\nrevision traversal machinery at all [*2*].\n\n> when I encounter the following command,\n>\n>   $ git cherry-pick maint ^master\n>\n> I should just pick two commits: maint, and ^master.\n\nSo the answer is aboslutely no. \"maint ^master\" is the same as saying\n\"master..maint\".\n\n[Footnote]\n\n*1* You would probably want to error out if you got zero positive ones in\nthis case (i.e. absolutely nothing was given, neither positive nor\nnegative).\n\n*2* The reason this is \"rough\" approximation is because we would probably\nwant to do Jonathan's \"maint..master master..next\" someday, and when that\nhappens, we would need to do much more than \"do we have any negative? then\nsend it through to prepare_revision_walk()\". But we are not there yet, so\nI think the above is actually more or less the complete implementation.\n"},{"id":"182466","messageId":"20120112201710.GA31427@sigill.intra.peff.net","threadId":"29345","inReplyTo":"20120112201122.GE6038@burratino","subject":"Re: [BUG] multi-commit cherry-pick messes up the order of commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-12T20:17:10Z","receivedAt":"2012-01-12T20:17:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2012 at 02:11:22PM -0600, Jonathan Nieder wrote:\n\n> > I am tempted to suggest\n> [...]\n> >              That would make all of these work as most people would\n> > expect:\n> >\n> >   git cherry-pick A B C\n> >   git cherry-pick A..B\n> >   git cherry-pick A..B B..C\n> >\n> > but would be a regression for:\n> >\n> >   git cherry-pick B ^A\n> >\n> > versus the current code. I suspect that the latter form is not all that\n> > commonly used, though, and certainly I would accept it as a casualty of\n> > making the \"A B C\" form work. My only hesitation is that it is in fact a\n> > regression.\n> \n> I find myself using such complicated expressions as\n> \n> \tlist-revs-to-skip |\n> \txargs git cherry-pick --cherry-pick --right-only HEAD...topic --not\n> \n> so yeah, that would be a pretty serious loss in functionality.\n\nThat's gross. :)\n\nBut thank you for providing a real-world example. I had a vague notion\nthat the full power of the revision parser was not actually useful to\npeople, but clearly not.\n\nOTOH, if cherry-pick were more simplistic, you could perhaps get by\nwith:\n\n  list-revs-to-skip |\n  xargs git rev-list --cherry-pick --right-only HEAD...topic --not |\n  git cherry-pick --stdin\n\n-Peff\n"}]}