| Message ID | 20210510153451.15090-7-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff-merges: let -m imply -p | expand |
Sergey Organov <sorganov@gmail.com> writes: > Fix long standing inconsistency between -c/--cc that do imply -p, on > one side, and -m that did not imply -p, on the other side. > > After this patch > > git log -m > > will start to produce diffs without need to provide -p as well, that > improves both consistency and usability. It gets even more useful if > one sets "log.diffMerges" configuration variable to "first-parent" to > force -m produce usual diff with respect to first parent only. Please make sure that you clearly state that you do not blindly force --patch output in the proposed log message. Explicitly mentioning that "git log --stat -m" would not give a patch but just diffstat would be assuring. Also this needs a test to ensure that is what happens. Having a test for "log -m" and another for "log -m --stat" would be sufficient. And in the context of this step, the rename of the member in the previous step makes quite a lot of sense. Thanks for working on this topic.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Fix long standing inconsistency between -c/--cc that do imply -p, on >> one side, and -m that did not imply -p, on the other side. >> >> After this patch >> >> git log -m >> >> will start to produce diffs without need to provide -p as well, that >> improves both consistency and usability. It gets even more useful if >> one sets "log.diffMerges" configuration variable to "first-parent" to >> force -m produce usual diff with respect to first parent only. > > Please make sure that you clearly state that you do not blindly > force --patch output in the proposed log message. Explicitly > mentioning that "git log --stat -m" would not give a patch but just > diffstat would be assuring. Also, avoid "-p" in the title. "let -m imply diff generation" might be a good compromise. What --cc/-c implies is to show some kind of diff for merges (dense_combined_merges, combine_merges and !ignore_merges are the members of the revs field that controls how merge commits) and they ask for specific kind of diff is shown. So "-c/--cc imply -p" is quite wrong (you never get straight --patch output for merges when you give -c/--cc---you get combined diff). In a sense, you could say -c/--cc implies -m (i.e. do show some kind of diff for merges). Taken together, perhaps: Subject: diff: let -m imply diff generation The "-c/--cc" options to "git log" asks for merges to be shown with patch-like output, implicitly enabling the "-m" option (which is used to tell "do not ignore merge commits when showing patches). However, the opposite is not true; giving "-m" alone does not tell "git log" that the user wants some form of patches. Make "-m" imply "we want some form of diff output", so that "git log -m" would behave identically to "git log -m -p". When the user explicitly asks for what kind of diff output is desired, e.g. "git log -m --stat", there is no need to imply anything, specifically, do NOT blindly turn on the "-p: option to turn it into "git log -m --stat -p:. or something like that. If we enable "some kind of diff" for "-m", I actually think that by default "git log -m" should be turned into "log --cc". As you told Alex in your response, "log -m -p" is a quite unpleasant format to read---it is there only because it was the only thing we had before we invented "-c/--cc". But that might be outside the scope of this series. I dunno, but if there is no other constraints (like backward compatibility issues), I have a moderately strong preference to use "--cc" over "-m -p" from the get go for unconfigured people, rather than forcing everybody to configure > Also this needs a test to ensure that is what happens. Having a > test for "log -m" and another for "log -m --stat" would be > sufficient. > > And in the context of this step, the rename of the member in the > previous step makes quite a lot of sense. > > Thanks for working on this topic.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: [...] > If we enable "some kind of diff" for "-m", I actually think that by > default "git log -m" should be turned into "log --cc". As you told > Alex in your response, "log -m -p" is a quite unpleasant format to > read---it is there only because it was the only thing we had before > we invented "-c/--cc". Please, no! --cc has unfortunate feature of outputting exactly nothing for a lot of merge commits, causing even more confusion than historical "-m -p" format. The best default for -m output is --diff-merges=first-parent. Everybody is familiar with it, and it's useful. > But that might be outside the scope of this series. I dunno, but if > there is no other constraints (like backward compatibility issues), > I have a moderately strong preference to use "--cc" over "-m -p" > from the get go for unconfigured people, rather than forcing > everybody to configure I rather have strong preference for --diff-merges=first-parent. --cc is only suitable for Git experts, and they know how to get what they want anyway. Yep, by using --cc. Why spare yet another short option for that? Overall, let's rather make -m give diff to the first parent by default. Simple. Useful. Not confusing. Thanks, -- Sergey Organov
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Fix long standing inconsistency between -c/--cc that do imply -p, on >>> one side, and -m that did not imply -p, on the other side. >>> >>> After this patch >>> >>> git log -m >>> >>> will start to produce diffs without need to provide -p as well, that >>> improves both consistency and usability. It gets even more useful if >>> one sets "log.diffMerges" configuration variable to "first-parent" to >>> force -m produce usual diff with respect to first parent only. >> >> Please make sure that you clearly state that you do not blindly >> force --patch output in the proposed log message. Explicitly >> mentioning that "git log --stat -m" would not give a patch but just >> diffstat would be assuring. > > Also, avoid "-p" in the title. "let -m imply diff generation" might > be a good compromise. > > What --cc/-c implies is to show some kind of diff for merges > (dense_combined_merges, combine_merges and !ignore_merges are the > members of the revs field that controls how merge commits) and they > ask for specific kind of diff is shown. So "-c/--cc imply -p" is > quite wrong (you never get straight --patch output for merges when > you give -c/--cc---you get combined diff). In a sense, you could > say -c/--cc implies -m (i.e. do show some kind of diff for merges). > > Taken together, perhaps: > > Subject: diff: let -m imply diff generation > > The "-c/--cc" options to "git log" asks for merges to be shown > with patch-like output, implicitly enabling the "-m" option > (which is used to tell "do not ignore merge commits when showing > patches). However, the opposite is not true; giving "-m" alone > does not tell "git log" that the user wants some form of patches. > > Make "-m" imply "we want some form of diff output", so that "git > log -m" would behave identically to "git log -m -p". When the > user explicitly asks for what kind of diff output is desired, > e.g. "git log -m --stat", there is no need to imply anything, > specifically, do NOT blindly turn on the "-p: option to turn it > into "git log -m --stat -p:. > > or something like that. Fine with me, will do, thanks! -- Sergey Organov
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> Fix long standing inconsistency between -c/--cc that do imply -p, on >> one side, and -m that did not imply -p, on the other side. >> >> After this patch >> >> git log -m >> >> will start to produce diffs without need to provide -p as well, that >> improves both consistency and usability. It gets even more useful if >> one sets "log.diffMerges" configuration variable to "first-parent" to >> force -m produce usual diff with respect to first parent only. > > Please make sure that you clearly state that you do not blindly > force --patch output in the proposed log message. Explicitly > mentioning that "git log --stat -m" would not give a patch but just > diffstat would be assuring. > > Also this needs a test to ensure that is what happens. Having a > test for "log -m" and another for "log -m --stat" would be > sufficient. OK, will do, thanks! -- Sergey Organov
On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Junio C Hamano <gitster@pobox.com> writes: > > [...] > > > If we enable "some kind of diff" for "-m", I actually think that by > > default "git log -m" should be turned into "log --cc". As you told > > Alex in your response, "log -m -p" is a quite unpleasant format to > > read---it is there only because it was the only thing we had before > > we invented "-c/--cc". > > Please, no! --cc has unfortunate feature of outputting exactly nothing > for a lot of merge commits, causing even more confusion than historical > "-m -p" format. > > The best default for -m output is --diff-merges=first-parent. Everybody > is familiar with it, and it's useful. > > > But that might be outside the scope of this series. I dunno, but if > > there is no other constraints (like backward compatibility issues), > > I have a moderately strong preference to use "--cc" over "-m -p" > > from the get go for unconfigured people, rather than forcing > > everybody to configure > > I rather have strong preference for --diff-merges=first-parent. --cc is > only suitable for Git experts, and they know how to get what they want > anyway. Yep, by using --cc. Why spare yet another short option for that? > > Overall, let's rather make -m give diff to the first parent by default. > Simple. Useful. Not confusing. Honestly --diff-merges=separate is fine. Two weeks ago, when I started this discussion, I was trying to use `git log -m` and `git show -m` to find which merge commit introduced a particular change. Extremely verbose diff output would have been great for that, the confusing part was just that `git show -m` produced diff output and `git log -m` did not. Maybe what we really want is a new short option like `git log -m1` which would both enable diff output and set --diff-merges=1. But again, I don't have a strong opinion on which particular diff output is "the best", so I'm happy to leave that decision to the experts. -Alex
On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Junio C Hamano <gitster@pobox.com> writes: > > [...] > > > If we enable "some kind of diff" for "-m", I actually think that by > > default "git log -m" should be turned into "log --cc". As you told > > Alex in your response, "log -m -p" is a quite unpleasant format to > > read---it is there only because it was the only thing we had before > > we invented "-c/--cc". > > Please, no! --cc has unfortunate feature of outputting exactly nothing > for a lot of merge commits, causing even more confusion than historical > "-m -p" format. > > The best default for -m output is --diff-merges=first-parent. Everybody > is familiar with it, and it's useful. > > > But that might be outside the scope of this series. I dunno, but if > > there is no other constraints (like backward compatibility issues), > > I have a moderately strong preference to use "--cc" over "-m -p" > > from the get go for unconfigured people, rather than forcing > > everybody to configure > > I rather have strong preference for --diff-merges=first-parent. --cc is > only suitable for Git experts, and they know how to get what they want > anyway. Yep, by using --cc. Why spare yet another short option for that? Interesting. I have a strong preference for --diff-merges=remerge (yeah, I know it's not upstream, but it's been ready to submit for months, but just backed up behind the other ort changes. Sorry, I can't push those through any faster). I've had others using it for about 9 months now. I think --cc is a lot better than -m for helping you find what users changed when they did the merge, but I agree the format is somewhat difficult for many users to understand. (--diff-merges=remerge, or --remerge-diff, fixes these problems, IMO.) I think --diff-merges=first-parent, while fine when explicitly requested on the command line, would be wildly misleading as a default because it would attribute changes to a merge commit that were made elsewhere. > Overall, let's rather make -m give diff to the first parent by default. > Simple. Useful. Not confusing. I think it's confusing.
Alex Henrie <alexhenrie24@gmail.com> writes: > On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > Junio C Hamano <gitster@pobox.com> writes: >> >> [...] >> >> > If we enable "some kind of diff" for "-m", I actually think that by >> > default "git log -m" should be turned into "log --cc". As you told >> > Alex in your response, "log -m -p" is a quite unpleasant format to >> > read---it is there only because it was the only thing we had before >> > we invented "-c/--cc". >> >> Please, no! --cc has unfortunate feature of outputting exactly nothing >> for a lot of merge commits, causing even more confusion than historical >> "-m -p" format. >> >> The best default for -m output is --diff-merges=first-parent. Everybody >> is familiar with it, and it's useful. >> >> > But that might be outside the scope of this series. I dunno, but if >> > there is no other constraints (like backward compatibility issues), >> > I have a moderately strong preference to use "--cc" over "-m -p" >> > from the get go for unconfigured people, rather than forcing >> > everybody to configure >> >> I rather have strong preference for --diff-merges=first-parent. --cc is >> only suitable for Git experts, and they know how to get what they want >> anyway. Yep, by using --cc. Why spare yet another short option for that? >> >> Overall, let's rather make -m give diff to the first parent by default. >> Simple. Useful. Not confusing. > > Honestly --diff-merges=separate is fine. Two weeks ago, when I started > this discussion, I was trying to use `git log -m` and `git show -m` to > find which merge commit introduced a particular change. Extremely > verbose diff output would have been great for that, the confusing part > was just that `git show -m` produced diff output and `git log -m` did > not. This is not a case in favor of "separate" over "first-parent" as the default for "-m", right? "Which merge commit introduced particular change" is exactly what --diff-merges=1 achieves, so "--diff-merges=separate" was not in fact needed, as I see it. Moreover, it could have produced wrong positives. Looks like --diff-merges=1 is a better fit. > Maybe what we really want is a new short option like `git log -m1` > which would both enable diff output and set --diff-merges=1. Hopefully this will be simply "-m" soon. "-m1" is no-go as optional arguments to short options is a bad idea. It could have been "--m1", but I believe that's not needed. > > But again, I don't have a strong opinion on which particular diff > output is "the best", so I'm happy to leave that decision to the > experts. There is no "the best", and at least "first-parent" and "dense-combined" are to survive, and "dense-combined" already has its "--cc" rather short variant, so it's logical to give -m the other one, especially as it already has this meaning when --first-parent is provided as well. Also, I'm not sure if -c is being in use, and if it isn't, then it could be changed to produce dense-combined format, especially as one still have --diff-merges=condensed nowadays anyway, so that -m and -c will finally give 2 most useful formats. Overall, I still find a lot of sense in giving "-m" exactly first-parent default meaning. Thanks, -- Sergey Organov P.S. If generic options machinery were in use, it could have been possible to say: git log -pm reducing the issue to consistency only. I wonder if anybody have plans to convert setup_revisions() to parse_options() utility?
Elijah Newren <newren@gmail.com> writes: > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > Junio C Hamano <gitster@pobox.com> writes: >> >> [...] >> >> > If we enable "some kind of diff" for "-m", I actually think that by >> > default "git log -m" should be turned into "log --cc". As you told >> > Alex in your response, "log -m -p" is a quite unpleasant format to >> > read---it is there only because it was the only thing we had before >> > we invented "-c/--cc". >> >> Please, no! --cc has unfortunate feature of outputting exactly nothing >> for a lot of merge commits, causing even more confusion than historical >> "-m -p" format. >> >> The best default for -m output is --diff-merges=first-parent. Everybody >> is familiar with it, and it's useful. >> >> > But that might be outside the scope of this series. I dunno, but if >> > there is no other constraints (like backward compatibility issues), >> > I have a moderately strong preference to use "--cc" over "-m -p" >> > from the get go for unconfigured people, rather than forcing >> > everybody to configure >> >> I rather have strong preference for --diff-merges=first-parent. --cc is >> only suitable for Git experts, and they know how to get what they want >> anyway. Yep, by using --cc. Why spare yet another short option for that? > > Interesting. I have a strong preference for --diff-merges=remerge > (yeah, I know it's not upstream, but it's been ready to submit for > months, but just backed up behind the other ort changes. Sorry, I > can't push those through any faster). I've had others using it for > about 9 months now. Once somebody uses it for 9 months and starts to understand what it is and really loves it, she can still set log.diffMerges=remerge (new feature) and have fun. > > I think --cc is a lot better than -m for helping you find what users > changed when they did the merge, Yes, but it doesn't mean it should be the default. In my workflows, the first thing that matters is what commit did what changes on the current branch. I don't typically care what the user changed during the merge operation, only about the result. If I do care, then only after I find the merge commit is responsible, and I can then use --cc if I want to. > but I agree the format is somewhat difficult for many users to > understand. (--diff-merges=remerge, or --remerge-diff, fixes these > problems, IMO.) I think --diff-merges=first-parent, while fine when > explicitly requested on the command line, would be wildly misleading > as a default because it would attribute changes to a merge commit that > were made elsewhere. No, it's exactly this merge commit that made these changes to the current branch. The changes you refer to have been made on another branch, and not by this particular merge commit, and we fortunately have the reference to those commits through the second parent of this one. > >> Overall, let's rather make -m give diff to the first parent by default. >> Simple. Useful. Not confusing. > > I think it's confusing. I think it isn't, once you accept that merge commit does introduce changes to the branch, by itself. Thanks, -- Sergey Organov
On Tue, May 11, 2021 at 12:46 PM Sergey Organov <sorganov@gmail.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > Junio C Hamano <gitster@pobox.com> writes: > >> > >> [...] > >> > >> > If we enable "some kind of diff" for "-m", I actually think that by > >> > default "git log -m" should be turned into "log --cc". As you told > >> > Alex in your response, "log -m -p" is a quite unpleasant format to > >> > read---it is there only because it was the only thing we had before > >> > we invented "-c/--cc". > >> > >> Please, no! --cc has unfortunate feature of outputting exactly nothing > >> for a lot of merge commits, causing even more confusion than historical > >> "-m -p" format. > >> > >> The best default for -m output is --diff-merges=first-parent. Everybody > >> is familiar with it, and it's useful. > >> > >> > But that might be outside the scope of this series. I dunno, but if > >> > there is no other constraints (like backward compatibility issues), > >> > I have a moderately strong preference to use "--cc" over "-m -p" > >> > from the get go for unconfigured people, rather than forcing > >> > everybody to configure > >> > >> I rather have strong preference for --diff-merges=first-parent. --cc is > >> only suitable for Git experts, and they know how to get what they want > >> anyway. Yep, by using --cc. Why spare yet another short option for that? > >> > >> Overall, let's rather make -m give diff to the first parent by default. > >> Simple. Useful. Not confusing. > > > > Honestly --diff-merges=separate is fine. Two weeks ago, when I started > > this discussion, I was trying to use `git log -m` and `git show -m` to > > find which merge commit introduced a particular change. Extremely > > verbose diff output would have been great for that, the confusing part > > was just that `git show -m` produced diff output and `git log -m` did > > not. > > This is not a case in favor of "separate" over "first-parent" as the > default for "-m", right? > > "Which merge commit introduced particular change" is exactly what > --diff-merges=1 achieves, so "--diff-merges=separate" was not in fact > needed, as I see it. Moreover, it could have produced wrong positives. > Looks like --diff-merges=1 is a better fit. I didn't know which branch the change came from. If the change came from the first branch, it would not have appeared under the merge commit with --diff-merges=first-parent. But the change would definitely appear with --diff-merges=separate, which enabled me to identify the merge commit that included it. So yes, this is a case in favor of "separate" over "first-parent", but it's probably not a common enough scenario to demand keeping "separate" for -m. On Tue, May 11, 2021 at 12:31 PM Elijah Newren <newren@gmail.com> wrote: > > Interesting. I have a strong preference for --diff-merges=remerge > (yeah, I know it's not upstream, but it's been ready to submit for > months, but just backed up behind the other ort changes. Sorry, I > can't push those through any faster). I've had others using it for > about 9 months now. --diff-merges=remerge is the default I would expect when no options have been configured or passed on the command line (although it would not have helped in the scenario I described above). I look forward to using it! -Alex
On Tue, May 11, 2021 at 12:00 PM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > Junio C Hamano <gitster@pobox.com> writes: > >> > >> [...] > >> > >> > If we enable "some kind of diff" for "-m", I actually think that by > >> > default "git log -m" should be turned into "log --cc". As you told > >> > Alex in your response, "log -m -p" is a quite unpleasant format to > >> > read---it is there only because it was the only thing we had before > >> > we invented "-c/--cc". > >> > >> Please, no! --cc has unfortunate feature of outputting exactly nothing > >> for a lot of merge commits, causing even more confusion than historical > >> "-m -p" format. > >> > >> The best default for -m output is --diff-merges=first-parent. Everybody > >> is familiar with it, and it's useful. > >> > >> > But that might be outside the scope of this series. I dunno, but if > >> > there is no other constraints (like backward compatibility issues), > >> > I have a moderately strong preference to use "--cc" over "-m -p" > >> > from the get go for unconfigured people, rather than forcing > >> > everybody to configure > >> > >> I rather have strong preference for --diff-merges=first-parent. --cc is > >> only suitable for Git experts, and they know how to get what they want > >> anyway. Yep, by using --cc. Why spare yet another short option for that? > > > > Interesting. I have a strong preference for --diff-merges=remerge > > (yeah, I know it's not upstream, but it's been ready to submit for > > months, but just backed up behind the other ort changes. Sorry, I > > can't push those through any faster). I've had others using it for > > about 9 months now. > > Once somebody uses it for 9 months and starts to understand what it is > and really loves it, she can still set log.diffMerges=remerge (new > feature) and have fun. > > > > > I think --cc is a lot better than -m for helping you find what users > > changed when they did the merge, > > Yes, but it doesn't mean it should be the default. I didn't say it should be. > In my workflows, the first thing that matters is what commit did what > changes on the current branch. I don't typically care what the user > changed during the merge operation, only about the result. If I do care, > then only after I find the merge commit is responsible, and I can then > use --cc if I want to. > > > but I agree the format is somewhat difficult for many users to > > understand. (--diff-merges=remerge, or --remerge-diff, fixes these > > problems, IMO.) I think --diff-merges=first-parent, while fine when > > explicitly requested on the command line, would be wildly misleading > > as a default because it would attribute changes to a merge commit that > > were made elsewhere. > > No, it's exactly this merge commit that made these changes to the > current branch. The changes you refer to have been made on another > branch, and not by this particular merge commit, and we fortunately have > the reference to those commits through the second parent of this one. If you only care about "what introduced these changes to the current branch", then it's not only the diff against second parent that is irrelevant: ALL commits that are part of the history only via the second or later parents are also irrelevant and thus you should be using --first-parent when asking this question. That changes both history traversal and the diff output. It's a reasonable question, sure, but I didn't see you suggest that -m should change both. Perhaps you did mean that, in which case I think that's a proposal that would make -m become useful. It'd be a drastically different option than what -m is today, which would normally make me wonder if it'd be considered a backward compatibility issue, but given how useless -m has been and since Junio apparently even suggested changing -m, this might be okay. > >> Overall, let's rather make -m give diff to the first parent by default. > >> Simple. Useful. Not confusing. > > > > I think it's confusing. > > I think it isn't, once you accept that merge commit does introduce > changes to the branch, by itself. Asking what commits introduced changes to the branch is a useful question, but one which requires changing history traversal. Unless making -m imply --first-parent is part of the proposal, I still find it somewhat confusing. Granted, "what changes were introduced to the branch?" is certainly not the only question you might be looking for answers to. Others include "what changes were people making in each commit" or "who added calls to this function and why" (e.g. with git log S$function -p $maybe_some_merge_diff_flag) or "who caused this function to end up the way it is" (e.g. with git log -L :<funcname>:<file> -p $some_diff_merge_flag). For any of those, a first parent diff is pretty terrible and highly misleading. A separate diff is no better. --cc would often be useful, with some caveats -- the biggest of which is just how esoteric combined-diffs are. --remerge-diff is way more natural for each of these questions I'm coming up with. Are there other questions that -m helps us answer where first-parent diffs make sense and are generally correct without also needing --first-parent? I'm not thinking of any right now.
Alex Henrie <alexhenrie24@gmail.com> writes: > On Tue, May 11, 2021 at 12:46 PM Sergey Organov <sorganov@gmail.com> wrote: >> >> Alex Henrie <alexhenrie24@gmail.com> writes: >> >> > On Tue, May 11, 2021 at 8:03 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> >> > Junio C Hamano <gitster@pobox.com> writes: >> >> >> >> [...] >> >> >> >> > If we enable "some kind of diff" for "-m", I actually think that by >> >> > default "git log -m" should be turned into "log --cc". As you told >> >> > Alex in your response, "log -m -p" is a quite unpleasant format to >> >> > read---it is there only because it was the only thing we had before >> >> > we invented "-c/--cc". >> >> >> >> Please, no! --cc has unfortunate feature of outputting exactly nothing >> >> for a lot of merge commits, causing even more confusion than historical >> >> "-m -p" format. >> >> >> >> The best default for -m output is --diff-merges=first-parent. Everybody >> >> is familiar with it, and it's useful. >> >> >> >> > But that might be outside the scope of this series. I dunno, but if >> >> > there is no other constraints (like backward compatibility issues), >> >> > I have a moderately strong preference to use "--cc" over "-m -p" >> >> > from the get go for unconfigured people, rather than forcing >> >> > everybody to configure >> >> >> >> I rather have strong preference for --diff-merges=first-parent. --cc is >> >> only suitable for Git experts, and they know how to get what they want >> >> anyway. Yep, by using --cc. Why spare yet another short option for that? >> >> >> >> Overall, let's rather make -m give diff to the first parent by default. >> >> Simple. Useful. Not confusing. >> > >> > Honestly --diff-merges=separate is fine. Two weeks ago, when I started >> > this discussion, I was trying to use `git log -m` and `git show -m` to >> > find which merge commit introduced a particular change. Extremely >> > verbose diff output would have been great for that, the confusing part >> > was just that `git show -m` produced diff output and `git log -m` did >> > not. >> >> This is not a case in favor of "separate" over "first-parent" as the >> default for "-m", right? >> >> "Which merge commit introduced particular change" is exactly what >> --diff-merges=1 achieves, so "--diff-merges=separate" was not in fact >> needed, as I see it. Moreover, it could have produced wrong positives. >> Looks like --diff-merges=1 is a better fit. > > I didn't know which branch the change came from. If the change came > from the first branch, it would not have appeared under the merge > commit with --diff-merges=first-parent. The change in question either came from this merge, or it didn't. If it came from this merge, it will be there in --diff-merges=first-parent even if you have octopus merge and multiple parents, in which case it will be cumulative change from all the side parents. > But the change would definitely appear with --diff-merges=separate, > which enabled me to identify the merge commit that included it. The second part of --diff-merges=separate, that is absent in --diff-merges=first-parent, shows the diff that this merge would have made to the side branch, if you had considered the result of merge the tip of that branch, and you didn't. This second part just makes no sense at all when you care about changes to your files in any typical git workflow, as far as I can tell. > So yes, this is a case in favor of "separate" over "first-parent", but > it's probably not a common enough scenario to demand keeping > "separate" for -m. To me it looks like it rather only shows how deeply confusing --diff-merges=separate actually is. I'd just kill it. Thanks, -- Sergey Organov
Elijah Newren <newren@gmail.com> writes: > On Tue, May 11, 2021 at 12:00 PM Sergey Organov <sorganov@gmail.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> >> > Junio C Hamano <gitster@pobox.com> writes: >> >> >> >> [...] >> >> >> >> > If we enable "some kind of diff" for "-m", I actually think that by >> >> > default "git log -m" should be turned into "log --cc". As you told >> >> > Alex in your response, "log -m -p" is a quite unpleasant format to >> >> > read---it is there only because it was the only thing we had before >> >> > we invented "-c/--cc". >> >> >> >> Please, no! --cc has unfortunate feature of outputting exactly nothing >> >> for a lot of merge commits, causing even more confusion than historical >> >> "-m -p" format. >> >> >> >> The best default for -m output is --diff-merges=first-parent. Everybody >> >> is familiar with it, and it's useful. >> >> >> >> > But that might be outside the scope of this series. I dunno, but if >> >> > there is no other constraints (like backward compatibility issues), >> >> > I have a moderately strong preference to use "--cc" over "-m -p" >> >> > from the get go for unconfigured people, rather than forcing >> >> > everybody to configure >> >> >> >> I rather have strong preference for --diff-merges=first-parent. --cc is >> >> only suitable for Git experts, and they know how to get what they want >> >> anyway. Yep, by using --cc. Why spare yet another short option for that? >> > >> > Interesting. I have a strong preference for --diff-merges=remerge >> > (yeah, I know it's not upstream, but it's been ready to submit for >> > months, but just backed up behind the other ort changes. Sorry, I >> > can't push those through any faster). I've had others using it for >> > about 9 months now. >> >> Once somebody uses it for 9 months and starts to understand what it is >> and really loves it, she can still set log.diffMerges=remerge (new >> feature) and have fun. >> >> > >> > I think --cc is a lot better than -m for helping you find what users >> > changed when they did the merge, >> >> Yes, but it doesn't mean it should be the default. > > I didn't say it should be. > >> In my workflows, the first thing that matters is what commit did what >> changes on the current branch. I don't typically care what the user >> changed during the merge operation, only about the result. If I do care, >> then only after I find the merge commit is responsible, and I can then >> use --cc if I want to. >> >> > but I agree the format is somewhat difficult for many users to >> > understand. (--diff-merges=remerge, or --remerge-diff, fixes these >> > problems, IMO.) I think --diff-merges=first-parent, while fine when >> > explicitly requested on the command line, would be wildly misleading >> > as a default because it would attribute changes to a merge commit that >> > were made elsewhere. >> >> No, it's exactly this merge commit that made these changes to the >> current branch. The changes you refer to have been made on another >> branch, and not by this particular merge commit, and we fortunately have >> the reference to those commits through the second parent of this one. > > If you only care about "what introduced these changes to the current > branch", then it's not only the diff against second parent that is > irrelevant: ALL commits that are part of the history only via the > second or later parents are also irrelevant and thus you should be > using --first-parent when asking this question. That changes both > history traversal and the diff output. No, it's exactly why I don't always want --first-parent. I want to traverse *all* the history, yet to see what's changed by every commit on *this* branch in the process of traversal, be it a merge on not a merge commit. Thanks, -- Sergey Organov
Elijah Newren <newren@gmail.com> writes: > On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: >> ... > I think --cc is a lot better than -m for helping you find what users > changed when they did the merge, but I agree the format is somewhat > difficult for many users to understand. (--diff-merges=remerge, or > --remerge-diff, fixes these problems, IMO.) I think > --diff-merges=first-parent, while fine when explicitly requested on > the command line, would be wildly misleading as a default because it > would attribute changes to a merge commit that were made elsewhere. > >> Overall, let's rather make -m give diff to the first parent by default. >> Simple. Useful. Not confusing. > > I think it's confusing. I do not think it is particularly confusing---after all, it shows "here is a comparison between two trees" the users are familiar with in a single strand of pearls. But I do think that it is an utterly misleading option to show merges in general. When "log" is used with the "--first-parent" traversal, "compare with the first parent to show everything the side branch did" is an acceptable alternative, but even there, it is far less suitable than the "remerge" or "cc", I would think, as the default format. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Elijah Newren <newren@gmail.com> writes: > >> On Tue, May 11, 2021 at 7:03 AM Sergey Organov <sorganov@gmail.com> wrote: >>> ... >> I think --cc is a lot better than -m for helping you find what users >> changed when they did the merge, but I agree the format is somewhat >> difficult for many users to understand. (--diff-merges=remerge, or >> --remerge-diff, fixes these problems, IMO.) I think >> --diff-merges=first-parent, while fine when explicitly requested on >> the command line, would be wildly misleading as a default because it >> would attribute changes to a merge commit that were made elsewhere. >> >>> Overall, let's rather make -m give diff to the first parent by default. >>> Simple. Useful. Not confusing. >> >> I think it's confusing. > > I do not think it is particularly confusing---after all, it shows > "here is a comparison between two trees" the users are familiar with > in a single strand of pearls. Moreover, it's comparison between two most natural trees, the same trees as for non-merge commits. > > But I do think that it is an utterly misleading option to show > merges in general. When "log" is used with the "--first-parent" > traversal, "compare with the first parent to show everything the > side branch did" is an acceptable alternative, but even there, it is > far less suitable than the "remerge" or "cc", I would think, as the > default format. What I still fail to see is why somebody wants to mix traversal rules with representation of merge commits, sorry. If I want to see merges in --cc, I want them in --cc, no matter which way I get to particular commit through the DAG. If I want to see them in --diff-merges=1, I want them this way, no matter how I happened to get to particular commit. Is it more likely that I want to see merges in --diff-merges=1 when I use --first-parent? Yes, of course! That's why --first-parent implies --diff-merges=1. Vice versa? No, why? Thanks, -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: >> But I do think that it is an utterly misleading option to show >> merges in general. When "log" is used with the "--first-parent" >> traversal, "compare with the first parent to show everything the >> side branch did" is an acceptable alternative, but even there, it is >> far less suitable than the "remerge" or "cc", I would think, as the >> default format. > > What I still fail to see is why somebody wants to mix traversal rules > with representation of merge commits, sorry. It is not about "mixing". What I meant was that diff between the pre-merge state and the merge result as two-tree comparison may be an acceptable view of "what did this _merge_ contribute to the history" and that is in line is what the first-parent traversal is trying to show the reader of the history, which is "this merge contributed to the history, and then this other merge contributed to the history, ...". It also may make sense when one is inspecting a merge in isolation, i.e. "git show M". When one is viewing individual commits without omitting commits on the side branch, however, showing "what did the side branch as a whole contributed to the history at this merge" makes little sense and is misleading. Such a patch given for a merge mostly overlaps with what the reader sees immediately after seeing the merge (i.e. the commits on the side branch, with their own patches, each of which has already been shown in the first-parent comparison of the merge). In other words, "git log --no-first-parent" is about contribution of each single commit (whether it is a merge or a non-merge) makes, and the --diff-merges=1 presentation is not in line with that worldview, in which the contribution to the history the merge alone makes is what --cc or remerge tries to show, i.e. "what did the merge do, on top of what the commits on the side branch contributed to the history?" So, it is not like two unrelated things are mixed together. It is more like showing the result of a merge as a diff relative to its first-parent is tied to and consistent with the viewpoint of history as single strand of pearls that is a series of merges into the first-parent chain, but not with non-first-parent presentation of the history.
Sergey Organov wrote: > Alex Henrie <alexhenrie24@gmail.com> writes: > > > I didn't know which branch the change came from. If the change came > > from the first branch, it would not have appeared under the merge > > commit with --diff-merges=first-parent. > > The change in question either came from this merge, or it didn't. If it > came from this merge, it will be there in --diff-merges=first-parent > even if you have octopus merge and multiple parents, in which case it > will be cumulative change from all the side parents. Precisely. The primary thing I care about is what is the status of the branch *after* the merge. In other words; if I rewind history and I hadn't done the merge, what would be diffent? That in my mind is the actual content of the merge.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Fix long standing inconsistency between -c/--cc that do imply -p, on >>> one side, and -m that did not imply -p, on the other side. >>> >>> After this patch >>> >>> git log -m >>> >>> will start to produce diffs without need to provide -p as well, that >>> improves both consistency and usability. It gets even more useful if >>> one sets "log.diffMerges" configuration variable to "first-parent" to >>> force -m produce usual diff with respect to first parent only. >> >> Please make sure that you clearly state that you do not blindly >> force --patch output in the proposed log message. Explicitly >> mentioning that "git log --stat -m" would not give a patch but just >> diffstat would be assuring. > > Also, avoid "-p" in the title. "let -m imply diff generation" might > be a good compromise. > > What --cc/-c implies is to show some kind of diff for merges > (dense_combined_merges, combine_merges and !ignore_merges are the > members of the revs field that controls how merge commits) and they > ask for specific kind of diff is shown. So "-c/--cc imply -p" is > quite wrong (you never get straight --patch output for merges when > you give -c/--cc---you get combined diff). In a sense, you could > say -c/--cc implies -m (i.e. do show some kind of diff for merges). > > Taken together, perhaps: > > Subject: diff: let -m imply diff generation > > The "-c/--cc" options to "git log" asks for merges to be shown > with patch-like output, implicitly enabling the "-m" option > (which is used to tell "do not ignore merge commits when showing > patches). However, the opposite is not true; giving "-m" alone > does not tell "git log" that the user wants some form of patches. > > Make "-m" imply "we want some form of diff output", so that "git > log -m" would behave identically to "git log -m -p". When the > user explicitly asks for what kind of diff output is desired, > e.g. "git log -m --stat", there is no need to imply anything, > specifically, do NOT blindly turn on the "-p: option to turn it > into "git log -m --stat -p:. > > or something like that. While working on this, I've added more tests and explanations to the next re-roll as I've promised, but I didn't change the subject nor did I try to explain that much, as my original subject is consistent with current Git documentation, concise, and straight to the point. "<opt> implies -p" -- that's how current Git manual describes the behavior, and that's how I described it in these patches. Whatever "imply" actually means for -c/--cc, it now means for -m as well, and that's the essence of the patches, so I stand for my original subject. I think that if we realize that "<opt> implies -p" is not precise enough description of actual behavior, it should be a separate topic of documentation improvements, and doesn't belong to these series anyway. Thanks, -- Sergey Organov
Hi, Elijah Newren wrote: > Interesting. I have a strong preference for --diff-merges=remerge > (yeah, I know it's not upstream, but it's been ready to submit for > months, but just backed up behind the other ort changes. Sorry, I > can't push those through any faster). I've had others using it for > about 9 months now. I agree. The --diff-merges=remerge behavior has also been the default in Gerrit since time immemorial, and when I first started using Gerrit it was one of my favorite things about it. That is because it shows the human content of the merge: it shows exactly what changes were made after the automated part was done. I don't have a strong opinion about what the default for -m should be. > I think --cc is a lot better than -m for helping you find what users > changed when they did the merge, but I agree the format is somewhat > difficult for many users to understand. My experience in training new users has been that --cc is quite counterintuitive for them. Like you're hinting, I suspect it's fundamentally just meant to be a fast approximation to --diff-merges=remerge. Thanks, Jonathan
Sergey Organov wrote: > Fix long standing inconsistency between -c/--cc that do imply -p, on > one side, and -m that did not imply -p, on the other side. > > After this patch > > git log -m > > will start to produce diffs without need to provide -p as well, Personally I don't ever use -m without -p and --first-parent, so in that sense this feels like a change in the right direction. Does this also affect the plumbing command "git diff-tree"? I'm guessing "no" because diff-tree already generates a diff by default, but it seems worth spelling out in the commit message to prevent worries about the effect on scripts that expect stable plumbing behavior. Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: > Sergey Organov wrote: > >> Fix long standing inconsistency between -c/--cc that do imply -p, on >> one side, and -m that did not imply -p, on the other side. >> >> After this patch >> >> git log -m >> >> will start to produce diffs without need to provide -p as well, > > Personally I don't ever use -m without -p and --first-parent, so in > that sense this feels like a change in the right direction. > > Does this also affect the plumbing command "git diff-tree"? I'm > guessing "no" because diff-tree already generates a diff by default, > but it seems worth spelling out in the commit message to prevent > worries about the effect on scripts that expect stable plumbing > behavior. Well, here are existing relevant tests: diff-tree master diff-tree -p master diff-tree -p -m master that all still pass after the patches, and I believe diff-tree -m master does change the behavior, but provided it's useless before these patches, it should be OK to have this change. Thanks, -- Sergey Organov
Jonathan Nieder <jrnieder@gmail.com> writes: > Sergey Organov wrote: > >> Fix long standing inconsistency between -c/--cc that do imply -p, on >> one side, and -m that did not imply -p, on the other side. >> >> After this patch >> >> git log -m >> >> will start to produce diffs without need to provide -p as well, > > Personally I don't ever use -m without -p and --first-parent, so in > that sense this feels like a change in the right direction. > > Does this also affect the plumbing command "git diff-tree"? I'm > guessing "no" because diff-tree already generates a diff by default, > but it seems worth spelling out in the commit message to prevent > worries about the effect on scripts that expect stable plumbing > behavior. This is about "log" from the "rev-list" family, not "diff" to compare two endpoints, so "git diff" won't be affected, and "git diff-tree" is not affected, either.
Junio C Hamano <gitster@pobox.com> writes: > Jonathan Nieder <jrnieder@gmail.com> writes: > >> Sergey Organov wrote: >> >>> Fix long standing inconsistency between -c/--cc that do imply -p, on >>> one side, and -m that did not imply -p, on the other side. >>> >>> After this patch >>> >>> git log -m >>> >>> will start to produce diffs without need to provide -p as well, >> >> Personally I don't ever use -m without -p and --first-parent, so in >> that sense this feels like a change in the right direction. >> >> Does this also affect the plumbing command "git diff-tree"? I'm >> guessing "no" because diff-tree already generates a diff by default, >> but it seems worth spelling out in the commit message to prevent >> worries about the effect on scripts that expect stable plumbing >> behavior. > > This is about "log" from the "rev-list" family, not "diff" to > compare two endpoints, so "git diff" won't be affected, and "git > diff-tree" is not affected, either. You are right. I've added a test for "git diff-tree -m", and the changes don't break it. Can easily re-roll if you think the test worth it. Thanks, -- Sergey Organov
Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: >> This is about "log" from the "rev-list" family, not "diff" to >> compare two endpoints, so "git diff" won't be affected, and "git >> diff-tree" is not affected, either. > > You are right. I've added a test for "git diff-tree -m", and the changes > don't break it. Good. > Can easily re-roll if you think the test worth it. I care more about the commit message than the test --- i.e. mentioning that this doesn't affect commands like git rev-list HEAD~5..HEAD | git diff-tree --stdin -m Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: > Sergey Organov wrote: >> Junio C Hamano <gitster@pobox.com> writes: > >>> This is about "log" from the "rev-list" family, not "diff" to >>> compare two endpoints, so "git diff" won't be affected, and "git >>> diff-tree" is not affected, either. >> >> You are right. I've added a test for "git diff-tree -m", and the changes >> don't break it. > > Good. > >> Can easily re-roll if you think the test worth it. > > I care more about the commit message than the test --- i.e. mentioning > that this doesn't affect commands like > > git rev-list HEAD~5..HEAD | git diff-tree --stdin -m OK, re-roll to follow... Thanks, -- Sergey Organov
Hi, Jonathan Nieder <jrnieder@gmail.com> writes: > Hi, > > Elijah Newren wrote: > >> Interesting. I have a strong preference for --diff-merges=remerge >> (yeah, I know it's not upstream, but it's been ready to submit for >> months, but just backed up behind the other ort changes. Sorry, I >> can't push those through any faster). I've had others using it for >> about 9 months now. > > I agree. The --diff-merges=remerge behavior has also been the default > in Gerrit since time immemorial, and when I first started using Gerrit > it was one of my favorite things about it. Due to previous set of diff-merges patches you now can set log.diffMerges=remerge and have fun having it as the default for "-m" in pure Git. I've introduced log.diffMerges exactly because I do realize that different people and different workflows might have different preferences here. > That is because it shows the human content of the merge: it shows > exactly what changes were made after the automated part was done. No, in fact it shows what additional changes were to be made if the merge had been achieved using particular automated merge algorithm you apply right now. Provided you utilize no external knowledge, you have no idea how merge has been in fact achieved, as core Git (fortunately) neither cares nor stores such information. Contrary to your view, and I believe more inline with Git core philosophy, I (usually) don't care *how* particular changes were achieved, I rather care *what* they are, so pure diff to the first parent is the best basic format for me. Only if I need to see *why* the changes are this particular way, I'd use advanced tools and formats to get to the best *guess*, and that's where these advanced formats get handy. In other words, for me merge commit is mostly *commit*, and only then it's *merge*: "here are the changes, and, by the way, here is where these changes came from." That said, please don't get me wrong: I'm all for advanced features such as --diff-merges=remerge, as I do understand how and when they are useful, I just personally won't make them active by default. > > I don't have a strong opinion about what the default for -m should be. > >> I think --cc is a lot better than -m for helping you find what users >> changed when they did the merge, but I agree the format is somewhat >> difficult for many users to understand. I do have strong opinion. --diff-merges=1 is the only sensible factory-default. Not only it has no implicit assumptions about how given commit has been achieved, it's also the only format even entire Git newbie might already be familiar with. Then, --cc and I suspect "remerge" too, give exactly empty output most of times, that'd cause even more surprises when one of them is the default: you enable diff for merges, but still get nothing. OTOH, --diff-merges=1 will give empty output if and only if given commit actually brings no changes to the branch, exactly as any commit out there. No surprises at all. Principle of least surprise is still a good thing to follow. Thanks, -- Sergey Organov
Sergey Organov wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > > I don't have a strong opinion about what the default for -m should be. > > > >> I think --cc is a lot better than -m for helping you find what users > >> changed when they did the merge, but I agree the format is somewhat > >> difficult for many users to understand. > > I do have strong opinion. --diff-merges=1 is the only sensible > factory-default. Not only it has no implicit assumptions about how given > commit has been achieved, it's also the only format even entire Git > newbie might already be familiar with. I agree. > Principle of least surprise is still a good thing to follow. I could not agree more.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 530d1159141f..32e6dee5ac3b 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -49,10 +49,9 @@ ifdef::git-log[] --diff-merges=m::: -m::: This option makes diff output for merge commits to be shown in - the default format. `-m` will produce the output only if `-p` - is given as well. The default format could be changed using + the default format. The default format could be changed using `log.diffMerges` configuration parameter, which default value - is `separate`. + is `separate`. `-m` implies `-p`. + --diff-merges=first-parent::: --diff-merges=1::: @@ -62,7 +61,8 @@ ifdef::git-log[] --diff-merges=separate::: This makes merge commits show the full diff with respect to each of the parents. Separate log entry and diff is generated - for each parent. + for each parent. This is the format that `-m` produced + historically. + --diff-merges=combined::: --diff-merges=c::: diff --git a/diff-merges.c b/diff-merges.c index 211c99482cac..a827482a97ff 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -98,6 +98,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) if (!strcmp(arg, "-m")) { set_to_default(revs); + revs->merges_imply_patch = 1; } else if (!strcmp(arg, "-c")) { set_combined(revs); revs->merges_imply_patch = 1;
Fix long standing inconsistency between -c/--cc that do imply -p, on one side, and -m that did not imply -p, on the other side. After this patch git log -m will start to produce diffs without need to provide -p as well, that improves both consistency and usability. It gets even more useful if one sets "log.diffMerges" configuration variable to "first-parent" to force -m produce usual diff with respect to first parent only. Previous semantics of -m could still be accessed using --diff-merges=separate option. Change documentation accordingly. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- Documentation/diff-options.txt | 8 ++++---- diff-merges.c | 1 + 2 files changed, 5 insertions(+), 4 deletions(-)