Dragan Simic <dsimic@xxxxxxxxxxx> writes:
> Extract the commonly used initialization of the --stat-width=<width>,
> --stat-name-width=<width> and --stat-graph-with=<width> parameters to the
> internal default values into a helper function, to avoid repeating the same
> initialization code in a few places.
Thanks.
If this is only about settings related to controlling widths of
various elements on diffstat lines, isn't the "std" in name a bit
too broad, though? init_diffstat_widths(&rev.diffopt) or something
like that might be a better fit. I dunno if it is a huge deal,
though.
> Add a couple of tests to additionally cover existing configuration options
> diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by
> git-merge to generate --stat outputs. This closes the gap in the tests that
> existed previously for the --stat tests, reducing the chances for having
> any regressions introduced by this commit.
Nice.
> While there, perform a bunch of small wording improvements and some minor
> code cleanups in the improved unit test, as spotted, to make it a bit neater
> and to improve its test-level consistency.
Alright. The last category of changes need somebody else to review
them in addition to myself, as I expect that it would be somewhat
subjective and I tend to be change-averse.
The code changes all looked sensible.
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index beb2ec2a55..aa947d93cf 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-terminal.sh
>
> -# 120 character name
> -name=aaaaaaaaaa
> -name=$name$name$name$name$name$name$name$name$name$name$name$name
> +# 120-character name
> +printf -v name 'a%.0s' {1..120}
This is a totally unnecessary and unacceptable change. "-v name"
may be available in the built-in variant found in bash, but you
would likely find that it is missing from other shells. {1..120} is
also a bash-ism.
And because we are still calling the result a "name" (not
"filename") ...
> cat >expect72 <<-'EOF'
> ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
> EOF
> -test_expect_success "format-patch: small change with long name gives more space to the name" '
> +test_expect_success "format-patch: small change with long filename gives more space to the filename" '
... I do not see the point of this change (and similar ones in the
rest of the patch). Even the configuration is called statNameWidth
and not statFileNameWidth. In the context of the tests that check
"stat-output" (that is in the filename of this script), we should be
able to use "name" consistently without causing any confusion, as it
is unlikely to be mistaken with other kinds of "name".
> -test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" '
> +test_expect_success "format-patch --cover-letter ignores COLUMNS envvar with big change" '
Not wrong per-se, but I wonder if it is necessary to stress that
COLUMNS is an environment variable that tells the programs how wide
a terminal they are showing their output. A usual shell variable
would not affect the "git" process it runs, and COLUMNS without any
dot in it cannot be our configuration variable, so even without deep
knowledge of tradition, I thought it would be rather obvious.
Same comment for "statNameWidth config"; with fewer number of bytes,
it would be more descriptive to say "diff.statNameWidth".
> - test_expect_success "$cmd --stat-graph-width with big change" '
> + test_expect_success "$cmd --stat-graph-width=width with big change" '
> git $cmd $args --stat-graph-width=26 >output &&
This may be a good change, especially if there are tests that feed
different parameters and if it helps clarifying which variant is
tested, e.g. "--stat=<width>,<name-width>" vs "--stat=<width>".
Ah, wait, "--stat-graph-width" always takes a single value, so the
above justification does not quite apply. But still, it is not
making it worse, and because there is another test that is labeled
with "--stat-width=width", being consistent with it has value.
OK.
> - test_expect_success "$cmd --stat-graph-width --graph with big change" '
> + test_expect_success "$cmd --stat-graph-width=width --graph with big change" '
Ditto.
Thanks.
