* [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified @ 2018-05-25 12:42 Sergey Organov 0 siblings, 0 replies; 18+ messages in thread From: Sergey Organov @ 2018-05-25 12:42 UTC (permalink / raw) To: git; +Cc: gitster When cherry-picking multiple commits, it's impossible to have both merge- and non-merge commits on the same command-line. Not specifying '-m 1' results in cherry-pick refusing to handle merge commits, while specifying '-m 1' fails on non-merge commits. This patch allows '-m 1' for non-merge commits. Besides, as mainline is always the only parent for a non-merge commit, it made little sense to disable it in the first place. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- sequencer.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1ce6326..2393bdf 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, return error(_("commit %s does not have parent %d"), oid_to_hex(&commit->object.oid), opts->mainline); parent = p->item; - } else if (0 < opts->mainline) - return error(_("mainline was specified but commit %s is not a merge."), - oid_to_hex(&commit->object.oid)); + } else if (1 < opts->mainline) + /* Non-first parent explicitly specified as mainline for + * non-merge commit */ + return error(_("commit %s does not have parent %d"), + oid_to_hex(&commit->object.oid), opts->mainline); else parent = commit->parents->item; -- 2.10.0.1.g57b01a3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified @ 2018-05-25 12:42 Sergey Organov 2018-06-21 15:54 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Sergey Organov @ 2018-05-25 12:42 UTC (permalink / raw) To: git; +Cc: gitster When cherry-picking multiple commits, it's impossible to have both merge- and non-merge commits on the same command-line. Not specifying '-m 1' results in cherry-pick refusing to handle merge commits, while specifying '-m 1' fails on non-merge commits. This patch allows '-m 1' for non-merge commits. Besides, as mainline is always the only parent for a non-merge commit, it made little sense to disable it in the first place. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- sequencer.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 1ce6326..2393bdf 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, return error(_("commit %s does not have parent %d"), oid_to_hex(&commit->object.oid), opts->mainline); parent = p->item; - } else if (0 < opts->mainline) - return error(_("mainline was specified but commit %s is not a merge."), - oid_to_hex(&commit->object.oid)); + } else if (1 < opts->mainline) + /* Non-first parent explicitly specified as mainline for + * non-merge commit */ + return error(_("commit %s does not have parent %d"), + oid_to_hex(&commit->object.oid), opts->mainline); else parent = commit->parents->item; -- 2.10.0.1.g57b01a3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-05-25 12:42 Sergey Organov @ 2018-06-21 15:54 ` Junio C Hamano 2018-06-22 9:16 ` Sergey Organov ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Junio C Hamano @ 2018-06-21 15:54 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > When cherry-picking multiple commits, it's impossible to have both > merge- and non-merge commits on the same command-line. Not specifying > '-m 1' results in cherry-pick refusing to handle merge commits, while > specifying '-m 1' fails on non-merge commits. Allowing "-m1" even when picking a single parent commit, because the 1-st parent is defined for such a commit, makes sense, espeially when running a cherry-pick on a range, exactly for the above reason. It is slightly less so when cherry-picking a single commit, but not by a large margin. I think the original reasoning for requiring "-m $n" not present, especially because cherry-pick was originally for replaying only a single commit, was because it would lead somebody to propose that the command should behave as if -m1 is always given (and when trying to cherry-pick a merge relative to its second parent, give -m2 to override it), which in turn encourage the 'first-parent is special' world-view from the tool-side. IOW, "The worldview to treat the first-parent chain specially is correct, because Git has many features to work with that worldview conveniently" was something we wanted to avoid; rather "Such and such workflows benefit from treating the first-parent chain specially, so let's add features to do so" was we wanted to do, and of course, back then cherry-pick that allows mixture of merges and single-parent commits to be picked, which would have made the need to do something like this patch does felt greater, did not exist. Now, it appears, at least to me, that the world pretty much accepted that the first-parent worldview is often very convenient and worth supporting by the tool, so the next logical step might be to set opts->mainline to 1 by default (and allow an explicit "-m $n" from the command line to override it). But that should happen after this patch lands---it is logically a separate step, I would think. > This patch allows '-m 1' for non-merge commits. Besides, as mainline is > always the only parent for a non-merge commit, it made little sense to > disable it in the first place. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > sequencer.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 1ce6326..2393bdf 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > return error(_("commit %s does not have parent %d"), > oid_to_hex(&commit->object.oid), opts->mainline); > parent = p->item; > - } else if (0 < opts->mainline) > - return error(_("mainline was specified but commit %s is not a merge."), > - oid_to_hex(&commit->object.oid)); > + } else if (1 < opts->mainline) > + /* Non-first parent explicitly specified as mainline for > + * non-merge commit */ Style. /* * Our multi-line comments are to be * formatted like this. */ > + return error(_("commit %s does not have parent %d"), > + oid_to_hex(&commit->object.oid), opts->mainline); > else > parent = commit->parents->item; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-06-21 15:54 ` Junio C Hamano @ 2018-06-22 9:16 ` Sergey Organov 2018-12-12 5:35 ` Sergey Organov 2019-03-19 11:29 ` Sergey Organov 2 siblings, 0 replies; 18+ messages in thread From: Sergey Organov @ 2018-06-22 9:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> When cherry-picking multiple commits, it's impossible to have both >> merge- and non-merge commits on the same command-line. Not specifying >> '-m 1' results in cherry-pick refusing to handle merge commits, while >> specifying '-m 1' fails on non-merge commits. > > Allowing "-m1" even when picking a single parent commit, because > the 1-st parent is defined for such a commit, makes sense, espeially > when running a cherry-pick on a range, exactly for the above reason. > It is slightly less so when cherry-picking a single commit, but not > by a large margin. [...] >> + } else if (1 < opts->mainline) >> + /* Non-first parent explicitly specified as mainline for >> + * non-merge commit */ > > Style. > > /* > * Our multi-line comments are to be > * formatted like this. > */ Ah, sorry for that. Will you fix it for me? Thanks! -- Sergey ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-06-21 15:54 ` Junio C Hamano 2018-06-22 9:16 ` Sergey Organov @ 2018-12-12 5:35 ` Sergey Organov 2018-12-13 4:20 ` Junio C Hamano 2019-03-19 11:29 ` Sergey Organov 2 siblings, 1 reply; 18+ messages in thread From: Sergey Organov @ 2018-12-12 5:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> When cherry-picking multiple commits, it's impossible to have both >> merge- and non-merge commits on the same command-line. Not specifying >> '-m 1' results in cherry-pick refusing to handle merge commits, while >> specifying '-m 1' fails on non-merge commits. > [...] > Now, it appears, at least to me, that the world pretty much accepted > that the first-parent worldview is often very convenient and worth > supporting by the tool, so the next logical step might be to set > opts->mainline to 1 by default (and allow an explicit "-m $n" from > the command line to override it). But that should happen after this > patch lands---it is logically a separate step, I would think. > [...] >> + } else if (1 < opts->mainline) >> + /* Non-first parent explicitly specified as mainline for >> + * non-merge commit */ > > Style. > > /* > * Our multi-line comments are to be > * formatted like this. > */ Comment style fixed: -- 8< -- When cherry-picking multiple commits, it's impossible to have both merge- and non-merge commits on the same command-line. Not specifying '-m 1' results in cherry-pick refusing to handle merge commits, while specifying '-m 1' fails on non-merge commits. This patch allows '-m 1' for non-merge commits. Besides, as mainline is always the only parent for a non-merge commit, it made little sense to disable it in the first place. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- sequencer.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index e1a4dd1..d0fd61b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, return error(_("commit %s does not have parent %d"), oid_to_hex(&commit->object.oid), opts->mainline); parent = p->item; - } else if (0 < opts->mainline) - return error(_("mainline was specified but commit %s is not a merge."), - oid_to_hex(&commit->object.oid)); + } else if (1 < opts->mainline) + /* + * Non-first parent explicitly specified as mainline for + * non-merge commit + */ + return error(_("commit %s does not have parent %d"), + oid_to_hex(&commit->object.oid), opts->mainline); else parent = commit->parents->item; -- 2.10.0.1.g57b01a3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-12-12 5:35 ` Sergey Organov @ 2018-12-13 4:20 ` Junio C Hamano 2018-12-13 6:35 ` Sergey Organov 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2018-12-13 4:20 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > When cherry-picking multiple commits, it's impossible to have both > merge- and non-merge commits on the same command-line. Not specifying > '-m 1' results in cherry-pick refusing to handle merge commits, while > specifying '-m 1' fails on non-merge commits. > > This patch allows '-m 1' for non-merge commits. Besides, as mainline is > always the only parent for a non-merge commit, it made little sense to > disable it in the first place. The feature to give a range to cherry-pick came much much later in 7e2bfd3f ("revert: allow cherry-picking more than one commit", 2010-06-02) that first appeared in v1.7.2. The feature to allow picking a merge commit came in 7791ecbc ("revert/cherry-pick: work on merge commits as well", 2007-10-23), first appeared in v1.5.4. In the original context to pick a single commit, it made perfect sense to avoid mistakes by blindly passing '-m 1' to non-merge commit. It may be fair to say that we failed to reconsider what to do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair history revisionism to say that it made little sense to disable it in the first place. The change to the code itself looks sane, but applying this patch alone will break existing tests whose expectations must be updated, and this new behaviour must be protected by a new test (or two) so that we won't accidentally stop accepting "-m 1" for a single-parent commit. Thanks. > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > sequencer.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index e1a4dd1..d0fd61b 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, > return error(_("commit %s does not have parent %d"), > oid_to_hex(&commit->object.oid), opts->mainline); > parent = p->item; > - } else if (0 < opts->mainline) > - return error(_("mainline was specified but commit %s is not a merge."), > - oid_to_hex(&commit->object.oid)); > + } else if (1 < opts->mainline) > + /* > + * Non-first parent explicitly specified as mainline for > + * non-merge commit > + */ > + return error(_("commit %s does not have parent %d"), > + oid_to_hex(&commit->object.oid), opts->mainline); > else > parent = commit->parents->item; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-12-13 4:20 ` Junio C Hamano @ 2018-12-13 6:35 ` Sergey Organov 2018-12-13 15:35 ` Sergey Organov 0 siblings, 1 reply; 18+ messages in thread From: Sergey Organov @ 2018-12-13 6:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> When cherry-picking multiple commits, it's impossible to have both >> merge- and non-merge commits on the same command-line. Not specifying >> '-m 1' results in cherry-pick refusing to handle merge commits, while >> specifying '-m 1' fails on non-merge commits. >> >> This patch allows '-m 1' for non-merge commits. Besides, as mainline is >> always the only parent for a non-merge commit, it made little sense to >> disable it in the first place. > > In the original context to pick a single commit, it made perfect > sense to avoid mistakes by blindly passing '-m 1' to non-merge > commit. > > It may be fair to say that we failed to reconsider what to > do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair > history revisionism to say that it made little sense to disable it > in the first place. In fact I had zero intention on any revisionism. Now I see I should have said "makes little sense" in present tense to avoid touching deep historical issues, sorry! Something like: "This patch allows '-m 1' for non-merge commits. As mainline is always the only parent for a non-merge commit, it makes little sense to disable it." sounds OK? > > The change to the code itself looks sane, but applying this patch > alone will break existing tests whose expectations must be updated, > and this new behaviour must be protected by a new test (or two) so > that we won't accidentally stop accepting "-m 1" for a single-parent > commit. I fixed most of the tests, but "t3510/4: cherry-pick persists opts correctly" is an offender for me. It looks like it [ab]uses current "-m 1" behavior just to stop in the middle of the sequence, and I'm not sure how to fix it most suitably. Thanks! -- Sergey ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-12-13 6:35 ` Sergey Organov @ 2018-12-13 15:35 ` Sergey Organov 2018-12-14 2:36 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Sergey Organov @ 2018-12-13 15:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> [...] >> >> The change to the code itself looks sane, but applying this patch >> alone will break existing tests whose expectations must be updated, >> and this new behaviour must be protected by a new test (or two) so >> that we won't accidentally stop accepting "-m 1" for a single-parent >> commit. > > I fixed most of the tests, but > > "t3510/4: cherry-pick persists opts correctly" > > is an offender for me. It looks like it [ab]uses current "-m 1" behavior > just to stop in the middle of the sequence, and I'm not sure how to fix > it most suitably. I came up with the following as a preparatory change. Looks acceptable? -- 8< -- t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks We are going to allow 'git cherry-pick -m 1' for non-merge commits, so this method to force failure will stop to work. Use '-m 4' instead as it's very unlikely we will ever have such an octopus in this test setup. Modified t/t3510-cherry-pick-sequence.sh diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index c84eeef..a873cf4 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' ' test_expect_success 'cherry-pick persists opts correctly' ' pristine_detach initial && - test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick && + m=4 && + test_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick && test_path_is_dir .git/sequencer && test_path_is_file .git/sequencer/head && test_path_is_file .git/sequencer/todo && @@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' ' echo "true" >expect && git config --file=.git/sequencer/opts --get-all options.signoff >actual && test_cmp expect actual && - echo "1" >expect && + echo "$m" >expect && git config --file=.git/sequencer/opts --get-all options.mainline >actual && test_cmp expect actual && echo "recursive" >expect && -- 8< -- -- Sergey ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-12-13 15:35 ` Sergey Organov @ 2018-12-14 2:36 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2018-12-14 2:36 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > I came up with the following as a preparatory change. Looks acceptable? > > -- 8< -- > > t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks > > We are going to allow 'git cherry-pick -m 1' for non-merge commits, so > this method to force failure will stop to work. > > Use '-m 4' instead as it's very unlikely we will ever have such an > octopus in this test setup. Yeah, that is a good approach. Thanks for coming up with it. I agree that it also is a good idea to use a variable to avoid repeating "4" (and risking the two uses of the constant drifting apart), but I find a single letter variable 'm' a bit too bland and not descriptive enough. Perhaps spell it out as mainline=4, possibly with a comment why that is not a more-commonly-seen number like "1"? # to make sure that the session to cherry-pick a sequence # gets interrupted, use a high-enough number that is larger # than the number of parents of any commit mainline=4 && or something. > Modified t/t3510-cherry-pick-sequence.sh > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > index c84eeef..a873cf4 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' ' > > test_expect_success 'cherry-pick persists opts correctly' ' > pristine_detach initial && > - test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick && > + m=4 && > + test_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick && > test_path_is_dir .git/sequencer && > test_path_is_file .git/sequencer/head && > test_path_is_file .git/sequencer/todo && > @@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' ' > echo "true" >expect && > git config --file=.git/sequencer/opts --get-all options.signoff >actual && > test_cmp expect actual && > - echo "1" >expect && > + echo "$m" >expect && > git config --file=.git/sequencer/opts --get-all options.mainline >actual && > test_cmp expect actual && > echo "recursive" >expect && > > -- 8< -- ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2018-06-21 15:54 ` Junio C Hamano 2018-06-22 9:16 ` Sergey Organov 2018-12-12 5:35 ` Sergey Organov @ 2019-03-19 11:29 ` Sergey Organov 2019-03-20 0:38 ` Junio C Hamano 2 siblings, 1 reply; 18+ messages in thread From: Sergey Organov @ 2019-03-19 11:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi Junio, [It's been a while since this discussion, and recently I've got some thoughts and questions about "first-parent" issues in general, below.] Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> When cherry-picking multiple commits, it's impossible to have both >> merge- and non-merge commits on the same command-line. Not specifying >> '-m 1' results in cherry-pick refusing to handle merge commits, while >> specifying '-m 1' fails on non-merge commits. > > Allowing "-m1" even when picking a single parent commit, because > the 1-st parent is defined for such a commit, makes sense, espeially > when running a cherry-pick on a range, exactly for the above reason. > It is slightly less so when cherry-picking a single commit, but not > by a large margin. > > I think the original reasoning for requiring "-m $n" not present, > especially because cherry-pick was originally for replaying only a > single commit, was because it would lead somebody to propose that > the command should behave as if -m1 is always given (and when trying > to cherry-pick a merge relative to its second parent, give -m2 to > override it), which in turn encourage the 'first-parent is special' > world-view from the tool-side. IOW, "The worldview to treat the > first-parent chain specially is correct, because Git has many > features to work with that worldview conveniently" was something we > wanted to avoid; rather "Such and such workflows benefit from > treating the first-parent chain specially, so let's add features to > do so" was we wanted to do, and of course, back then cherry-pick > that allows mixture of merges and single-parent commits to be > picked, which would have made the need to do something like this > patch does felt greater, did not exist. > > Now, it appears, at least to me, that the world pretty much accepted > that the first-parent worldview is often very convenient and worth > supporting by the tool, so the next logical step might be to set > opts->mainline to 1 by default (and allow an explicit "-m $n" from > the command line to override it). But that should happen after this > patch lands---it is logically a separate step, I would think. I think that "first-parent is special" is the way to go indeed for porcelain, as it does make many thing easier and more convenient[*]. Is there a road-map already outlined in that direction, I wonder? OTOH, for plumbing, it's rather keeping the original pure-DAG "symmetrical merges" approach that seems to be the right thing to do. [*] One example that immediately comes to mind is "git log -p" for a merge commit. I doesn't currently (as of v2.10) show the first-parent diff, for whatever reason. "git log -p -m --first-parent" is needed to get the answer to most "obvious" question: what (merge) commit did to my mainline? "git show" has its own issues. -- Sergey ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-19 11:29 ` Sergey Organov @ 2019-03-20 0:38 ` Junio C Hamano 2019-03-20 5:09 ` Jeff King 2019-03-25 6:43 ` Sergey Organov 0 siblings, 2 replies; 18+ messages in thread From: Junio C Hamano @ 2019-03-20 0:38 UTC (permalink / raw) To: Sergey Organov; +Cc: git Sergey Organov <sorganov@gmail.com> writes: > I think that "first-parent is special" is the way to go indeed for > porcelain, as it does make many thing easier and more convenient[*]. Perhaps. However ... > [*] One example that immediately comes to mind is "git log -p" for a > merge commit. I doesn't currently (as of v2.10) show the first-parent > diff, for whatever reason. "git log -p -m --first-parent" is needed to > get the answer to most "obvious" question: what (merge) commit did to my > mainline? "git show" has its own issues. ... this is very much deliberate and will remain so. A single ball of wax "diff M^ M" for a merge commit is not always what you would want, especially while viewing "git log -p" (without "--first-parent"). The reason why the user does not explicitly say "--first-parent" is because the user wants to even see the details of individual steps of what happened on side branches. Incidentally, in such an "I am interested in what happened in each individual step" mode, the primary change that a merge by itself does is better shown with "diff --cc M^ M", not "diff -p M^ M". That is why "show" defaults to "--cc". "git log -p --first-parent" that requires "-m" to show the single ball of wax diff for a merge is a separate matter. When the user explicitly says "log --first-parent", it is a clear indication that the user does not want to see individual steps of how side branches built what each merge brings into the mainline. From that point of view, ever sice I introduced "--first-parent" traversal, I've been wondering what the true downside would be if we turned "-m" on automatically when these two options are used without "-m". But it has not disturbed me deeply enough to bother looking into it further. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-20 0:38 ` Junio C Hamano @ 2019-03-20 5:09 ` Jeff King 2019-03-25 6:43 ` Sergey Organov 1 sibling, 0 replies; 18+ messages in thread From: Jeff King @ 2019-03-20 5:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sergey Organov, git On Wed, Mar 20, 2019 at 09:38:57AM +0900, Junio C Hamano wrote: > "git log -p --first-parent" that requires "-m" to show the single > ball of wax diff for a merge is a separate matter. When the user > explicitly says "log --first-parent", it is a clear indication that > the user does not want to see individual steps of how side branches > built what each merge brings into the mainline. From that point of > view, ever sice I introduced "--first-parent" traversal, I've been > wondering what the true downside would be if we turned "-m" on > automatically when these two options are used without "-m". Sort of a drive-by two cents, but I have often wondered the same thing. I cannot think of a time when I wanted "--first-parent" without "-m" (unless I was not viewing diffs at all). (And I agree with everything else you said :) ). -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-20 0:38 ` Junio C Hamano 2019-03-20 5:09 ` Jeff King @ 2019-03-25 6:43 ` Sergey Organov 2019-03-26 16:32 ` Jeff King 1 sibling, 1 reply; 18+ messages in thread From: Sergey Organov @ 2019-03-25 6:43 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I think that "first-parent is special" is the way to go indeed for >> porcelain, as it does make many thing easier and more convenient[*]. > > Perhaps. However ... > >> [*] One example that immediately comes to mind is "git log -p" for a >> merge commit. I doesn't currently (as of v2.10) show the first-parent >> diff, for whatever reason. "git log -p -m --first-parent" is needed to >> get the answer to most "obvious" question: what (merge) commit did to my >> mainline? "git show" has its own issues. > > ... this is very much deliberate and will remain so. > A single ball of wax "diff M^ M" for a merge commit is not always > what you would want, especially while viewing "git log -p" (without > "--first-parent"). OK, point taken. Then it's an issue of suppressing (presumably huge) parts of output for merge commits by default, and is only vaguely relevant to the "first parent is special" trend that I intended to discuss. So, let's leave in peace the "git log -p" for now, and let me try it from different angle. How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect. Also, is current output of "git log -m", being extremely confusing, suitable for anything? Maybe consider to change it to output diff with respect to the first parent only? Though it's then a pity "-m" lacks argument here, similar to what it has in cherry-pick. -- Sergey ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-25 6:43 ` Sergey Organov @ 2019-03-26 16:32 ` Jeff King 2019-03-26 22:07 ` Elijah Newren 2019-03-27 13:54 ` Sergey Organov 0 siblings, 2 replies; 18+ messages in thread From: Jeff King @ 2019-03-26 16:32 UTC (permalink / raw) To: Sergey Organov; +Cc: Junio C Hamano, git On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote: > How about changing "git show -p M" to output "diff -p M^ M" rather than > "diff-tree --cc M" for merge commits? It's really surprising specifying > -p has no visible effect. That's because "-p" is already the default, and the format selection is orthogonal to the handling of merge commits. Providing "-m" would actually override the "--cc" default (though "--first-parent -m" is likely to be less noisy, per this discussion). As far as defaults go, I dunno. The idea is that "--cc" would give you a nice summary of what the merge _itself_ had to touch. I think that's valuable, too. If we were starting from scratch, I think there could be a discussion about whether one default is better than the other. But at this point I have a hard time finding one so much obviously better than the other to merit changing the behavior. > Also, is current output of "git log -m", being extremely confusing, > suitable for anything? Maybe consider to change it to output diff with > respect to the first parent only? Though it's then a pity "-m" lacks > argument here, similar to what it has in cherry-pick. I've used "-m" without "--first-parent" sometimes in order to track down mis-merges manually. It's not usually a big deal to say "--first-parent" if that's what you want. But one thing I don't think is currently possible is to ask only for the first-parent diff, but _not_ restrict the actual traversal. If that's what you mean by giving an argument to "-m", then yeah, I think that would be a useful addition. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-26 16:32 ` Jeff King @ 2019-03-26 22:07 ` Elijah Newren 2019-03-26 22:20 ` Jeff King 2019-03-27 13:54 ` Sergey Organov 1 sibling, 1 reply; 18+ messages in thread From: Elijah Newren @ 2019-03-26 22:07 UTC (permalink / raw) To: Jeff King; +Cc: Sergey Organov, Junio C Hamano, Git Mailing List On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote: > > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote: > > > How about changing "git show -p M" to output "diff -p M^ M" rather than > > "diff-tree --cc M" for merge commits? It's really surprising specifying > > -p has no visible effect. > > That's because "-p" is already the default, and the format selection is > orthogonal to the handling of merge commits. Providing "-m" would > actually override the "--cc" default (though "--first-parent -m" is > likely to be less noisy, per this discussion). > > As far as defaults go, I dunno. The idea is that "--cc" would give you a > nice summary of what the merge _itself_ had to touch. I think that's > valuable, too. If we were starting from scratch, I think there could be > a discussion about whether one default is better than the other. But at > this point I have a hard time finding one so much obviously better than > the other to merit changing the behavior. Indeed, some of us would view a first parent diff default for merges as problematic. However, I'd like to point out (or remind) that these two options aren't the only ways you could view a merge. Thomas Rast's --remerge-diff[1] is another (even if not yet part of git.git). Gerrit uses something similar-ish for its default way of showing a merge. [1] See e.g. https://bugs.chromium.org/p/git/issues/detail?id=12 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-26 22:07 ` Elijah Newren @ 2019-03-26 22:20 ` Jeff King 2019-03-27 0:33 ` Elijah Newren 0 siblings, 1 reply; 18+ messages in thread From: Jeff King @ 2019-03-26 22:20 UTC (permalink / raw) To: Elijah Newren; +Cc: Sergey Organov, Junio C Hamano, Git Mailing List On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote: > On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote: > > > > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote: > > > > > How about changing "git show -p M" to output "diff -p M^ M" rather than > > > "diff-tree --cc M" for merge commits? It's really surprising specifying > > > -p has no visible effect. > > > > That's because "-p" is already the default, and the format selection is > > orthogonal to the handling of merge commits. Providing "-m" would > > actually override the "--cc" default (though "--first-parent -m" is > > likely to be less noisy, per this discussion). > > > > As far as defaults go, I dunno. The idea is that "--cc" would give you a > > nice summary of what the merge _itself_ had to touch. I think that's > > valuable, too. If we were starting from scratch, I think there could be > > a discussion about whether one default is better than the other. But at > > this point I have a hard time finding one so much obviously better than > > the other to merit changing the behavior. > > Indeed, some of us would view a first parent diff default for merges > as problematic. However, I'd like to point out (or remind) that these > two options aren't the only ways you could view a merge. Thomas > Rast's --remerge-diff[1] is another (even if not yet part of git.git). > Gerrit uses something similar-ish for its default way of showing a > merge. Heh, I almost mentioned remerge-diff, but since it's not actually part of Git, I didn't want to get into a tangent. But since you mention it, yes, I actually find it quite a useful way of looking at the diff, especially when I want to see what the person resolving the conflicts actually _did_. The --cc combined diff is too eager to throw away hunks that resolved purely to one side (which _most_ of the time is what you want, but when you're hunting a possible error in the merge, it's quite confusing). How close is merge-recursive.c to actually doing a pure in-memory merge? I seem to recall that was a (the?) sticking point for the original remerge-diff. -Peff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-26 22:20 ` Jeff King @ 2019-03-27 0:33 ` Elijah Newren 0 siblings, 0 replies; 18+ messages in thread From: Elijah Newren @ 2019-03-27 0:33 UTC (permalink / raw) To: Jeff King; +Cc: Sergey Organov, Junio C Hamano, Git Mailing List On Tue, Mar 26, 2019 at 3:20 PM Jeff King <peff@peff.net> wrote: > > On Tue, Mar 26, 2019 at 03:07:42PM -0700, Elijah Newren wrote: > > > On Tue, Mar 26, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote: > > > > > > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote: > > > > > > > How about changing "git show -p M" to output "diff -p M^ M" rather than > > > > "diff-tree --cc M" for merge commits? It's really surprising specifying > > > > -p has no visible effect. > > > > > > That's because "-p" is already the default, and the format selection is > > > orthogonal to the handling of merge commits. Providing "-m" would > > > actually override the "--cc" default (though "--first-parent -m" is > > > likely to be less noisy, per this discussion). > > > > > > As far as defaults go, I dunno. The idea is that "--cc" would give you a > > > nice summary of what the merge _itself_ had to touch. I think that's > > > valuable, too. If we were starting from scratch, I think there could be > > > a discussion about whether one default is better than the other. But at > > > this point I have a hard time finding one so much obviously better than > > > the other to merit changing the behavior. > > > > Indeed, some of us would view a first parent diff default for merges > > as problematic. However, I'd like to point out (or remind) that these > > two options aren't the only ways you could view a merge. Thomas > > Rast's --remerge-diff[1] is another (even if not yet part of git.git). > > Gerrit uses something similar-ish for its default way of showing a > > merge. > > Heh, I almost mentioned remerge-diff, but since it's not actually part > of Git, I didn't want to get into a tangent. But since you mention it, > yes, I actually find it quite a useful way of looking at the diff, > especially when I want to see what the person resolving the conflicts > actually _did_. The --cc combined diff is too eager to throw away hunks > that resolved purely to one side (which _most_ of the time is what you > want, but when you're hunting a possible error in the merge, it's quite > confusing). > > How close is merge-recursive.c to actually doing a pure in-memory merge? > > I seem to recall that was a (the?) sticking point for the original > remerge-diff. Doing a pure in-memory merge is tied up with my overall merge-recursive rewrite, but I haven't touched merge stuff in quite a while now other than the recent make-directory-rename-detection-be-a-conflict-by-default stuff. I'm hoping I can finish that soon (though I've struggled a bit to find the time to do so), and finish out the filter-repo stuff, then I plan to get back to merge stuff again. A pure in-memory merge is near the top of my priorities for the rewrite, so we'll get it...eventually. (Maybe a Christmas present?) I do think there's more than just that sticking point for the remerge-diff, but the other things are on my radar too (a few slots later on my todo list). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified 2019-03-26 16:32 ` Jeff King 2019-03-26 22:07 ` Elijah Newren @ 2019-03-27 13:54 ` Sergey Organov 1 sibling, 0 replies; 18+ messages in thread From: Sergey Organov @ 2019-03-27 13:54 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git Jeff King <peff@peff.net> writes: > On Mon, Mar 25, 2019 at 09:43:09AM +0300, Sergey Organov wrote: > >> How about changing "git show -p M" to output "diff -p M^ M" rather than >> "diff-tree --cc M" for merge commits? It's really surprising specifying >> -p has no visible effect. > > That's because "-p" is already the default, and the format selection is > orthogonal to the handling of merge commits. Seems to be more convoluted than that. If "-p" were simply default, then "git show --raw" and "git show -p --raw" would output the same thing. They don't. That said, does "-p" select a format, or not? Should it? For me, "--patch" sounds specific enough to expect it to select the format that is most close to what "patch" utility is able to apply, that would still be "diff -p M^ M" format, universally, be it merge commit or not. > Providing "-m" would actually override the "--cc" default (though > "--first-parent -m" is likely to be less noisy, per this discussion). Right, but "--first-parent" (while accepted) even is not documented for "git show", and it could be argued if history traversal option has any sense for a command that shows separate commits. Further, even in "git log", having the "--first-parent" change the way diffs are output is, strictly speaking, violation of orthogonality (that nobody seems to care about). > As far as defaults go, I dunno. I'm not after changing the default behavior. I'm rather after making the whole system of options somewhat more logical, self consistent, and thus less confusing. > The idea is that "--cc" would give you a nice summary of what the > merge _itself_ had to touch. I think that's valuable, too. I'm not against "--cc" either, be it a default or not, even though personally for me it's not very useful, as it shows how merge (the operation) supposedly went, when I'm usually interested in how merge (the resulting commit) affects the mainline, no matter how this result has been achieved. Another thought about "--cc" is that it's in fact a case of "merges are symmetric" approach to the UI that is supposedly shifting to "mainline is special" one. > If we were starting from scratch, I think there could be > a discussion about whether one default is better than the other. But at > this point I have a hard time finding one so much obviously better than > the other to merit changing the behavior. I'm yet to figure what exactly the best set of options would be, even personally for me, even in the "start from scratch" case, so, for now, I'm basically just gathering relevant information and opinions. >> Also, is current output of "git log -m", being extremely confusing, >> suitable for anything? Maybe consider to change it to output diff with >> respect to the first parent only? Though it's then a pity "-m" lacks >> argument here, similar to what it has in cherry-pick. > > I've used "-m" without "--first-parent" sometimes in order to track down > mis-merges manually. Have you been interested specifically in diffs with respect to side branches in these cases, I wonder, or did you actually look only at "-m 1" part of the whole "-m" output? When I tried "-m", I found it rather difficult to even visually find the margin between diffs to different parents, that confused me and forced to resort to other methods. Besides, it didn't appear to me at that time that there is "--first-parent" that might "help", so as I recall I ended up using "diff -p M~1 M" for the merge in question. > It's not usually a big deal to say "--first-parent" if that's what you > want. But one thing I don't think is currently possible is to ask only > for the first-parent diff, but _not_ restrict the actual traversal. That's due to "--first-parent" breaking orthogonality. It should rather only affect graph traversal, I'd expect. Admittedly, it may imply some other option(s) for convenience (say, such option could have been "-m 1", if it existed), but then there /must/ exist the option(s) it implies in the first place. Currently, "--first-parent" behaves as if it implies some nonexistent option, so the user has no way indeed to provide the latter without the former. > If that's what you mean by giving an argument to "-m", then yeah, I > think that would be a useful addition. I thought that maybe the part of "-m" that outputs diffs to side branch(es) is in fact useless feature when result is to be directly consumed by human beings. Then, if we decide to change it to output diff to single parent for porcelain command(s), it may be useful to be able to explicitly ask for other parents than the default, the first one. It also strikes me as inconsistent that "-m" in log/show on one hand, and "-m" in cherry-pick on the other, being essentially the same thing, are so different in appearance and description. Unfortunately, adding an argument to "-m" bumps either into generic evilness of optional arguments for options, or into backward incompatibility (if the argument to "-m" becomes mandatory), so it doesn't seem to be such a good thing to do after all. -- Sergey ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-03-27 13:54 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-25 12:42 [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov -- strict thread matches above, loose matches on Subject: below -- 2018-05-25 12:42 Sergey Organov 2018-06-21 15:54 ` Junio C Hamano 2018-06-22 9:16 ` Sergey Organov 2018-12-12 5:35 ` Sergey Organov 2018-12-13 4:20 ` Junio C Hamano 2018-12-13 6:35 ` Sergey Organov 2018-12-13 15:35 ` Sergey Organov 2018-12-14 2:36 ` Junio C Hamano 2019-03-19 11:29 ` Sergey Organov 2019-03-20 0:38 ` Junio C Hamano 2019-03-20 5:09 ` Jeff King 2019-03-25 6:43 ` Sergey Organov 2019-03-26 16:32 ` Jeff King 2019-03-26 22:07 ` Elijah Newren 2019-03-26 22:20 ` Jeff King 2019-03-27 0:33 ` Elijah Newren 2019-03-27 13:54 ` Sergey Organov
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).