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

From: Sergey Organov <sorganov@gmail.com>
Date: Nov 28, 2022, 14:42 UTC
Message-ID: <877czfgmye.fsf@osv.gnss.ru>
In-Reply-To: <xmqqfse37c0n.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 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 do not know how this can be a good idea.  For those users who set
> the configuration variables, those scripts and aliases get broken
> anyway, don't they?  IOW, I am not sure how this is better than the
> "modification of '-m'" that is "a bad idea".

The history behind this is that before the patch #1 of these series there was no way to get exact semantics of current '-m' option using new --diff-merges option, so there was no sensible way to "fix" these scripts/aliases. That was in fact the primary objection for the new '-m' semantics.

Now, when one stomps on such script/alias (by explicitly enabling the new configuration option), they can fix it by replacing '-m' with '--diff-merges=on,hide', that, along with the last patch of these series that produces warning when lone '-m' is detected, looks to me as a way to eventually get rid of the legacy and surprising '-m' semantics.

> I would understand why it may be a safer and more sensible solution,
> if the proposed approach were to find an unused letter $X and to
> introduce "-$X" that is the same as "-m" but implies "-p", though.

I've in fact considered this as well, and '-d' was free for such use last time I've checked. It was very tempting to just add '-d' that always shows diff to first parent for everything, be it merge or not, and call it a day, but it won't fix '-m', which inconsistent behavior still surprises people, and besides started the whole --diff-merges business in the first place.

Show 9 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.
>
> Not convinced it is a good idea for the same reason as above (not
> convinced it is a bad idea, either, though).

Here the intention was entirely different though. I'm using magit and it does hard-code --cc in one place that I'd like to be able to override. If I got this problem, I figured chances are high somebody else will get it as well, and that's the rationale.

Then I figure everybody here has own favorite format for merge commits, and this toy gives you exactly this: whatever option is used (once), you get what you prefer instead, or use an option second time to enforce it.

For stable scripting, we for rather long time now have --diff-merges=c, and --diff-merges=cc, as well as slightly more recent --diff-merges=remerge, that are not affected by the configuration in question.

That said, it looks useful to me, but I won't insist on the feature should you guys object.

Show 6 quoted lines

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

Was useless before 'hide' though, as the rest of options just override each other entirely.

Thanks, -- Sergey Organov