| Message ID | 20230909125446.142715-3-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | diff-merges: introduce '-d' option | expand |
Sergey Organov <sorganov@gmail.com> writes: > This option provides a shortcut to request diff with respect to first > parent for any kind of commit, universally. It's implemented as pure > synonym for "--diff-merges=first-parent --patch". > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- Sounds very straight-forward. Given that "--first-parent" in "git log --first-parent -p" already defeats "-m" and shows the diff against the first parent only, people may find it confusing if "git log -d" does not act as a shorthand for that. From the above and also from the documentation update, it is hard to tell if that is what you implemented, or it only affects the "diff-merges" part. Other than that, the patch looks quite small and to the point. Thanks. > Documentation/diff-options.txt | 4 ++++ > Documentation/git-log.txt | 2 +- > diff-merges.c | 3 +++ > t/t4013-diff-various.sh | 8 ++++++++ > 4 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index f93aa3e46a52..d773dafcb10a 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -51,6 +51,10 @@ ifdef::git-log[] > Note: This option not implying `-p` is legacy feature that is > preserved for the sake of backward compatibility. > > +-d:: > + Produce diff with respect to first parent. > + Shortcut for '--diff-merges=first-parent -p'. > + > -c:: > Produce combined diff output for merge commits. > Shortcut for '--diff-merges=combined -p'. > diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt > index 9b7ec96e767a..59bd74a1a596 100644 > --- a/Documentation/git-log.txt > +++ b/Documentation/git-log.txt > @@ -120,7 +120,7 @@ By default, `git log` does not generate any diff output. The options > below can be used to show the changes made by each commit. > > Note that unless one of `--diff-merges` variants (including short > -`-m`, `-c`, and `--cc` options) is explicitly given, merge commits > +`-d`, `-m`, `-c`, and `--cc` options) is explicitly given, merge commits > will not show a diff, even if a diff format like `--patch` is > selected, nor will they match search options like `-S`. The exception > is when `--first-parent` is in use, in which case `first-parent` is > diff --git a/diff-merges.c b/diff-merges.c > index ec97616db1df..6eb72e6fc28a 100644 > --- a/diff-merges.c > +++ b/diff-merges.c > @@ -125,6 +125,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) > if (!suppress_m_parsing && !strcmp(arg, "-m")) { > set_to_default(revs); > revs->merges_need_diff = 0; > + } else if (!strcmp(arg, "-d")) { > + set_first_parent(revs); > + revs->merges_imply_patch = 1; > } else if (!strcmp(arg, "-c")) { > set_combined(revs); > revs->merges_imply_patch = 1; > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 5de1d190759f..a07d6eb6dd97 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -473,6 +473,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' ' > test_cmp expected actual > ' > > +test_expect_success 'log -d matches --diff-merges=1 -p' ' > + git log --diff-merges=1 -p master >result && > + process_diffs result >expected && > + git log -d master >result && > + process_diffs result >actual && > + test_cmp expected actual > +' > + > test_expect_success 'deny wrong log.diffMerges config' ' > test_config log.diffMerges wrong-value && > test_expect_code 128 git log
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> This option provides a shortcut to request diff with respect to first >> parent for any kind of commit, universally. It's implemented as pure >> synonym for "--diff-merges=first-parent --patch". >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- > > Sounds very straight-forward. > > Given that "--first-parent" in "git log --first-parent -p" already > defeats "-m" and shows the diff against the first parent only, > people may find it confusing if "git log -d" does not act as a > shorthand for that. It doesn't, and I believe it's a good thing, as primary function of --first-parent is to change history traversal rules, and if -d did that, it would be extremely confusing. Also, --first-parent is correctly documented as implying --diff-merges=first-parent, not as defeating -m. > From the above and also from the documentation update, it is hard to > tell if that is what you implemented, or it only affects the > "diff-merges" part. If we read resulting documentation with a fresh eye, -d is similar to --cc, and -c, just producing yet another kind of output, so I think all this fits together quite nicely and shouldn't cause confusion. > > Other than that, the patch looks quite small and to the point. Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: >> Sounds very straight-forward. >> >> Given that "--first-parent" in "git log --first-parent -p" already >> defeats "-m" and shows the diff against the first parent only, >> people may find it confusing if "git log -d" does not act as a >> shorthand for that. > > It doesn't, and I believe it's a good thing, as primary function of > --first-parent is to change history traversal rules, and if -d did that, > it would be extremely confusing. I am not sure about that. > Also, --first-parent is correctly documented as implying > --diff-merges=first-parent, not as defeating -m. Yes, exactly. That makes me even more convinced that the intuitive behaviour, when we say "we have this great short-hand option that lets your 'git log' to do the first-parent thing with patch output", is to do the first-parent traversal _and_ show first-parent patches. "-d" is documented as a short-hand for "--diff-merges=first-parent --patch" and not for "--first-parent --patch", so the behaviour may correctly match documentation, but that does not make the documented behaviour an intuitive one. And a behaviour that is not intuitive is confusing. > If we read resulting documentation with a fresh eye, -d is similar to > --cc, and -c, just producing yet another kind of output, so I think all > this fits together quite nicely and shouldn't cause confusion. Another thing is that showing first-parent patch for merges while letting the traversal also visit the second-parent chain is not as useful an option as it could be, even though it is not so bad as the original "-m -p" that also showed second-parent patch for merges as well. People would have to say "log --first-parent -p" to get the first-parent traversal with first-parent patch output, and they would not behefit from having "-d".
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >>> Sounds very straight-forward. >>> >>> Given that "--first-parent" in "git log --first-parent -p" already >>> defeats "-m" and shows the diff against the first parent only, >>> people may find it confusing if "git log -d" does not act as a >>> shorthand for that. >> >> It doesn't, and I believe it's a good thing, as primary function of >> --first-parent is to change history traversal rules, and if -d did that, >> it would be extremely confusing. > > I am not sure about that. > >> Also, --first-parent is correctly documented as implying >> --diff-merges=first-parent, not as defeating -m. > > Yes, exactly. That makes me even more convinced that the intuitive > behaviour, when we say "we have this great short-hand option that > lets your 'git log' to do the first-parent thing with patch output", > is to do the first-parent traversal _and_ show first-parent patches. > > "-d" is documented as a short-hand for "--diff-merges=first-parent > --patch" and not for "--first-parent --patch", so the behaviour may > correctly match documentation, but that does not make the documented > behaviour an intuitive one. And a behaviour that is not intuitive > is confusing. I think both behaviors make sense, provided they are correctly documented. I just prefer the one that is more basic, yet allows to achieve things that another one does not. > >> If we read resulting documentation with a fresh eye, -d is similar to >> --cc, and -c, just producing yet another kind of output, so I think all >> this fits together quite nicely and shouldn't cause confusion. > > Another thing is that showing first-parent patch for merges while > letting the traversal also visit the second-parent chain is not as > useful an option as it could be, even though it is not so bad as the > original "-m -p" that also showed second-parent patch for merges as > well. I don't see why desire to look at diff-to-first-parent on "side" branches is any different from desire to look at them on "primary" branch, sorry, so I still don't want "-d" to affect traversal or other commit filtering rules. We do have --first-parent as well as a few others for that. > People would have to say "log --first-parent -p" to get the > first-parent traversal with first-parent patch output, and they > would not behefit from having "-d". Well, at least they can now say "log --first-parent -d" as well ;) Honestly, the "log --first-parent -p" (without "-m") suddenly producing diffs for merge commits is already unnatural, needs yet another special-casing in documentation, and then, finally, this relatively new behavior was introduced exactly because there were no "-d" at that time, to save typing "-m". The latter is yet another example of why "-d" in its current form is a good idea. That said, if you feel like there is place for a short-cut for this particular use-case, it'd be fine with me, say: --fpd: short-cut for "--first-parent -d" would fit quite nicely into the picture, I think. Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: > I don't see why desire to look at diff-to-first-parent on "side" > branches is any different from desire to look at them on "primary" > branch Yeah, but that is not what I meant. The above argues for why "--diff-merges=first-parent" should exist independently from the "--first-parent" traversal *and* display option. I am not saying it should not exist. But I view that the desire to look at any commits and its changes on the "side" branch at all *is* at odds with the wish to look at first-parent change for merge commits. Once you decide to look at first-parent change for a merge commit, then every change you see for each commit on the "side" branch, whether it is shown as first-parent diff or N pairwise diffs, is what you have already seen in the change in the merge commit, because "git log" goes newer to older, and the commits on the side branches appear after the merge that brings them to the mainline. Making "log -d" mean "log --diff-merges=first-parent --patch" lets that less useful combination ("show first-parent patches but traverse side branches as well") squat on the short and sweet "-d" that could be used for more useful "log --first-parent --patch", which would also be more common and intuitive to users, and that is what I suspect will become problematic in the longer run. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> I don't see why desire to look at diff-to-first-parent on "side" >> branches is any different from desire to look at them on "primary" >> branch > > Yeah, but that is not what I meant. The above argues for why > "--diff-merges=first-parent" should exist independently from the > "--first-parent" traversal *and* display option. I am not saying > it should not exist. I was not assuming you were saying this, as it has been discussed and agreed upon when --diff-merges=first-parent was introduced, though I think I now see your point more clearly. > > But I view that the desire to look at any commits and its changes on > the "side" branch at all *is* at odds with the wish to look at > first-parent change for merge commits. I think I do now understand what you mean, yet I have alternative view on the issue. > Once you decide to look at first-parent change for a merge commit, > then every change you see for each commit on the "side" branch, > whether it is shown as first-parent diff or N pairwise diffs, is what > you have already seen in the change in the merge commit, Actually, this happens to be exactly one of intended use-cases for "-d". It's useful to see how some change introduced by the merge looked in the context of the original commit, or to figure where the change came from. > because "git log" goes newer to older, and the commits on the side > branches appear after the merge that brings them to the mainline. The exact order is orthogonal to the issue at hands, I think. > Making "log -d" mean "log --diff-merges=first-parent --patch" lets > that less useful combination ("show first-parent patches but > traverse side branches as well") squat on the short and sweet "-d" > that could be used for more useful "log --first-parent --patch", > which would also be more common and intuitive to users, and that is > what I suspect will become problematic in the longer run. Sorry, "-d ≡ --first-parent --patch" you suggest contradicts my view on the whole scheme of things, for several reasons: * I still find it problematic if -d, intended to fit nicely among --cc, -c, -d, -m, -p, --remerge-diff options, suddenly implies --first-parent. This would bring yet another inconsistency, and I don't want to be the one who introduced it. * In its current state -d conveniently means: "gimme simple diff output for everything", where --first-parent you suggest doesn't fit at all. * Current -d implementation is semantically as close to -p as possible, tweaking exactly one thing compared to -p: the format of output for merge commits, so is simpler than what you suggest from all angles, as --first-parent tweaks more than one thing. * To me what you argue for looks mostly like a desire to have a short-cut for "--first-parent --patch", and my patch in question does not seem to contradict this desire, as it'd be very surprising if somebody came up with the name "-d" for such a short-cut. Definitely not me. * Finally, if -d becomes "--patch --first-parent", how do I get back useful "--patch --diff-merges=first-parent" part of it, provided --first-parent is unreversable? And even if it were reversable, then git log -d --no-first-parent = git log --patch --first-parent --no-first-parent = git log --patch is definitely not what is needed, nor frequent demand to revert implied things indicates optimal design. Compare this to git log -d --first-parent that current -d provides for you to get what you need, and that unambiguously reads: "gimme *d*iff for all commits while following *first parent* through the history" (while, unlike, -p not requiring --first-parent to implicitly tweak diff for merges output). Overall, after considering your concern, I'd still prefer to leave "-d" semantics as implemented, consistent with the rest of similar options, and let somebody else define more shortcuts for their frequent use-cases if they feel like it. Thanks, -- Sergey Organov P.S. I also figure that maybe our divergence comes from the fact that I consider merge commits to be primarily commits (introducing particular set of changes, and then having reference to the source of the changes), whereas you consider them primarily merges (joining two histories, and then maybe some artificial changes that make merges "evil"). That's why we often end up agreeing to disagree, as both these points of view seem pretty valid.
Sergey Organov <sorganov@gmail.com> writes: > P.S. I also figure that maybe our divergence comes from the fact that I > consider merge commits to be primarily commits (introducing particular > set of changes, and then having reference to the source of the changes), > whereas you consider them primarily merges (joining two histories, and > then maybe some artificial changes that make merges "evil"). That's why > we often end up agreeing to disagree, as both these points of view seem > pretty valid. It rarely is the case that two opposing world views are equally valid, though. If there were an option that forbids any comparison output from a single parent commit (say --ndfnm "no-diff-for-non-merge"), then those with "merges are the primary thing, single-parent commits on the merged branches are implementation details" worldview would be commonly using "--diff-merges=first-parent --patch --ndfnm" and (1) viewing only the combined effect of merging side branches without seeing noise from individual commits whose effects are already shown in these merges, and (2) traversing the side branches as well, so that merges from side-side branches into the side branches are viewable the same way as merges into the mainline. But because no such option exists and nobody asked for such an option during the whole lifetime of the project, I highly doubt that it is a valid world view with wide backing from the users. Even if it were a valid world view with wide backing, the combination "--diff-merges=first-parent --patch" would be less than ideal presentation for them (due to lack of "--ndfnm"). And as I already said, it would not be useful without --first-parent traversal for the worldview git has supported. That is why I find it of dubious value to let short-and-sweet '-d' be squatted by less than ideal "--diff-merges=first-parent --patch" combination. Shorthands are scarse resources, and we want to be careful before handing them out. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> P.S. I also figure that maybe our divergence comes from the fact that I >> consider merge commits to be primarily commits (introducing particular >> set of changes, and then having reference to the source of the changes), >> whereas you consider them primarily merges (joining two histories, and >> then maybe some artificial changes that make merges "evil"). That's why >> we often end up agreeing to disagree, as both these points of view seem >> pretty valid. > > It rarely is the case that two opposing world views are equally > valid, though. Yes. In this particular case the two are not opposing though, rather orthogonal, as they reflect the intrinsic dualism of the concept of "merge commit". Merge commit is both a new state, and history junction, neither of which is more or less valid or essential, and I use both views myself, depending on situation. An electron is both a particle and a wave, and one just uses its side that is more convenient for explanation of the case in hand. I promote features that I routinely need in my workflows, yet respecting the other side of the coin as well, even though I may rarely find this other side useful. I mean, for me, this -c/--cc (let alone -m) output is only confusing, yet I won't be saying that it's somehow less valid than proposed -d. > If there were an option that forbids any comparison output from a > single parent commit (say --ndfnm "no-diff-for-non-merge"), > then those with "merges are the primary thing, single-parent commits > on the merged branches are implementation details" worldview would be > commonly using "--diff-merges=first-parent --patch --ndfnm" and (1) > viewing only the combined effect of merging side branches without > seeing noise from individual commits whose effects are already shown > in these merges, and (2) traversing the side branches as well, so that > merges from side-side branches into the side branches are viewable the > same way as merges into the mainline. No need to ask for a new option, as the behavior you describe is already there, and is spelled "git log --diff-merges=first-parent" (--diff-merges=1 for short). > But because no such option exists and nobody asked for such an > option during the whole lifetime of the project, I highly doubt > that it is a valid world view with wide backing from the users. Your concern above seems to be void, so this doesn't hold either. As a side-note though, something like this has been asked recently, as what you describe by --ndfnm should in fact have been what --no-patch does, but surprisingly does not (please recall recent discussion of this issue). > Even if it were a valid world view with wide backing, Apparently it is valid, otherwise there would be no need for diff to first parent at all, let alone "git log --first-parent -p" have used it by default. > the combination "--diff-merges=first-parent --patch" would be less > than ideal presentation for them (due to lack of "--ndfnm"). First, as we figured above, --ndfnm is not needed, and second, me, being one of "them", tries hard to convince you it is the best presentation "them" can get, while "ideal" simply never exists. > And as I already said, it would not be useful without --first-parent > traversal for the worldview git has supported. Yes, you said it earlier in this thread, and as well I already explained how it is useful without --first-parent. > That is why I find it of dubious value to let short-and-sweet '-d' > be squatted by less than ideal "--diff-merges=first-parent --patch" > combination. Hopefully I do understand your concerns, yet I believe "--diff-merges=first-parent --patch" is way better for "-d" shortcut than "--first-parent --patch", for the reasons I already explained earlier in this thread. > Shorthands are scarse resources, and we want to be careful before > handing them out. Yep, agreed. I believe I carefully thought it over though, weighing all pros and cons, and thus -d fits nicely among -c and --cc, being yet another frequently desired format for merges, plays nice with -p as well, and is very mnemonic, giving us convenient, user-friendly, and consistent user interface overall. Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: > No need to ask for a new option, as the behavior you describe is already > there, and is spelled "git log --diff-merges=first-parent" > (--diff-merges=1 for short). Ah, that changes things. Making "--diff-merges=<how>" only about the presentation of merge commits, requiring a separate "-p" for single-parent commits [*], does make the life for those in the "merges are the only interesting things" camp a lot easier, exactly because the lack of "-p" can be used to say "I am not interested in chanages by single-parent commits". Side note: I personally think it is a design mistake of --diff-merges=<how> (e.g., --cc and --diff-merges=cc do not behave the same way) but that is a different story, and it is way too late now anyway to "fix" or change. So "-d" that stands for "--diff-merges=first-parent -p" makes the more useful (to those who think "merges are the only interesting things", which I do not belong to) "--diff-merges=first-parent" (without "-p") less useful. And the combination is not useful for those of us who find individual patches plus tweaks by merges (either --cc or --remerge-diff) are the way to look at the history. I still do not think that we want to give a short-and-sweet single letter option for such a combination. Thanks for clarification.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> No need to ask for a new option, as the behavior you describe is already >> there, and is spelled "git log --diff-merges=first-parent" >> (--diff-merges=1 for short). > > Ah, that changes things. Only a tiny bit, unfortunately, as I'm still struggling to finally convince you ((( > > Making "--diff-merges=<how>" only about the presentation of merge > commits, requiring a separate "-p" for single-parent commits [*], > does make the life for those in the "merges are the only interesting > things" camp a lot easier, exactly because the lack of "-p" can be > used to say "I am not interested in chanages by single-parent > commits". > > Side note: I personally think it is a design mistake of > --diff-merges=<how> (e.g., --cc and --diff-merges=cc do not > behave the same way) but that is a different story, and it > is way too late now anyway to "fix" or change. Side note: This has been considered and agreed upon when --diff-merges= options were introduced, and as far as I recall, at that time you explicitly agreed it might be useful to be able to get output only for merge commits. --cc is a simple alias for "--diff-merges=cc --patch" nowadays, so yes, they do behave differently, and that's by design. Dunno see any design mistake here, as we get all useful variations of behavior with a straightforward design, more frequent use-cases served by shorter options. Looks fine. > > So "-d" that stands for "--diff-merges=first-parent -p" makes the > more useful (to those who think "merges are the only interesting > things", which I do not belong to) "--diff-merges=first-parent" > (without "-p") less useful. And the combination is not useful for > those of us who find individual patches plus tweaks by merges > (either --cc or --remerge-diff) are the way to look at the history. Yes, you have your --cc, -c, and --remerge-diff (that I'd call something like --rd probably, but anyway). Could I please have my simple, straightforward, mnemonic, and terribly useful "-d" as well? In other words, will I finally be faced with "if you need it, do it yourself" argument? ;) > I still do not think that we want to give a short-and-sweet single > letter option for such a combination. I have very simple desire: convenient way to tell Git to show me diff to the first parent for merge commits, as that's the thing I need 99% of times when I do request diff output at all. That's exactly what I'd have seen as changes when I was about to commit the merge as well, similar to any other commit. It's so natural that I can't figure why it looks so damn rare or unusual to you, and that it makes you argue so hard against -d, especially when -p, -c, --cc, or even -m, are already there? I do sympathize your desire to be careful about short options, but what reservation for "-d" do you still have in mind? It seems that it was just waiting for me to come and finally bring it to life with the best meaning possible. How long should I wait for it to remain unused to finally be able to make use of it? Thanks, -- Sergey Organov
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index f93aa3e46a52..d773dafcb10a 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -51,6 +51,10 @@ ifdef::git-log[] Note: This option not implying `-p` is legacy feature that is preserved for the sake of backward compatibility. +-d:: + Produce diff with respect to first parent. + Shortcut for '--diff-merges=first-parent -p'. + -c:: Produce combined diff output for merge commits. Shortcut for '--diff-merges=combined -p'. diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt index 9b7ec96e767a..59bd74a1a596 100644 --- a/Documentation/git-log.txt +++ b/Documentation/git-log.txt @@ -120,7 +120,7 @@ By default, `git log` does not generate any diff output. The options below can be used to show the changes made by each commit. Note that unless one of `--diff-merges` variants (including short -`-m`, `-c`, and `--cc` options) is explicitly given, merge commits +`-d`, `-m`, `-c`, and `--cc` options) is explicitly given, merge commits will not show a diff, even if a diff format like `--patch` is selected, nor will they match search options like `-S`. The exception is when `--first-parent` is in use, in which case `first-parent` is diff --git a/diff-merges.c b/diff-merges.c index ec97616db1df..6eb72e6fc28a 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -125,6 +125,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) if (!suppress_m_parsing && !strcmp(arg, "-m")) { set_to_default(revs); revs->merges_need_diff = 0; + } else if (!strcmp(arg, "-d")) { + set_first_parent(revs); + revs->merges_imply_patch = 1; } else if (!strcmp(arg, "-c")) { set_combined(revs); revs->merges_imply_patch = 1; diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 5de1d190759f..a07d6eb6dd97 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -473,6 +473,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' ' test_cmp expected actual ' +test_expect_success 'log -d matches --diff-merges=1 -p' ' + git log --diff-merges=1 -p master >result && + process_diffs result >expected && + git log -d master >result && + process_diffs result >actual && + test_cmp expected actual +' + test_expect_success 'deny wrong log.diffMerges config' ' test_config log.diffMerges wrong-value && test_expect_code 128 git log
This option provides a shortcut to request diff with respect to first parent for any kind of commit, universally. It's implemented as pure synonym for "--diff-merges=first-parent --patch". Signed-off-by: Sergey Organov <sorganov@gmail.com> --- Documentation/diff-options.txt | 4 ++++ Documentation/git-log.txt | 2 +- diff-merges.c | 3 +++ t/t4013-diff-various.sh | 8 ++++++++ 4 files changed, 16 insertions(+), 1 deletion(-)