Re: [PATCH] t4013: add expected failure for "log --patch --no-patch"

From: Junio C Hamano <gitster@pobox.com>
Date: May 4, 2023, 20:53 UTC
Message-ID: <xmqqpm7fizsl.fsf@gitster.g>
In-Reply-To: <87o7n03qgq.fsf@osv.gnss.ru>

Sergey Organov <sorganov@gmail.com> writes:

> [-s meaning "silent" at that time? If so, making --no-patch a synonym for
> "silent", and then documenting -s a synonym for --no-patch sounds like
> quite a twitch.]

I thought it stood for "squelch", but it turns out that it was introduced by f4f21ce3 (git-diff-tree: clean up output, 2005-05-06), as a "silent" mode, way before anything else in the thread has happened.

In Git v1.8.4, "--no-patch" was added as a synonym for "-s", and ever since we documented that they are equivalents.

Show 10 quoted lines

> Anyway, this seems pretty irrelevant to the test case. Even
> if we spell --no-patch as -s,
>
>  git diff -s --raw HEAD^ HEAD
>
> should produce what? To me it should be the same as
>
>  git diff --raw HEAD^ HEAD
>
> as -s turns off everything, and then --raw is turned on.

Ah, yeah, I missed that part of your sample command. It does seem quite puzzling, and the bad (or good?) part of the story is that my fresh build of v1.8.4 behaves exactly the same way. And with "-s" we can try versions before v1.8.4 (the only change that version made was to introduce "--no-patch" as its synonym). It seems it behaved that way at least back to v1.5.3 (which happens to be the oldest version I consider worth comparing with more modern versions).

Show 10 quoted lines

> Notice that
>
>  git diff -s --patch
>
> does produce the patch output, whereas
>
>  git diff -s --raw
>  git diff -s --stat
>
> produce none.

Yes, you're right.

> Sounds like nonsense.

Sounds like a bug to me. I wonder how it came about. Did we forget to add support of the equivalent of what "-s --patch" does, when we added "--raw" and "--stat", perhaps?

Show 10 quoted lines

> I'm afraid the solution I'd come up with won't be welcomed. If I'd start
> to "fix" it, it'd be likely set of independent options:
>
>  --patch --no-patch
>  --raw   --no-raw
>  --stat  --no-stat
>
> and then
>
>  -s being just a shortcut for "--no-raw --no-patch --no-stat"

If I were writing Git from scratch without any existing users, that would be how I would design it (modulo that I would make sure we have some mechanism to make it easier for developers who may add a new output <format> to ensure that "-s" also implies "--no-<format>" for the new <format> they are adding to the mix).

The fact that this wasn't brought up until now may mean that nobody would notice if we redefined the definition of "--no-patch" to behave that way, as long as "-s" keeps its original meaning.

I dunno.

Thanks.