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