Patrick Steinhardt <ps@xxxxxx> writes:
> - Patch 3/9: Added a test to verify that `git fetch -c fetch.output`
> without a value set fails as expected. Also dropped the tests that
> checked whether stdout was empty.
Thank you for the attention to this small detail of valueless
configuration variable definition.
> - 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.
> - Patch 5/9: New patch that makes calculation of the table width for
> displayed reference updates self-contained in `refcol_width()`.
> This is a preparatory refactoring that should make patch 6/9
> easier to review.
It is an excellent idea to avoid calls to refcol_width() for each
ref that gets shown and make the helper responsible for computing
the required maxwidth. The result indeed has become easier to
follow as you mentioned in your response to my review on the
previous round.
> - Patch 7/9: Refactored the code to parse the "fetch.output" config
> variable inside of `git_fetch_config()` before we parse command
> line options. Also fixed that the commit message was still
> referring to `--output-format=porcelain` instead of the new
> `--porcelain` switch.
As a standalone step, it leaves an impression that the step makes
the way we handle the output-format configuration variable
inconsistent with the way we handle the other configuration
variables, but I think it is a good place to stop for the purpose of
this topic. It lays a good foundation for future clean-up after the
dust settles from this topic---we may want to move global variables
assigned in the git_fetch_config() into the fetch_config structure.
> - 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.
Looking good.
Thanks.
