Re: [PATCH] diff --stat: set the width defaults in a helper function
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 22, 2023, 17:04 UTC
- Message-ID
- <xmqqjzsi2l7r.fsf@gitster.g>
- In-Reply-To
- <166396f0a98e248fc3d1236757632c5d648ddc0b.1695364961.git.dsimic@manjaro.org>
Dragan Simic <dsimic@manjaro.org> 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.
Show 5 quoted lines
> 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.
Show 13 quoted lines
> 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") ...
Show 5 quoted lines
> 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.