Re: [PATCH] diff --stat: add config option to limit filename width

> On 2023-09-12 01:12, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> Add new configuration option diff.statNameWidth=<width> that is 
>>> equivalent
>>> to the command-line option --stat-name-width=<width>, but it is 
>>> ignored
>>> by format-patch.  This follows the logic established by the already
>>> existing configuration option diff.statGraphWidth=<width>.
>>> 
>>> Limiting the widths of names and graphs in the --stat output makes 
>>> sense
>>> for interactive work on wide terminals with many columns, hence the 
>>> support
>>> for these configuration options.  They don't affect format-patch 
>>> because
>>> it already adheres to the traditional 80-column standard.
>>> 
>>> Update the documentation and add more tests to cover new 
>>> configuration
>>> option diff.statNameWidth=<width>.  While there, perform a few minor 
>>> code
>>> and whitespace cleanups here and there, as spotted.
>>> 
>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>>> ---
>> 
>> The stat lines have <width> (the entire display width),
>> <graph-width> (what appears after '|') and <name-width> (what
>> appears before '|'), so I would worry about letting users specify
>> all three to contradictory values, if there weren't an existing
>> command line option already.  But of course there already is a
>> command line option, so somebody more clever than me must have
>> thought about how to deal with such an impossible settings, and
>> adding a configuration variable to specify the same impossible
>> settings to the system would not make things worse.
> 
> Good point, but we're actually fine with adding diff.statNameWidth as
> a new configuration option, because the real troubles with
> contradictory configuration values might arise if we ever add
> diff.statWidth later.  However, we should still be fine at that point,
> because the code in diff.c, starting around the line #2730, performs a
> reasonable amount of sanity checks and value adjustments.
> 
> If we ever add diff.statWidth later, a good thing to do would be to
> emit warnings from the above-mentioned code in diff.c if it actually
> performs the adjustments, to make the users aware of the contradictory
> values.  I might even have a look at that separately, if you're fine
> with adding such warnings.

>>>  Documentation/config/diff.txt  |  4 ++++
>>>  Documentation/diff-options.txt | 17 +++++++-------
>>>  builtin/diff.c                 |  1 +
>>>  builtin/log.c                  |  1 +
>>>  builtin/merge.c                |  1 +
>>>  builtin/rebase.c               |  1 +
>> 
>> Someday, as a follow-up after the dust from this topic settles, we
>> would probably want to look at how these rev.diffopt.* members are
>> initialized and refactor the common code out to a helper.  It would
>> allow us to instead of doing this ...
> 
> Another good point.  If you agree, I'd prefer to have my patch
> accepted and merged as-is, after which I'll have a look into unifying
> the initialization of the rev.diffopt.* members.  Such an approach
> should, in general, also be better in case any regressions are
> detected at some point in the future.
> 
> I'll also have a look into the NEEDSWORK note in diff.c that asks for
> using utf8_strnwidth() to calculate the display width of line_prefix.
> I already had a brief look at that, and it seems that it leaves enough
> space for some rather nice related code cleanups.

>>>  	/* Set up defaults that will apply to both no-index and regular 
>>> diffs. */
>>>  	rev.diffopt.stat_width = -1;
>>> +	rev.diffopt.stat_name_width = -1;
>>>  	rev.diffopt.stat_graph_width = -1;
>>>  	rev.diffopt.flags.allow_external = 1;
>>>  	rev.diffopt.flags.allow_textconv = 1;
>> 
>> ... in many places, do so in a single place in the helper function,
>> and these places will just call the helper:
>> 
>> 	std_graph_options(&rev.diffopt);
>> 
>> I do not know offhand if "stat graph options related members" is a
>> good line to draw, or there are other things that are common outside
>> these .stat_foo members.  If the latter and the helper function will
>> initialize the members other than the stat-graph settings, its name
>> obviously needs a bit more thought, but you get the idae.
> 
> Sure, I'm willing to have a detailed look into all that, as I already
> described above.