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. So given that:

    - The added tests explicitly are about verifying the output format.

    - The distinction between stdout and stderr matters.

    - The distinction matters even more with the addition of
      `--porcelain`.

I think that explicitly verifing both output streams is the correct
thing to do.

> >     - Patch 9/9: The `--porcelain` option is now a simple `OPT_BOOL()`
> >       that can be negated. Added a test that `--no-porcelain` works as
> >       expected.
> 
> OK, this time the familiar "prepare a variable to its default, let
> config callback to overwrite it by reading configuration variables,
> and then let the command line option override it" is used and the
> result easy to follow.  I do not think .display_format is never
> 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.

I'll refrain from sending a new version just to remove this constant as
it doesn't really feel worth it, though. But please, let me know in case
you disagree and I'll send an updated version.

Thanks
Patrick