On 2023-09-22 19:04, Junio C Hamano wrote:
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.
Makes sense to me, init_diffstat_widths(&rev.diffopt) fits better and
it's more descriptive. I'm a big fan of using self-descriptive naming,
but originally I wanted to follow the already present naming scheme, so
I'm glad to see that you asked for a more descriptive function name.
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.
Thanks.
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.
Makes sense to stick with the simpler variant, which won't cause
compatibility issues.
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".
On second thought, I agree that sticking with just "name" is better in
this test.
-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.
On second thought, I agree that using just "COLUMN" suffices while also
being more compact.
Same comment for "statNameWidth config"; with fewer number of bytes,
it would be more descriptive to say "diff.statNameWidth".
Oh, nice catch! Makes sense.
- 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.
Yes, I think that the improved consistency outweights the slight
redundancy.
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.
Thanks, I'll prepare and send v2 of the patch, based on your feedback.
