Glen Choo <chooglen@xxxxxxxxxx> writes: > We covered this series in Review Club, thanks for coming Sergey! For > those who are interested, the notes are here: > > https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing > > and reviewers will send feedback to the list separately anyway. I mostly > had comments on the design, so I'll leave most of my comments here. > > Commenting on the cover letter as a whole, on first read, it wasn't > obvious to me what this series was trying to achieve because the CL > presents the 5 patches individually instead of a cohesive story. From my > understanding, the story is as follows: we want '-m' (enable > diff-merges) to also imply '-p', but we can't just change the default > behavior, so we do the following instead: > > - Add a config option that gives the behavior that we want (2/5). > - Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and > encouraging users to use that instead (1,4,5/5). Well, very close, but not exactly. I'd rather say: 1. Provide exact semantics of '-m' trough --diff-merges UI by introducing 'hide' option, after which '--diff-merges=on,hide' and '-m' have exactly the same behavior. 2. Add a config option that gives the new behavior we want to '-m': to rather be a synonym for '--diff-merges=on[,hide] -p". (Please notice that 'hide' is not needed here as it's immediately canceled by '-p'.) So, essentially, after (1) is there, the config option turns '-m' meaning from default '--diff-merges=on,hide' to desired '--diff-merges=on -p'. Please also notice that at this point we may instead decide to just switch '-m' meaning to new semantics, either without config at all, or with config that'd rather restore previous semantics. In fact, the primary reason why previously such patch has been reverted was absence of (1), and so with (2) maybe I was just overly cautious. > Patch 3/5 is completely separate. There was some resistance to it during > Review Club, but if we still want this, it might be worth splitting off > into its own series so that we can keep the discussions separate. OK, I'll cut it off for now. > > During the discussion, it also appeared that this "modification of '-m' > semantics" refers to a patch that changed the default but got reverted > due to breaking legacy scripts. It would be extremely useful to include > a link to that previous patch and the discussion around its revert, > especially given the discussion about whether users actually need > '-diff-merges=hide' ([1] and elsewhere). Yep, please see references I've sent in my previous answer to this message. > > [1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@xxxxxxxxxxxxxx/ > > Sergey Organov <sorganov@xxxxxxxxx> writes: > >> 1. --diff-merges=[no]-hide >> >> The set of diff-merges options happened to be incomplete, and failed >> to implement exact semantics of -m option that hides output of diffs >> for merge commits unless -p option is active as well. >> >> The new "hide" option fixes this issue, so that now >> >> --diff-merges=on --diff-merges=hide >> >> combo is the exact synonym for -m. >> >> The log.diffMerges configuration also accepts "hide" and "no-hide" >> values, and governs the default value for the hide bit. The >> configuration behaves as if "--diff-merges=[no-]hide" is inserted >> first in the command-line. > > I had the same concerns as Elijah, which is that this behavior is > probably clearer as a separate flag (like "--hide-diff-merges"), which > is more consistent with how '--diff-options' is used today, which means > that: > > a) it is easier to explain to users > b) the implementation is simpler (I'll comment on Patch 1 code > separately) > c) it makes Patch 4 obsolete I'd postpone implementation/design discussion till we get to agreement of the need for this option in the first place. > But I'm not convinced that we actually want this behavior at all. I > don't see why a user would use a flag that says "do nothing unless > other flags are given". don't find the 'alias use case' compelling, > because the user still has to choose whether to pass '-p', so at that > point they could just add a different alias. If one travels back the history, they will find that originally all -m, -c, and --cc were behaving exactly this way: "do nothing, unless diffs are actually requested", i.e. they specified only diff format to be used once requested, and did not request the output themselves. I prefer to stay on the safe side, and assume that such behavior is still useful, even though -c/--cc turned to imply '-p' eventually, as doing the same to '-m' caused so much desire and resistance simultaneously. OTOH, --diff-merges=<format> does two things: specifies the format and requests the output for merge commits. It could have been design mistake, even though it has been discussed at the time of introduction, but now the only way to get another behavior is to turn off one of the actions they do: turn off requesting the actual output for merge commits, and that's what proposed 'hide' means. > I haven't dug through the code/ML to figure out whether '-m' requiring > '-p' was an intentional feature or not, but if you could find the old > thread where you changed the default (and it got reverted), that would > help the discussion a lot :) Yep, I gave the links in my first answer to this message. > >> 2. log.diffMerges-m-imply-p >> >> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' >> options do. Simply fixing this inconsistency by unconditional >> modification of '-m' semantics appeared to be a bad idea, as it broke >> some legacy scripts/aliases. This patch rather provides configuration >> variable to tweak '-m' behavior accordingly. > > I thought that Junio's suggestion to implement a flag that acts like > '-m' with '-p' [2] was quite a good one (maybe '-M' or > '--diff-merges=show'), since I think that very few users would actually > set this config, but the ones that would actually use it can just > replace '-m' with '-M'. Introducing new short convenience option(s) is out of scope of these series, and suggested --diff-merges=show, as I see it, is essentially "--diff-merges=on -p" that I find hard to explain inside the '--diff-merges=' context which name suggests it affects merge commits only. That said, saying: "we have slightly broken '-m' that we refrain to fix, so let's introduce '-M' instead of fixing '-m'" does not sound very convincing either. > > [2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/ > >> 3. log.diffMergesForce >> >> Force specific log format for -c, --cc, and --remerge-diff options >> instead of their respective formats. The override is useful when some >> external tool hard-codes diff for merges format option. > > This might be better off as its own series, since the change isn't > related to '-m', Yep, I'm sorry to mix it into the series, -- the only excuse is that it looked very relevant to me when I did it :) > but I'm worried about the precedent that this sets. > To my knowledge, CLI options always overwrite config, but this is the > opposite. I would prefer not to do this, especially if the use case is > to work around an external tool (since it is arguably the tool that is > broken). The tool was only an initial motivation for the patch. From following a few discussions on the list I got feeling that every person has their own preference for --diff-merges format, and rarely wants to see anything else. This config essentially gives them a way to say "please use my preferred format everywhere, unless I explicitly say otherwise", in a centralized manner. > >> 5. Issue warning for lone '-m'. >> >> Lone '-m' is in use by scripts/aliases that aim at enabling diff >> output for merge commits, but only if '-p' is then specified as well. >> >> As '-m' may now be configured to imply '-p' (using >> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified, >> and suggest to instead use '--diff-merges=on,hide' that does not >> depend on user configuration. >> >> This is expected to give a provision for enabling >> log.diffMerges-m-imply-p by default in the future. > > Since '-m' without '-p' is a mistake in most cases, I wonder if we > should just emit this warning today (maybe via the advise() API). Even > if we don't keep '--diff-options=hide', deprecating lone '-m' and giving > a warning seems good to keep. I'd tend to rather not. Actually, as far as I'm aware, the only actual use that has been detected was "--fist-parent -m", and that use case was exactly what you guys don't find useful: specify default format for merge commits. In this particular case it is the diff to the first parent, and dates back to the days before --diff-merges, when using history traversal option --first-parent was the only way to get diffs with respect to the first parent for merges. Maybe we should instead flag the "--first-parent -m" as a warning, as producing a warning for lone "-m" without these patches is effectively getting users out of using lone "-m" instead of fixing the latter. I rather see the bright future as using "-m" all the time, as it's now extremely configurable. Thanks, -- Sergey Organov
