Re: [PATCH 0/5] diff-merges: more features

From: Glen Choo <chooglen@google.com>
Date: Dec 7, 2022, 23:55 UTC
Message-ID: <kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20221127093721.31012-1-sorganov@gmail.com>

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).

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.

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).

[1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/

Sergey Organov <sorganov@gmail.com> writes:

Show 16 quoted lines

> 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

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". I 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.

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 :)

Show 7 quoted lines

> 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'.

[2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/

Show 5 quoted lines

> 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', 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).

Show 12 quoted lines

> 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.