Re: [PATCH] diff: fix behaviour of the "-s" option

From: Junio C Hamano <gitster@pobox.com>
Date: May 5, 2023, 16:51 UTC
Message-ID: <xmqqlei2en6t.fsf@gitster.g>
In-Reply-To: <xmqq4joribyv.fsf@gitster.g>

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

Show 12 quoted lines

> ...  I think the "historical
> reasons" why we did not do that was because we wanted to be able to
> do a flexible defaulting. ...
> This would almost work, except that it would
> make it hard to tell "no command line options" case and "'-s' cleared
> all bits" case apart (the former wants to default to "--patch",
> while the latter wants to stay "no output"), and it probably was the
> reason why we gave an extra NO_OUTPUT bit to the "-s" option.  In
> hindsight, the arrangement certainly made other things harder and
> prone to unnecessary bugs.
>
> Anyway...

The distinction between the presense of NO_OUTPUT bit and absolutely empty output_format word indeed is used by "git show", in the builtin/log.c::show_setup_revisions_tweak() function.

We could lose DIFF_FORMAT_NO_OUTPUT bit, but then we need to replace it with something else (i.e. DIFF_FORMAT_OPTION_GIVEN bit), and

 * "--patch", "--raw", etc. will set DIFF_FORMAT_$format bit and
   DIFF_FORMAT_OPTION_GIVEN bit on for each format.  "--no-raw", 
   etc. will set off DIFF_FORMAT_$format bit but still record the
   fact that we saw an option from the command line by setting
   DIFF_FORMAT_OPTION_GIVEN bit.

 * "-s" (and its synonym "--no-patch") will set the
   DIFF_FORMAT_OPTION_GIVEN bit on, and clear all other bits.

which I suspect would make the code much cleaner without breaking any end-user expectations.

Once that is in place, transitioning "--no-patch" to mean the counterpart of "--patch", just like "--no-raw" only defeats an earlier "--raw", would be quite simple at the code level. The social cost of migrating the end-user expectations might be too great for it to be worth, but at least the "GIVEN" bit clean-up alone may be worth it.

Not that I would be starting the process right away...