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

From: Sergey Organov <sorganov@gmail.com>
Date: Nov 30, 2022, 13:16 UTC
Message-ID: <87lenstwfh.fsf@osv.gnss.ru>
In-Reply-To: <CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 13 quoted lines

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

Yes, it's a precedent indeed, but I don't see any actual problem here. Unlike git silently dropping changes on rebase, this can cause no damage. I think I can emphasize that we now have "formats" and "flags" in the documentation, where "formats" are mutually exclusive (the latest specified wins), while "flags" are cumulative.

Show 22 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?

Completeness is essential as I want '--diff-merges' to provide all the needed capabilities, and one of them was actually missing, that is there in the '-m' semantics, exactly as I said in the descriptions.

Show 21 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.
>
>> 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)?

As I said in my answer to your other mail, '-m' is already configurable, so it is not needed to be included.

None of --diff-merges= options are affected by diffMergesForce, only 3 specific options from the documentation.

Show 5 quoted lines

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

Yeah, thanks, I got your suggestion you put in another mail.

Show 9 quoted lines

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

Yes, they can. For now it's useful only for 'hide', but we might add more flags in the future. It's also harmless, so I don't see it as a serious issue.

Thanks, -- Sergey Organov