Subject: Re: [PATCH v5 0/9] fetch: introduce machine-parseable output

From: Junio C Hamano <[email-masked]>
Date: Thu, 11 May 2023 09:53:13 -0700

Patrick Steinhardt <[email-masked]> writes:

> On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <[email-masked]> writes:
> [snip]
>> >     - Patch 4/9: Reformulated the commit message to treat the missing
>> >       left-hand side of displayed references as an inconsistency instead
>> >       of a bug. I've also added a testcase to verify that direct OID
>> >       fetches continue to work as expected.
>> 
>> Again, the direct OID fetch is a good thing to test here.  I noticed
>> that the test added here insists that the standard output stream is
>> empty when the command errors out, which is not consistent with [3/9]
>> above.
>
> I think you misread the test: yes, we do test stdout and stderr
> separately and in many cases assert that stdout is in fact empty. But
> none of the added tests are about failing commands.

Ah, you are absolutely right. Sorry for the noise---I do not have an
objection to ensure that program output in the successful case is
predictable.  My main concern is to avoid giving unneeded assurance
in the failing case.

>> assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could
>> lose the value from the enum, I think.  The defensive switch
>> statement that has BUG() to notice an erroneous caller that pass
>> values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good
>> idea.
>
> Ah, right, `DISPLAY_FORMAT_UNKNOWN` isn't really needed anymore. I think
> it's still good to have valid values of the enum start with `1` so that
> it becomes easier to detect cases where we accidentally use a default
> initialized variable. But that can be achieved without giving the
> default value an explicit name.

Unless we explicitly take advantage of UNKNOWN being 0 and perform
clever defaulting, the result is far safer if you get rid of it, and
instead give 0 to a real choice that is used as the default, for two
reasons: (1) naturally 0 initialization work fine, (2) we have one
less enum constant people (or editor's auto-completer) assign to a
variable by mistake instead of a real one they intended to.  I agree
with the last sentence of your paragraph above.