Re: [PATCH] diff --stat: set the width defaults in a helper function

> 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.

>> 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.

>> -	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.