{"thread":{"id":"48750","subject":"[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","startedAt":"2018-05-25T12:42:03Z","lastAt":"2019-03-27T13:54:09Z","messageCount":22,"participants":["Sergey Organov","Junio C Hamano","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"350624","messageId":"87efh0pdln.fsf@javad.com","threadId":"48750","inReplyTo":null,"subject":"[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-05-25T12:42:03Z","receivedAt":"2018-05-25T12:42:03Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"When cherry-picking multiple commits, it's impossible to have both\nmerge- and non-merge commits on the same command-line. Not specifying\n'-m 1' results in cherry-pick refusing to handle merge commits, while\nspecifying '-m 1' fails on non-merge commits.\n\nThis patch allows '-m 1' for non-merge commits. Besides, as mainline is\nalways the only parent for a non-merge commit, it made little sense to\ndisable it in the first place.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n sequencer.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1ce6326..2393bdf 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (0 < opts->mainline)\n-\t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n-\t\t\toid_to_hex(&commit->object.oid));\n+\t} else if (1 < opts->mainline)\n+\t\t/* Non-first parent explicitly specified as mainline for\n+\t\t * non-merge commit */\n+\t\treturn error(_(\"commit %s does not have parent %d\"),\n+\t\t\t     oid_to_hex(&commit->object.oid), opts->mainline);\n \telse\n \t\tparent = commit->parents->item;\n \n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"350651","messageId":"xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com","threadId":"48750","inReplyTo":"87efh0pdln.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-21T15:54:17Z","receivedAt":"2018-06-21T15:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> When cherry-picking multiple commits, it's impossible to have both\n> merge- and non-merge commits on the same command-line. Not specifying\n> '-m 1' results in cherry-pick refusing to handle merge commits, while\n> specifying '-m 1' fails on non-merge commits.\n\nAllowing \"-m1\" even when picking a single parent commit, because\nthe 1-st parent is defined for such a commit, makes sense, espeially\nwhen running a cherry-pick on a range, exactly for the above reason.\nIt is slightly less so when cherry-picking a single commit, but not\nby a large margin.\n\nI think the original reasoning for requiring \"-m $n\" not present,\nespecially because cherry-pick was originally for replaying only a\nsingle commit, was because it would lead somebody to propose that\nthe command should behave as if -m1 is always given (and when trying\nto cherry-pick a merge relative to its second parent, give -m2 to\noverride it), which in turn encourage the 'first-parent is special'\nworld-view from the tool-side.  IOW, \"The worldview to treat the\nfirst-parent chain specially is correct, because Git has many\nfeatures to work with that worldview conveniently\" was something we\nwanted to avoid; rather \"Such and such workflows benefit from\ntreating the first-parent chain specially, so let's add features to\ndo so\" was we wanted to do, and of course, back then cherry-pick\nthat allows mixture of merges and single-parent commits to be\npicked, which would have made the need to do something like this\npatch does felt greater, did not exist.\n\nNow, it appears, at least to me, that the world pretty much accepted\nthat the first-parent worldview is often very convenient and worth\nsupporting by the tool, so the next logical step might be to set\nopts->mainline to 1 by default (and allow an explicit \"-m $n\" from\nthe command line to override it).  But that should happen after this\npatch lands---it is logically a separate step, I would think.\n\n> This patch allows '-m 1' for non-merge commits. Besides, as mainline is\n> always the only parent for a non-merge commit, it made little sense to\n> disable it in the first place.\n>\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  sequencer.c | 8 +++++---\n>  1 file changed, 5 insertions(+), 3 deletions(-)\n\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 1ce6326..2393bdf 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n>  \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n>  \t\tparent = p->item;\n> -\t} else if (0 < opts->mainline)\n> -\t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n> -\t\t\toid_to_hex(&commit->object.oid));\n> +\t} else if (1 < opts->mainline)\n> +\t\t/* Non-first parent explicitly specified as mainline for\n> +\t\t * non-merge commit */\n\nStyle.\n\n\t/*\n\t * Our multi-line comments are to be\n\t * formatted like this.\n\t */\n\n> +\t\treturn error(_(\"commit %s does not have parent %d\"),\n> +\t\t\t     oid_to_hex(&commit->object.oid), opts->mainline);\n>  \telse\n>  \t\tparent = commit->parents->item;\n"},{"id":"350716","messageId":"874lhvmbak.fsf@javad.com","threadId":"48750","inReplyTo":"xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-06-22T09:16:03Z","receivedAt":"2018-06-22T09:16:11Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> When cherry-picking multiple commits, it's impossible to have both\n>> merge- and non-merge commits on the same command-line. Not specifying\n>> '-m 1' results in cherry-pick refusing to handle merge commits, while\n>> specifying '-m 1' fails on non-merge commits.\n>\n> Allowing \"-m1\" even when picking a single parent commit, because\n> the 1-st parent is defined for such a commit, makes sense, espeially\n> when running a cherry-pick on a range, exactly for the above reason.\n> It is slightly less so when cherry-picking a single commit, but not\n> by a large margin.\n\n[...]\n\n>> +\t} else if (1 < opts->mainline)\n>> +\t\t/* Non-first parent explicitly specified as mainline for\n>> +\t\t * non-merge commit */\n>\n> Style.\n>\n> \t/*\n> \t * Our multi-line comments are to be\n> \t * formatted like this.\n> \t */\n\nAh, sorry for that. Will you fix it for me?\n\nThanks!\n\n-- Sergey\n"},{"id":"365151","messageId":"871s6n5mtd.fsf@javad.com","threadId":"48750","inReplyTo":"xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-12T05:35:58Z","receivedAt":"2018-12-12T05:36:03Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> When cherry-picking multiple commits, it's impossible to have both\n>> merge- and non-merge commits on the same command-line. Not specifying\n>> '-m 1' results in cherry-pick refusing to handle merge commits, while\n>> specifying '-m 1' fails on non-merge commits.\n>\n\n[...]\n\n> Now, it appears, at least to me, that the world pretty much accepted\n> that the first-parent worldview is often very convenient and worth\n> supporting by the tool, so the next logical step might be to set\n> opts->mainline to 1 by default (and allow an explicit \"-m $n\" from\n> the command line to override it).  But that should happen after this\n> patch lands---it is logically a separate step, I would think.\n>\n\n[...]\n\n>> +\t} else if (1 < opts->mainline)\n>> +\t\t/* Non-first parent explicitly specified as mainline for\n>> +\t\t * non-merge commit */\n>\n> Style.\n>\n> \t/*\n> \t * Our multi-line comments are to be\n> \t * formatted like this.\n> \t */\n\nComment style fixed:\n\n-- 8< --\n\nWhen cherry-picking multiple commits, it's impossible to have both\nmerge- and non-merge commits on the same command-line. Not specifying\n'-m 1' results in cherry-pick refusing to handle merge commits, while\nspecifying '-m 1' fails on non-merge commits.\n\nThis patch allows '-m 1' for non-merge commits. Besides, as mainline is\nalways the only parent for a non-merge commit, it made little sense to\ndisable it in the first place.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n sequencer.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e1a4dd1..d0fd61b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (0 < opts->mainline)\n-\t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n-\t\t\toid_to_hex(&commit->object.oid));\n+\t} else if (1 < opts->mainline)\n+\t\t/*\n+\t\t *  Non-first parent explicitly specified as mainline for\n+\t\t *  non-merge commit\n+\t\t */\n+\t\treturn error(_(\"commit %s does not have parent %d\"),\n+\t\t\t     oid_to_hex(&commit->object.oid), opts->mainline);\n \telse\n \t\tparent = commit->parents->item;\n \n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"365224","messageId":"xmqqo99qf46q.fsf@gitster-ct.c.googlers.com","threadId":"48750","inReplyTo":"871s6n5mtd.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T04:20:29Z","receivedAt":"2018-12-13T04:20:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> When cherry-picking multiple commits, it's impossible to have both\n> merge- and non-merge commits on the same command-line. Not specifying\n> '-m 1' results in cherry-pick refusing to handle merge commits, while\n> specifying '-m 1' fails on non-merge commits.\n>\n> This patch allows '-m 1' for non-merge commits. Besides, as mainline is\n> always the only parent for a non-merge commit, it made little sense to\n> disable it in the first place.\n\n\nThe feature to give a range to cherry-pick came much much later in\n7e2bfd3f (\"revert: allow cherry-picking more than one commit\",\n2010-06-02) that first appeared in v1.7.2.  The feature to allow\npicking a merge commit came in 7791ecbc (\"revert/cherry-pick: work\non merge commits as well\", 2007-10-23), first appeared in v1.5.4.\n\nIn the original context to pick a single commit, it made perfect\nsense to avoid mistakes by blindly passing '-m 1' to non-merge\ncommit.  It may be fair to say that we failed to reconsider what to\ndo with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair \nhistory revisionism to say that it made little sense to disable it\nin the first place.\n\nThe change to the code itself looks sane, but applying this patch\nalone will break existing tests whose expectations must be updated,\nand this new behaviour must be protected by a new test (or two) so\nthat we won't accidentally stop accepting \"-m 1\" for a single-parent\ncommit.\n\nThanks.\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  sequencer.c | 10 +++++++---\n>  1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index e1a4dd1..d0fd61b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n>  \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n>  \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n>  \t\tparent = p->item;\n> -\t} else if (0 < opts->mainline)\n> -\t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n> -\t\t\toid_to_hex(&commit->object.oid));\n> +\t} else if (1 < opts->mainline)\n> +\t\t/*\n> +\t\t *  Non-first parent explicitly specified as mainline for\n> +\t\t *  non-merge commit\n> +\t\t */\n> +\t\treturn error(_(\"commit %s does not have parent %d\"),\n> +\t\t\t     oid_to_hex(&commit->object.oid), opts->mainline);\n>  \telse\n>  \t\tparent = commit->parents->item;\n"},{"id":"365230","messageId":"87o99qylv5.fsf@javad.com","threadId":"48750","inReplyTo":"xmqqo99qf46q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-13T06:35:58Z","receivedAt":"2018-12-13T06:36:04Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> When cherry-picking multiple commits, it's impossible to have both\n>> merge- and non-merge commits on the same command-line. Not specifying\n>> '-m 1' results in cherry-pick refusing to handle merge commits, while\n>> specifying '-m 1' fails on non-merge commits.\n>>\n>> This patch allows '-m 1' for non-merge commits. Besides, as mainline is\n>> always the only parent for a non-merge commit, it made little sense to\n>> disable it in the first place.\n>\n> In the original context to pick a single commit, it made perfect\n> sense to avoid mistakes by blindly passing '-m 1' to non-merge\n> commit.\n>\n> It may be fair to say that we failed to reconsider what to\n> do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair \n> history revisionism to say that it made little sense to disable it\n> in the first place.\n\nIn fact I had zero intention on any revisionism. Now I see I should have\nsaid \"makes little sense\" in present tense to avoid touching deep\nhistorical issues, sorry!\n\nSomething like:\n\n\"This patch allows '-m 1' for non-merge commits. As mainline is always\nthe only parent for a non-merge commit, it makes little sense to disable\nit.\"\n\nsounds OK?\n\n>\n> The change to the code itself looks sane, but applying this patch\n> alone will break existing tests whose expectations must be updated,\n> and this new behaviour must be protected by a new test (or two) so\n> that we won't accidentally stop accepting \"-m 1\" for a single-parent\n> commit.\n\nI fixed most of the tests, but\n\n\"t3510/4: cherry-pick persists opts correctly\"\n\nis an offender for me. It looks like it [ab]uses current \"-m 1\" behavior\njust to stop in the middle of the sequence, and I'm not sure how to fix\nit most suitably.\n\nThanks!\n\n-- \nSergey\n"},{"id":"365259","messageId":"878t0twibw.fsf@javad.com","threadId":"48750","inReplyTo":"87o99qylv5.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-13T15:35:15Z","receivedAt":"2018-12-13T15:35:19Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n\n[...]\n\n>>\n>> The change to the code itself looks sane, but applying this patch\n>> alone will break existing tests whose expectations must be updated,\n>> and this new behaviour must be protected by a new test (or two) so\n>> that we won't accidentally stop accepting \"-m 1\" for a single-parent\n>> commit.\n>\n> I fixed most of the tests, but\n>\n> \"t3510/4: cherry-pick persists opts correctly\"\n>\n> is an offender for me. It looks like it [ab]uses current \"-m 1\" behavior\n> just to stop in the middle of the sequence, and I'm not sure how to fix\n> it most suitably.\n\nI came up with the following as a preparatory change. Looks acceptable?\n\n-- 8< --\n\n    t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks\n    \n    We are going to allow 'git cherry-pick -m 1' for non-merge commits, so\n    this method to force failure will stop to work.\n    \n    Use '-m 4' instead as it's very unlikely we will ever have such an\n    octopus in this test setup.\n\n\tModified   t/t3510-cherry-pick-sequence.sh\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex c84eeef..a873cf4 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n \n test_expect_success 'cherry-pick persists opts correctly' '\n \tpristine_detach initial &&\n-\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&\n+\tm=4 &&\n+\ttest_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \techo \"true\" >expect &&\n \tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n \ttest_cmp expect actual &&\n-\techo \"1\" >expect &&\n+\techo \"$m\" >expect &&\n \tgit config --file=.git/sequencer/opts --get-all options.mainline >actual &&\n \ttest_cmp expect actual &&\n \techo \"recursive\" >expect &&\n\n-- 8< --\n\n-- \nSergey\n"},{"id":"365319","messageId":"xmqq5zvwesvz.fsf@gitster-ct.c.googlers.com","threadId":"48750","inReplyTo":"878t0twibw.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-14T02:36:48Z","receivedAt":"2018-12-14T02:36:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> I came up with the following as a preparatory change. Looks acceptable?\n>\n> -- 8< --\n>\n>     t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks\n>     \n>     We are going to allow 'git cherry-pick -m 1' for non-merge commits, so\n>     this method to force failure will stop to work.\n>     \n>     Use '-m 4' instead as it's very unlikely we will ever have such an\n>     octopus in this test setup.\n\nYeah, that is a good approach.  Thanks for coming up with it.\n\nI agree that it also is a good idea to use a variable to avoid\nrepeating \"4\" (and risking the two uses of the constant drifting\napart), but I find a single letter variable 'm' a bit too bland and\nnot descriptive enough.  Perhaps spell it out as mainline=4,\npossibly with a comment why that is not a more-commonly-seen number\nlike \"1\"?\n\n\t# to make sure that the session to cherry-pick a sequence\n\t# gets interrupted, use a high-enough number that is larger\n\t# than the number of parents of any commit\n\tmainline=4 &&\n\nor something.\n\n> \tModified   t/t3510-cherry-pick-sequence.sh\n> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\n> index c84eeef..a873cf4 100755\n> --- a/t/t3510-cherry-pick-sequence.sh\n> +++ b/t/t3510-cherry-pick-sequence.sh\n> @@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n>  \n>  test_expect_success 'cherry-pick persists opts correctly' '\n>  \tpristine_detach initial &&\n> -\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&\n> +\tm=4 &&\n> +\ttest_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick &&\n>  \ttest_path_is_dir .git/sequencer &&\n>  \ttest_path_is_file .git/sequencer/head &&\n>  \ttest_path_is_file .git/sequencer/todo &&\n> @@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n>  \techo \"true\" >expect &&\n>  \tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n>  \ttest_cmp expect actual &&\n> -\techo \"1\" >expect &&\n> +\techo \"$m\" >expect &&\n>  \tgit config --file=.git/sequencer/opts --get-all options.mainline >actual &&\n>  \ttest_cmp expect actual &&\n>  \techo \"recursive\" >expect &&\n>\n> -- 8< --\n"},{"id":"366208","messageId":"cover.1546789223.git.sorganov@gmail.com","threadId":"48750","inReplyTo":"xmqq5zvwesvz.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3 0/4] Allow 'cherry-pick -m 1' for non-merge commits","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-14T04:39:03Z","receivedAt":"2018-12-14T04:39:03Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Change from v2: t/t3502: disambiguation added to prevent failing on\ncase-insensitive file systems.\n\nWhen cherry-picking multiple commits, it's impossible to have both\nmerge- and non-merge commits on the same command-line. Not specifying\n'-m 1' results in cherry-pick refusing to handle merge commits, while\nspecifying '-m 1' fails on non-merge commits.\n\nThis patch series allow '-m 1' for non-merge commits as well as fixes\nrelevant tests in accordance.\n\nIt also opens the way to making '-m 1' the default, that would be\ninline with the trends to assume first parent to be the mainline in\nmost workflows.\n\nSergey Organov (4):\n  t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks\n  cherry-pick: do not error on non-merge commits when '-m 1' is\n    specified\n  t3502: validate '-m 1' argument is now accepted for non-merge commits\n  t3506: validate '-m 1 -ff' is now accepted for non-merge commits\n\n sequencer.c                     | 10 +++++++---\n t/t3502-cherry-pick-merge.sh    | 12 ++++++------\n t/t3506-cherry-pick-ff.sh       |  6 +++---\n t/t3510-cherry-pick-sequence.sh |  8 ++++++--\n 4 files changed, 22 insertions(+), 14 deletions(-)\n\n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"366209","messageId":"47bde24fd077a7f2a5a6db4c9bb9411c7255b89a.1546789304.git.sorganov@gmail.com","threadId":"48750","inReplyTo":"cover.1546789223.git.sorganov@gmail.com","subject":"[PATCH v3 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-14T04:53:51Z","receivedAt":"2018-12-14T04:53:51Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"We are going to allow 'git cherry-pick -m 1' for non-merge commits, so\nthis method to force failure will stop to work.\n\nUse '-m 4' instead as it's very unlikely we will ever have such an\noctopus in this test setup.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t3510-cherry-pick-sequence.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex c84eeef..941d502 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '\n \n test_expect_success 'cherry-pick persists opts correctly' '\n \tpristine_detach initial &&\n-\ttest_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&\n+\t# to make sure that the session to cherry-pick a sequence\n+\t# gets interrupted, use a high-enough number that is larger\n+\t# than the number of parents of any commit we have created\n+\tmainline=4 &&\n+\ttest_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&\n \ttest_path_is_dir .git/sequencer &&\n \ttest_path_is_file .git/sequencer/head &&\n \ttest_path_is_file .git/sequencer/todo &&\n@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '\n \techo \"true\" >expect &&\n \tgit config --file=.git/sequencer/opts --get-all options.signoff >actual &&\n \ttest_cmp expect actual &&\n-\techo \"1\" >expect &&\n+\techo \"$mainline\" >expect &&\n \tgit config --file=.git/sequencer/opts --get-all options.mainline >actual &&\n \ttest_cmp expect actual &&\n \techo \"recursive\" >expect &&\n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"366210","messageId":"a8b86bc307141963d0ce6b8f8710b38bd802e302.1546789304.git.sorganov@gmail.com","threadId":"48750","inReplyTo":"cover.1546789223.git.sorganov@gmail.com","subject":"[PATCH v3 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-14T04:53:51Z","receivedAt":"2018-12-14T04:53:51Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"When cherry-picking multiple commits, it's impossible to have both\nmerge- and non-merge commits on the same command-line. Not specifying\n'-m 1' results in cherry-pick refusing to handle merge commits, while\nspecifying '-m 1' fails on non-merge commits.\n\nThis patch allows '-m 1' for non-merge commits. As mainline is always\nthe only parent for a non-merge commit, it makes little sense to\ndisable it.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n sequencer.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e1a4dd1..d0fd61b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\treturn error(_(\"commit %s does not have parent %d\"),\n \t\t\t\toid_to_hex(&commit->object.oid), opts->mainline);\n \t\tparent = p->item;\n-\t} else if (0 < opts->mainline)\n-\t\treturn error(_(\"mainline was specified but commit %s is not a merge.\"),\n-\t\t\toid_to_hex(&commit->object.oid));\n+\t} else if (1 < opts->mainline)\n+\t\t/*\n+\t\t *  Non-first parent explicitly specified as mainline for\n+\t\t *  non-merge commit\n+\t\t */\n+\t\treturn error(_(\"commit %s does not have parent %d\"),\n+\t\t\t     oid_to_hex(&commit->object.oid), opts->mainline);\n \telse\n \t\tparent = commit->parents->item;\n \n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"366211","messageId":"dbed11a33444f308edb729282e26bc6048aebdc4.1546789304.git.sorganov@gmail.com","threadId":"48750","inReplyTo":"cover.1546789223.git.sorganov@gmail.com","subject":"[PATCH v3 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-14T04:53:51Z","receivedAt":"2018-12-14T04:53:51Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Signed-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t3502-cherry-pick-merge.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\nindex b160271..8b635a1 100755\n--- a/t/t3502-cherry-pick-merge.sh\n+++ b/t/t3502-cherry-pick-merge.sh\n@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '\n \ttest_expect_code 129 git cherry-pick -m 0 b\n '\n \n-test_expect_success 'cherry-pick a non-merge with -m should fail' '\n+test_expect_success 'cherry-pick explicit first parent of a non-merge' '\n \n \tgit reset --hard &&\n \tgit checkout a^0 &&\n-\ttest_expect_code 128 git cherry-pick -m 1 b &&\n-\tgit diff --exit-code a --\n+\tgit cherry-pick -m 1 b &&\n+\tgit diff --exit-code c --\n \n '\n \n@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f\n \n '\n \n-test_expect_success 'revert a non-merge with -m should fail' '\n+test_expect_success 'revert explicit first parent of a non-merge' '\n \n \tgit reset --hard &&\n \tgit checkout c^0 &&\n-\ttest_must_fail git revert -m 1 b &&\n-\tgit diff --exit-code c\n+\tgit revert -m 1 b &&\n+\tgit diff --exit-code a --\n \n '\n \n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"366212","messageId":"7a93e62dd12bd890ea13c7b3dd483ed4bafcf642.1546789304.git.sorganov@gmail.com","threadId":"48750","inReplyTo":"cover.1546789223.git.sorganov@gmail.com","subject":"[PATCH v3 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2018-12-14T04:53:51Z","receivedAt":"2018-12-14T04:53:51Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Signed-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t3506-cherry-pick-ff.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh\nindex fb889ac..127dd00 100755\n--- a/t/t3506-cherry-pick-ff.sh\n+++ b/t/t3506-cherry-pick-ff.sh\n@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '\n \tgit checkout -b new A\n '\n \n-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '\n+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '\n \tgit reset --hard A -- &&\n-\ttest_must_fail git cherry-pick --ff -m 1 B &&\n-\tgit diff --exit-code A --\n+\tgit cherry-pick --ff -m 1 B &&\n+\tgit diff --exit-code C --\n '\n \n test_expect_success 'cherry pick a merge with --ff but without -m should fail' '\n-- \n2.10.0.1.g57b01a3\n\n"},{"id":"371901","messageId":"8736nj2jcl.fsf@javad.com","threadId":"48750","inReplyTo":"xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-03-19T11:29:14Z","receivedAt":"2019-03-19T11:29:19Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Hi Junio,\n\n[It's been a while since this discussion, and recently I've got some\nthoughts and questions about \"first-parent\" issues in general, below.]\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> When cherry-picking multiple commits, it's impossible to have both\n>> merge- and non-merge commits on the same command-line. Not specifying\n>> '-m 1' results in cherry-pick refusing to handle merge commits, while\n>> specifying '-m 1' fails on non-merge commits.\n>\n> Allowing \"-m1\" even when picking a single parent commit, because\n> the 1-st parent is defined for such a commit, makes sense, espeially\n> when running a cherry-pick on a range, exactly for the above reason.\n> It is slightly less so when cherry-picking a single commit, but not\n> by a large margin.\n>\n> I think the original reasoning for requiring \"-m $n\" not present,\n> especially because cherry-pick was originally for replaying only a\n> single commit, was because it would lead somebody to propose that\n> the command should behave as if -m1 is always given (and when trying\n> to cherry-pick a merge relative to its second parent, give -m2 to\n> override it), which in turn encourage the 'first-parent is special'\n> world-view from the tool-side.  IOW, \"The worldview to treat the\n> first-parent chain specially is correct, because Git has many\n> features to work with that worldview conveniently\" was something we\n> wanted to avoid; rather \"Such and such workflows benefit from\n> treating the first-parent chain specially, so let's add features to\n> do so\" was we wanted to do, and of course, back then cherry-pick\n> that allows mixture of merges and single-parent commits to be\n> picked, which would have made the need to do something like this\n> patch does felt greater, did not exist.\n>\n> Now, it appears, at least to me, that the world pretty much accepted\n> that the first-parent worldview is often very convenient and worth\n> supporting by the tool, so the next logical step might be to set\n> opts->mainline to 1 by default (and allow an explicit \"-m $n\" from\n> the command line to override it).  But that should happen after this\n> patch lands---it is logically a separate step, I would think.\n\nI think that \"first-parent is special\" is the way to go indeed for\nporcelain, as it does make many thing easier and more convenient[*].\n\nIs there a road-map already outlined in that direction, I wonder?\n\nOTOH, for plumbing, it's rather keeping the original pure-DAG\n\"symmetrical merges\" approach that seems to be the right thing to do.\n\n[*] One example that immediately comes to mind is \"git log -p\" for a\nmerge commit. I doesn't currently (as of v2.10) show the first-parent\ndiff, for whatever reason. \"git log -p -m --first-parent\" is needed to\nget the answer to most \"obvious\" question: what (merge) commit did to my\nmainline? \"git show\" has its own issues.\n\n-- Sergey\n"},{"id":"371937","messageId":"xmqqbm26xtum.fsf@gitster-ct.c.googlers.com","threadId":"48750","inReplyTo":"8736nj2jcl.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-20T00:38:57Z","receivedAt":"2019-03-20T00:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> I think that \"first-parent is special\" is the way to go indeed for\n> porcelain, as it does make many thing easier and more convenient[*].\n\nPerhaps.  However ...\n\n> [*] One example that immediately comes to mind is \"git log -p\" for a\n> merge commit. I doesn't currently (as of v2.10) show the first-parent\n> diff, for whatever reason. \"git log -p -m --first-parent\" is needed to\n> get the answer to most \"obvious\" question: what (merge) commit did to my\n> mainline? \"git show\" has its own issues.\n\n... this is very much deliberate and will remain so.\n\nA single ball of wax \"diff M^ M\" for a merge commit is not always\nwhat you would want, especially while viewing \"git log -p\" (without\n\"--first-parent\").  The reason why the user does not explicitly say\n\"--first-parent\" is because the user wants to even see the details\nof individual steps of what happened on side branches.\n\nIncidentally, in such an \"I am interested in what happened in each\nindividual step\" mode, the primary change that a merge by itself\ndoes is better shown with \"diff --cc M^ M\", not \"diff -p M^ M\".\nThat is why \"show\" defaults to \"--cc\".\n\n\"git log -p --first-parent\" that requires \"-m\" to show the single\nball of wax diff for a merge is a separate matter.  When the user\nexplicitly says \"log --first-parent\", it is a clear indication that\nthe user does not want to see individual steps of how side branches\nbuilt what each merge brings into the mainline.  From that point of\nview, ever sice I introduced \"--first-parent\" traversal, I've been\nwondering what the true downside would be if we turned \"-m\" on\nautomatically when these two options are used without \"-m\".\n\nBut it has not disturbed me deeply enough to bother looking into it\nfurther.\n"},{"id":"371953","messageId":"20190320050944.GB6401@sigill.intra.peff.net","threadId":"48750","inReplyTo":"xmqqbm26xtum.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-20T05:09:44Z","receivedAt":"2019-03-20T05:09:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 20, 2019 at 09:38:57AM +0900, Junio C Hamano wrote:\n\n> \"git log -p --first-parent\" that requires \"-m\" to show the single\n> ball of wax diff for a merge is a separate matter.  When the user\n> explicitly says \"log --first-parent\", it is a clear indication that\n> the user does not want to see individual steps of how side branches\n> built what each merge brings into the mainline.  From that point of\n> view, ever sice I introduced \"--first-parent\" traversal, I've been\n> wondering what the true downside would be if we turned \"-m\" on\n> automatically when these two options are used without \"-m\".\n\nSort of a drive-by two cents, but I have often wondered the same thing.\nI cannot think of a time when I wanted \"--first-parent\" without \"-m\"\n(unless I was not viewing diffs at all).\n\n(And I agree with everything else you said :) ).\n\n-Peff\n"},{"id":"372400","messageId":"87h8bra1z6.fsf@javad.com","threadId":"48750","inReplyTo":"xmqqbm26xtum.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-03-25T06:43:09Z","receivedAt":"2019-03-25T06:43:14Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> I think that \"first-parent is special\" is the way to go indeed for\n>> porcelain, as it does make many thing easier and more convenient[*].\n>\n> Perhaps.  However ...\n>\n>> [*] One example that immediately comes to mind is \"git log -p\" for a\n>> merge commit. I doesn't currently (as of v2.10) show the first-parent\n>> diff, for whatever reason. \"git log -p -m --first-parent\" is needed to\n>> get the answer to most \"obvious\" question: what (merge) commit did to my\n>> mainline? \"git show\" has its own issues.\n>\n> ... this is very much deliberate and will remain so.\n\n> A single ball of wax \"diff M^ M\" for a merge commit is not always\n> what you would want, especially while viewing \"git log -p\" (without\n> \"--first-parent\").\n\nOK, point taken. Then it's an issue of suppressing (presumably huge)\nparts of output for merge commits by default, and is only vaguely\nrelevant to the \"first parent is special\" trend that I intended to\ndiscuss. So, let's leave in peace the \"git log -p\" for now, and let me\ntry it from different angle.\n\nHow about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n\"diff-tree --cc M\" for merge commits? It's really surprising specifying\n-p has no visible effect.\n\nAlso, is current output of \"git log -m\", being extremely confusing,\nsuitable for anything? Maybe consider to change it to output diff with\nrespect to the first parent only? Though it's then a pity \"-m\" lacks\nargument here, similar to what it has in cherry-pick.\n\n-- Sergey\n"},{"id":"372530","messageId":"20190326163204.GC29627@sigill.intra.peff.net","threadId":"48750","inReplyTo":"87h8bra1z6.fsf@javad.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T16:32:04Z","receivedAt":"2019-03-26T16:32:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:\n\n> How about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n> \"diff-tree --cc M\" for merge commits? It's really surprising specifying\n> -p has no visible effect.\n\nThat's because \"-p\" is already the default, and the format selection is\northogonal to the handling of merge commits. Providing \"-m\" would\nactually override the \"--cc\" default (though \"--first-parent -m\" is\nlikely to be less noisy, per this discussion).\n\nAs far as defaults go, I dunno. The idea is that \"--cc\" would give you a\nnice summary of what the merge _itself_ had to touch. I think that's\nvaluable, too. If we were starting from scratch, I think there could be\na discussion about whether one default is better than the other. But at\nthis point I have a hard time finding one so much obviously better than\nthe other to merit changing the behavior.\n\n> Also, is current output of \"git log -m\", being extremely confusing,\n> suitable for anything? Maybe consider to change it to output diff with\n> respect to the first parent only? Though it's then a pity \"-m\" lacks\n> argument here, similar to what it has in cherry-pick.\n\nI've used \"-m\" without \"--first-parent\" sometimes in order to track down\nmis-merges manually. It's not usually a big deal to say \"--first-parent\"\nif that's what you want. But one thing I don't think is currently\npossible is to ask only for the first-parent diff, but _not_ restrict\nthe actual traversal.\n\nIf that's what you mean by giving an argument to \"-m\", then yeah, I\nthink that would be a useful addition.\n\n-Peff\n"},{"id":"372551","messageId":"CABPp-BF7cd2+4jr=zGVP8x_QLdFbyOoGRWUgMtgiGDCdqZdjFA@mail.gmail.com","threadId":"48750","inReplyTo":"20190326163204.GC29627@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-03-26T22:07:42Z","receivedAt":"2019-03-26T22:07:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:\n>\n> > How about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n> > \"diff-tree --cc M\" for merge commits? It's really surprising specifying\n> > -p has no visible effect.\n>\n> That's because \"-p\" is already the default, and the format selection is\n> orthogonal to the handling of merge commits. Providing \"-m\" would\n> actually override the \"--cc\" default (though \"--first-parent -m\" is\n> likely to be less noisy, per this discussion).\n>\n> As far as defaults go, I dunno. The idea is that \"--cc\" would give you a\n> nice summary of what the merge _itself_ had to touch. I think that's\n> valuable, too. If we were starting from scratch, I think there could be\n> a discussion about whether one default is better than the other. But at\n> this point I have a hard time finding one so much obviously better than\n> the other to merit changing the behavior.\n\nIndeed, some of us would view a first parent diff default for merges\nas problematic.  However, I'd like to point out (or remind) that these\ntwo options aren't the only ways you could view a merge.  Thomas\nRast's --remerge-diff[1] is another (even if not yet part of git.git).\nGerrit uses something similar-ish for its default way of showing a\nmerge.\n\n[1] See e.g. https://bugs.chromium.org/p/git/issues/detail?id=12\n"},{"id":"372553","messageId":"20190326222052.GD1445@sigill.intra.peff.net","threadId":"48750","inReplyTo":"CABPp-BF7cd2+4jr=zGVP8x_QLdFbyOoGRWUgMtgiGDCdqZdjFA@mail.gmail.com","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T22:20:52Z","receivedAt":"2019-03-26T22:20:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote:\n\n> On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:\n> >\n> > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:\n> >\n> > > How about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n> > > \"diff-tree --cc M\" for merge commits? It's really surprising specifying\n> > > -p has no visible effect.\n> >\n> > That's because \"-p\" is already the default, and the format selection is\n> > orthogonal to the handling of merge commits. Providing \"-m\" would\n> > actually override the \"--cc\" default (though \"--first-parent -m\" is\n> > likely to be less noisy, per this discussion).\n> >\n> > As far as defaults go, I dunno. The idea is that \"--cc\" would give you a\n> > nice summary of what the merge _itself_ had to touch. I think that's\n> > valuable, too. If we were starting from scratch, I think there could be\n> > a discussion about whether one default is better than the other. But at\n> > this point I have a hard time finding one so much obviously better than\n> > the other to merit changing the behavior.\n> \n> Indeed, some of us would view a first parent diff default for merges\n> as problematic.  However, I'd like to point out (or remind) that these\n> two options aren't the only ways you could view a merge.  Thomas\n> Rast's --remerge-diff[1] is another (even if not yet part of git.git).\n> Gerrit uses something similar-ish for its default way of showing a\n> merge.\n\nHeh, I almost mentioned remerge-diff, but since it's not actually part\nof Git, I didn't want to get into a tangent. But since you mention it,\nyes, I actually find it quite a useful way of looking at the diff,\nespecially when I want to see what the person resolving the conflicts\nactually _did_. The --cc combined diff is too eager to throw away hunks\nthat resolved purely to one side (which _most_ of the time is what you\nwant, but when you're hunting a possible error in the merge, it's quite\nconfusing).\n\nHow close is merge-recursive.c to actually doing a pure in-memory merge?\n\nI seem to recall that was a (the?) sticking point for the original\nremerge-diff.\n\n-Peff\n"},{"id":"372554","messageId":"CABPp-BGoH3qE0VqMyov-3YTSFSgDBy6xTKKgrckabnAt2pz05A@mail.gmail.com","threadId":"48750","inReplyTo":"20190326222052.GD1445@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-03-27T00:33:47Z","receivedAt":"2019-03-27T00:34:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 26, 2019 at 3:20 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote:\n>\n> > On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:\n> > >\n> > > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:\n> > >\n> > > > How about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n> > > > \"diff-tree --cc M\" for merge commits? It's really surprising specifying\n> > > > -p has no visible effect.\n> > >\n> > > That's because \"-p\" is already the default, and the format selection is\n> > > orthogonal to the handling of merge commits. Providing \"-m\" would\n> > > actually override the \"--cc\" default (though \"--first-parent -m\" is\n> > > likely to be less noisy, per this discussion).\n> > >\n> > > As far as defaults go, I dunno. The idea is that \"--cc\" would give you a\n> > > nice summary of what the merge _itself_ had to touch. I think that's\n> > > valuable, too. If we were starting from scratch, I think there could be\n> > > a discussion about whether one default is better than the other. But at\n> > > this point I have a hard time finding one so much obviously better than\n> > > the other to merit changing the behavior.\n> >\n> > Indeed, some of us would view a first parent diff default for merges\n> > as problematic.  However, I'd like to point out (or remind) that these\n> > two options aren't the only ways you could view a merge.  Thomas\n> > Rast's --remerge-diff[1] is another (even if not yet part of git.git).\n> > Gerrit uses something similar-ish for its default way of showing a\n> > merge.\n>\n> Heh, I almost mentioned remerge-diff, but since it's not actually part\n> of Git, I didn't want to get into a tangent. But since you mention it,\n> yes, I actually find it quite a useful way of looking at the diff,\n> especially when I want to see what the person resolving the conflicts\n> actually _did_. The --cc combined diff is too eager to throw away hunks\n> that resolved purely to one side (which _most_ of the time is what you\n> want, but when you're hunting a possible error in the merge, it's quite\n> confusing).\n>\n> How close is merge-recursive.c to actually doing a pure in-memory merge?\n>\n> I seem to recall that was a (the?) sticking point for the original\n> remerge-diff.\n\nDoing a pure in-memory merge is tied up with my overall\nmerge-recursive rewrite, but I haven't touched merge stuff in quite a\nwhile now other than the recent\nmake-directory-rename-detection-be-a-conflict-by-default stuff.  I'm\nhoping I can finish that soon (though I've struggled a bit to find the\ntime to do so), and finish out the filter-repo stuff, then I plan to\nget back to merge stuff again.  A pure in-memory merge is near the top\nof my priorities for the rewrite, so we'll get it...eventually.\n(Maybe a Christmas present?)\n\nI do think there's more than just that sticking point for the\nremerge-diff, but the other things are on my radar too (a few slots\nlater on my todo list).\n"},{"id":"372578","messageId":"878sx0xw1u.fsf@javad.com","threadId":"48750","inReplyTo":"20190326163204.GC29627@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2019-03-27T13:54:05Z","receivedAt":"2019-03-27T13:54:09Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote:\n>\n>> How about changing \"git show -p M\" to output \"diff -p M^ M\" rather than\n>> \"diff-tree --cc M\" for merge commits? It's really surprising specifying\n>> -p has no visible effect.\n>\n> That's because \"-p\" is already the default, and the format selection is\n> orthogonal to the handling of merge commits.\n\nSeems to be more convoluted than that. If \"-p\" were simply default, then\n\"git show --raw\" and \"git show -p --raw\" would output the same thing.\nThey don't.\n\nThat said, does \"-p\" select a format, or not? Should it? For me,\n\"--patch\" sounds specific enough to expect it to select the format that\nis most close to what \"patch\" utility is able to apply, that would still\nbe \"diff -p M^ M\" format, universally, be it merge commit or not.\n\n> Providing \"-m\" would actually override the \"--cc\" default (though\n> \"--first-parent -m\" is likely to be less noisy, per this discussion).\n\nRight, but \"--first-parent\" (while accepted) even is not documented for\n\"git show\", and it could be argued if history traversal option has any\nsense for a command that shows separate commits.\n\nFurther, even in \"git log\", having the \"--first-parent\" change the way\ndiffs are output is, strictly speaking, violation of orthogonality (that\nnobody seems to care about).\n\n> As far as defaults go, I dunno.\n\nI'm not after changing the default behavior. I'm rather after making the\nwhole system of options somewhat more logical, self consistent, and thus\nless confusing.\n\n> The idea is that \"--cc\" would give you a nice summary of what the\n> merge _itself_ had to touch. I think that's valuable, too.\n\nI'm not against \"--cc\" either, be it a default or not, even though\npersonally for me it's not very useful, as it shows how merge (the\noperation) supposedly went, when I'm usually interested in how merge\n(the resulting commit) affects the mainline, no matter how this result\nhas been achieved.\n\nAnother thought about \"--cc\" is that it's in fact a case of \"merges are\nsymmetric\" approach to the UI that is supposedly shifting to \"mainline\nis special\" one.\n\n> If we were starting from scratch, I think there could be\n> a discussion about whether one default is better than the other. But at\n> this point I have a hard time finding one so much obviously better than\n> the other to merit changing the behavior.\n\nI'm yet to figure what exactly the best set of options would be, even\npersonally for me, even in the \"start from scratch\" case, so, for now,\nI'm basically just gathering relevant information and opinions.\n\n>> Also, is current output of \"git log -m\", being extremely confusing,\n>> suitable for anything? Maybe consider to change it to output diff with\n>> respect to the first parent only? Though it's then a pity \"-m\" lacks\n>> argument here, similar to what it has in cherry-pick.\n>\n> I've used \"-m\" without \"--first-parent\" sometimes in order to track down\n> mis-merges manually.\n\nHave you been interested specifically in diffs with respect to side\nbranches in these cases, I wonder, or did you actually look only at \"-m\n1\" part of the whole \"-m\" output?\n\nWhen I tried \"-m\", I found it rather difficult to even visually find the\nmargin between diffs to different parents, that confused me and forced\nto resort to other methods. Besides, it didn't appear to me at that time\nthat there is \"--first-parent\" that might \"help\", so as I recall I ended\nup using \"diff -p M~1 M\" for the merge in question.\n\n> It's not usually a big deal to say \"--first-parent\" if that's what you\n> want. But one thing I don't think is currently possible is to ask only\n> for the first-parent diff, but _not_ restrict the actual traversal.\n\nThat's due to \"--first-parent\" breaking orthogonality. It should rather\nonly affect graph traversal, I'd expect.\n\nAdmittedly, it may imply some other option(s) for convenience (say, such\noption could have been \"-m 1\", if it existed), but then there /must/\nexist the option(s) it implies in the first place. Currently,\n\"--first-parent\" behaves as if it implies some nonexistent option, so\nthe user has no way indeed to provide the latter without the former.\n\n> If that's what you mean by giving an argument to \"-m\", then yeah, I\n> think that would be a useful addition.\n\nI thought that maybe the part of \"-m\" that outputs diffs to side\nbranch(es) is in fact useless feature when result is to be directly\nconsumed by human beings. Then, if we decide to change it to output diff\nto single parent for porcelain command(s), it may be useful to be able\nto explicitly ask for other parents than the default, the first one.\n\nIt also strikes me as inconsistent that \"-m\" in log/show on one hand,\nand \"-m\" in cherry-pick on the other, being essentially the same thing,\nare so different in appearance and description.\n\nUnfortunately, adding an argument to \"-m\" bumps either into generic\nevilness of optional arguments for options, or into backward\nincompatibility (if the argument to \"-m\" becomes mandatory), so it\ndoesn't seem to be such a good thing to do after all.\n\n-- Sergey\n"}]}