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

From: Elijah Newren <newren@gmail.com>
Date: Nov 29, 2022, 04:50 UTC
Message-ID: <CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com>
In-Reply-To: <20221127093721.31012-1-sorganov@gmail.com>

On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:

>
> 1. --diff-merges=[no]-hide

This seems problematic to me. Currently, all the options to diff-merges are exclusive of each other; the user is picking one of them to determine how to format diffs for merges. Now you are introducing the ability to combine various options, leading users to think that perhaps they can run with all three of `--diff-merges=combined-dense --diff-merges=remerge --diff-merges=separate` or other nonsensical combinations. Shouldn't this [no-]hide stuff be a separate flag rather than reusing --diff-merges?

Show 9 quoted lines

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

Why is completeness important here?  Perhaps I should state this
another way: when would users ever want to use this new "hide" option?
 I got through your cover letter not knowing the answer to this, but
was hoping it'd at least be covered in one of your commit messages or
documentation changes.  Maybe it was there, but I somehow missed it.

Is the only goal some sense of developer completeness for these options, or are these end-user-facing options of utility to actual end users? I'm hoping the latter, but if so, can that be documented and explained somewhere? I'm pretty sure this is explained somewhere in an old mailing list discussion, but where?

Show 12 quoted lines

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

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.

Why just these three options and not -m (or --diff-merges=separate)?

Also, I read this and didn't quite fully grasp the intent; your explanation in response to Junio seemed much more enlightening. Perhaps the wording/explanation could be cleaned up a bit? I'll comment more on that specific patch...

> 4. Support list of values for --diff-merges
>
> This allows for shorter --diff-merges=on,hide forms.

And thus making users think they can pass --diff-merges=combined-dense,remerge,separate and suspecting that it'll do something useful? Seems like this is reinforcing a mistake to me.

Show 34 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.
>
> Sergey Organov (5):
>   diff-merges: implement [no-]hide option and log.diffMergesHide config
>   diff-merges: implement log.diffMerges-m-imply-p config
>   diff-merges: implement log.diffMergesForce config
>   diff-merges: support list of values for --diff-merges
>   diff-merges: issue warning on lone '-m' option
>
>  Documentation/config/log.txt                  |  20 ++++
>  Documentation/diff-options.txt                |  20 +++-
>  builtin/log.c                                 |   6 +
>  diff-merges.c                                 | 108 +++++++++++++++---
>  diff-merges.h                                 |   6 +
>  t/t4013-diff-various.sh                       |  89 ++++++++++++++-
>  ...ges=first-parent_--diff-merges=hide_master |  34 ++++++
>  t/t9902-completion.sh                         |   9 ++
>  8 files changed, 272 insertions(+), 20 deletions(-)
>  create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
>
> --
> 2.37.3.526.g5f84746cb16b
>