Junio C Hamano <gitster@xxxxxxxxx> writes: > Junio C Hamano <gitster@xxxxxxxxx> writes: > >> Sergey Organov <sorganov@xxxxxxxxx> writes: >> >>> Yep, but --diff-merges=m doesn't imply -p either, though it does produce >>> diff output without -p, for merge commits only. >> >> I misspoke without thinking it through. It is absolutely wrong for >> the "-m" option (or "--diff-merges=m" for that matter) to imply >> "-p". >> >> $ git log --stat --summary >> >> would show "diff", but the kind of "diff" requested is not a textual >> patch but just diffstat and the summary of new/removed files, and >> the "diff" is shown only for single-parent commits, and it omits >> "diff" for merge commits. Adding "-m" to this command line is *not* >> a request to show the textual patch. It is to ask "diff" to be >> shown pairwise with each of the parent. >> >> $ git log -m --stat --summary >> >> It is probably OK to special case "-m" given alone without any other >> option [*1*] that specifies what kind of "diff" is requested and >> make it imply "-p". But unconditionally flipping "-p" on only >> because you saw "-m" (or "--diff-merges=m" for that matter) is just >> wrong. > > Luckily, > > $ git log [--stat] --diff-merges=first-parent master..seen > > seems to do almost the right thing, with respect to the "It is > probably OK to special case" I gave above. I believe any special-casing is to be a last resort, and definitely is not the right thing to do in this particular case. > > It only "enables diff" for merge commits, which does not quite feel > right and we may want to do the same "enable diff" for single parent > commits, but the good part is that it does not blindly imply "-p". > > It seems to do the "enable diff" the right way by honoring other > command line options that specify the format of the diff, so with > "--stat" included in the sample command line above, we get the > diffstat for single parent commits (because we ask for "--stat" from > the command line to show it throughout the history) and also for > merge commits (because --diff-merges=first-parent does *not* blindly > turn the textual patch '-p' on). Good to know! I must admit I did nothing special in this regard, just paid attention to avoid breaking any existing logic, at least knowingly. > >> [Footnote] >> >> *1* They are not limited to "-p", "--stat" and "--summary", but >> you'd need to also pay attention to "--raw", "--name-only", etc.) > > I've merged the so/log-diff-merge topic to 'master', with this > (possibly) known breakage that it does not do anything for single > parent commits. We may want to fix this last mile before the > release that is scheduled to happen around early June. I have no idea what the breakage is or could be. Do we have any relevant tests in place already, or can somebody suggest some to check for possible breakage? > Note that I didn't check if you are doing the right thing for all > formats, or if I was lucky and --stat was only one of them you paid > attention to, when you needed to notice all others that you don't. > But if you used the same logic that allows "git show" to by default > give "-p/--cc" output while "git show --stat" to squelch the patch > output, you should be OK. In fact I didn't do anything specific to --stat, nor to other options you mention (--raw, --name-only, --summary), so I'd expect all of them still work the same way they were before my --diff-merges series. Need to be checked anyway, sure thing, and that gets us back to the questions of tests. I personally don't know what expectations are, so it's hard for me to implement needed tests. Thanks, -- Sergey Organov
