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

Glen Choo [off-list ref] writes:

Hi Sergey,

Sergey Organov [off-list ref] writes:

quoted

Junio C Hamano [off-list ref] writes:

quoted

* 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: [off-list ref]

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.

Except I can't configure this in centralized manner for multiple users
on our host machine?

  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.

The only drawback of this is that this leaves "-m" inconsistent with no
apparent reason.

OTOH, teaching "git log" to use common options machinery to accept "-pm"
looks to be quite a project, and doesn't fix '-m' inconsistency anyway.

Then, out of curiosity, what do you think was the reason to change
"--cc" behavior to match that of "-c"? To save typing? To me, changing
"-m" accordingly is an improvement to the overall feel of git interface,
the same way as changing "--cc" was.

That said, if we decide to go for new option, I'd suggest to add "-d",
and probably obsolete "-m". This does not look to me as appealing as
finally fixing "-m" though.

- 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 incompatibility found was obsolete use of "--first-parent -m",
that was the only use-case for "-m" for a long time, and this could
probably be detected and handled specially, though it sounds like a
nasty kludge.

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

Yes, let's just change "-m" behavior then, and let exotic scripts just
drop "-m", as it's not needed for a long time already in the combo
"--first-parent -m" that currently reads "--first-parent".

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/

All you say is understood and are valid arguments, but then we will
continue to face pretty valid confusion of why "-m" behaves differently
from "-c/--cc/--remerge". I personally don't care, provided I get a way
to make it behave sanely, and that's what "log.diffMerges-m-imply-p"
patch does.

As a kind of complaint, it was simple 1 patch from Junio once upon a
time to change "--cc" to a sane behavior of implying -p, and now it
takes rounds and rounds to do the same for -m. This is rather sad.

Finally, event without "log.diffMerges-m-imply-p", the rest of the
series still makes sense, so if the conclusion is to remove it, let's
still merge the rest, please! Let me know, and I'll then remove the
patch and will change documentation accordingly.

Thanks,
-- Sergey Organov

Sergey Organov [off-list ref] writes:

quoted

  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.

The only drawback of this is that this leaves "-m" inconsistent with no
apparent reason.
[...]
Then, out of curiosity, what do you think was the reason to change
"--cc" behavior to match that of "-c"? To save typing? To me, changing
"-m" accordingly is an improvement to the overall feel of git interface,
the same way as changing "--cc" was.

FWIW, I agree that having an inconsistent "-m" is quite an eyesore, and
there isn't a particularly good reason for it.

The unfortunate reality of Hyrum's law is that some 'mistakes' or CLI
cleanups are difficult or even downright impossible to fix. I'd consider
"-m on its own does nothing" one of those.

All you say is understood and are valid arguments, but then we will
continue to face pretty valid confusion of why "-m" behaves differently
from "-c/--cc/--remerge". I personally don't care, provided I get a way
to make it behave sanely, and that's what "log.diffMerges-m-imply-p"
patch does.

We could say, e.g.:

  "-m" is inconsistent, but we can't change it for backwards
  compatibility reasons. Please do not use it, use "-d" instead, because
  that is more sensible.

As a kind of complaint, it was simple 1 patch from Junio once upon a
time to change "--cc" to a sane behavior of implying -p, and now it
takes rounds and rounds to do the same for -m. This is rather sad.

It is not because Junio has special privileges or anything of the sort,
though. My understanding is that it was just an accident that "-m"
_happened_ to be misused by enough people that we deemed that breakage
unacceptable, whereas "--cc" just _happened_ to be fixable.

FWIW, I might have also been on your side when we rolled back the first
"-m" implies "-p" change, but the consensus since then is that breaking
backwards compatibility just isn't worth it in this case, and it would
take a herculean effort to change that consensus. It might not be
completely impossible though, more on that later...

Finally, event without "log.diffMerges-m-imply-p", the rest of the
series still makes sense, so if the conclusion is to remove it, let's
still merge the rest, please! Let me know, and I'll then remove the
patch and will change documentation accordingly.

Oops. Sorry, I missed this the first time I read this message. This
alone should have warranted a response.

I'm not convinced that the series makes sense without
"log.diffMerges-m-imply-p":

* The main patch is

    diff-merges: implement [no-]hide option and log.diffMergesHide config

  which gives us a way to redefine "-m" as "--diff-merges=hide
  --diff-merges=on". However, we haven't seen any compelling reasons to
  use --diff-merges=hide [1]. I'm also fairly convinced that if we go
  back in time, "-m" wouldn't have the semantics of 'do nothing unless
  -p is also given', and I don't think we should immortalize a mistake
  by giving it an explicit option.

  All the other patches in their current form are dependent on this
  patch going in.

* diff-merges: support list of values for --diff-merges

  This only makes sense if we want to accept multiple values, which we
  don't without the main patch.

* diff-merges: issue warning on lone '-m' option
  diff-merges: improve --diff-merges documentation

  These are docs and UX cleanups as a result of the main patch.

_Maybe_ the number of lone "-m" users will shrink over time to the point
where we could make the switch? We could prepare for that by warning
that lone "-m" is a no-op now, but might imply "-p" at some point in the
future, and then after X amount of time, we might say that users have
had enough warning and actually change the default. I find this
unlikely, but not impossible. However, the patches don't prepare us for
this path, and that would deserve new patches and a different discussion
from what we've been having.

1. https://lore.kernel.org/git/kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com/