Re: Can we clarify the purpose of `git diff -s`?

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Junio C Hamano <gitster@pobox.com>, Sergey Organov <sorganov@gmail.com>
Cc: Matthieu Moy <Matthieu.Moy@univ-lyon1.fr>,
	Felipe Contreras <felipe.contreras@gmail.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: Can we clarify the purpose of `git diff -s`?
Date: Fri, 12 May 2023 14:28:14 -0600	[thread overview]
Message-ID: <645ea15eca6fa_21989f294f5@chronos.notmuch> (raw)
In-Reply-To: <xmqqv8gxpd8r.fsf@gitster.g>

Junio C Hamano wrote:
> Sergey Organov <sorganov@gmail.com> writes:
> 
> > --patch. Thus, making --no-patch a synonym for -s was a mistake in the
> > first place that leaked through review process at that time, and
> >
> >    git show --format="%ad" --no-patch
> >
> > will still work the same way even if we fix --no-patch to disable
> > --patch only.
> 
> Not so fast.  I have a show.outputFormat configuration variable to
> teach builtin/log.c::show_setup_revisions_tweak() to tweak the
> hardcoded default from DIFF_FORMAT_PATCH to others (primarily
> because I often find myself doing "git show -p --stat").  Changing
> "--no-patch" to toggle only "--patch" away will close the door for
> future improvement like that, and "will still work" is an illusion.

So your rationale to reject a perfectly logical behavior that *everyone* agrees
with is that it might break a hypothetical patch?

Just do `--silent` instead.

> The user needs to be told that "--no-patch" no longer means "-s" and
> somebody needs to apologize to them that we are deliberately
> breaking their reliance they held for 10 years,

Nothing is broken.

And we never apologized for choosing the wrong default in `git pull`, did we?

> based on what we documented and prepare a smooth transition for them.

The transition is easy: just use `--silent` instead of `--no-patch`.

We could add a warning that these semantics will change in the future, instead
of just changing it right away.

But I bet very few people will see that warning (if any), because it makes
little sense to use `--no-patch` to turn off something other than `--patch`.

> Until the time when nobody uses "--no-patch" as a synonym for "-s" any
> longer, such a future improvement would be blocked.  And that is another
> reason why I want to be much more careful about "should --no-patch be changed
> to mean something other than -s" than "should -s be fixed not to be sticky
> for some but not all options".

> The latter is not a documented "feature" and it clearly was a bug that "-s
> --raw" did not honor "--raw".

Users can rely on what you call "bugs".

It's still a backwards-incompatible change for which you did not provide a
transitioning plan in [1].

Or is it a backwards-incompatible change *only* if the person proposing the
patch is somebody else other than the maintainer?

[1] https://lore.kernel.org/git/20230505165952.335256-1-gitster@pobox.com/

-- 
Felipe Contreras