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

Patrick Steinhardt [off-list ref] writes:

On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote:

quoted

Patrick Steinhardt [off-list ref] writes:

[snip]

quoted

quoted

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

quoted

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.