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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2023, 16:53 UTC
Message-ID: <xmqqilcyyfly.fsf@gitster.g>
In-Reply-To: <ZFzMDCK8rprq6qUs@ncase>

Patrick Steinhardt <ps@pks.im> writes:

Show 16 quoted lines

> On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> 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.

Show 11 quoted lines

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