so/diff-merges-more (was Re: What's cooking in git.git (Feb 2023, #01; Thu, 2))

From: Glen Choo <chooglen@google.com>
Date: Feb 7, 2023, 04:06 UTC
Message-ID: <kl6lr0v2i0gn.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <871qn5pyez.fsf@osv.gnss.ru>

Hi Sergey,

Sergey Organov <sorganov@gmail.com> writes:

Show 19 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>
>> * so/diff-merges-more (2022-12-18) 5 commits
>>  - diff-merges: improve --diff-merges documentation
>>  - diff-merges: issue warning on lone '-m' option
>>  - diff-merges: support list of values for --diff-merges
>>  - diff-merges: implement log.diffMerges-m-imply-p config
>>  - diff-merges: implement [no-]hide option and log.diffMergesHide config
>>
>>  Assorted updates to "--diff-merges=X" option.
>>
>>  May want to discard.  Breaking compatibility does not seem worth it.
>>  source: <20221217132955.108542-1-sorganov@gmail.com>
>
> Hi Junio,
>
> This does not break any compatibility, as far as me and I believe
> reviewers of these series are aware.

My 2 cents (which maybe lines up with what Junio meant) is that this series doesn't break compatibility on its own, but IMO the approach doesn't make sense unless we also intend to flip the default somewhere down the line. "log.diffMerges-m-imply-p" is a really specific config option, and it needs to justify its addition with an outsized benefit.

I don't think it meets that bar, the only use cases I can think of are:

- Before we change the default, setting "log.diffMerges-m-imply-p=true"
  gives the 'nicer' behavior. But if the user had the permissions and
  knowledge to do so, they could just pass "-p" instead for correctness.

  If the goal is to reduce typing, then we could add a different CLI
  flag that would behave like "-m -p", or we could teach "git diff" to
  accept proper single-character flags. Either of these options would be
  more discoverable and cleaner.

- After we change the default, setting "log.diffMerges-m-imply-p=false"
  gives the old behavior, which might be needed if the user is running
  scripts written in for an older version of Git. I would find this
  compelling, but as Junio mentioned [1], breaking compatibility doesn't
  seem worth it, so this point is moot. On the other hand, neither of
  the alternative approaches break compatibility, so they're superior in
  this regard too.

- The only legitimate use case I think of is something like a script
  that uses "-m" assuming that it implied "-p", AND the user has no
  ability to modify the script. Then the user's only recourse is to set
  a config. But this is so exotic that I don't think this is a good
  enough reason to have the config.

I wouldn't mind seeing a version of these patches without "log.diffMerges-m-imply-p=false" , but in its current form, I agree with Junio that this isn't worth it.

[1] https://lore.kernel.org/git/xmqqbko1xv86.fsf@gitster.g/