[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 0/5] diff-merges: more features
From:       Sergey Organov <sorganov () gmail ! com>
Date:       2022-11-30 13:16:02
Message-ID: 87lenstwfh.fsf () osv ! gnss ! ru
[Download RAW message or body]

Elijah Newren <newren@gmail.com> writes:

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

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

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

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

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

[prev in list] [next in list] [prev in thread] [next in thread]