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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2023, 23:12 UTC
Message-ID: <xmqqil8gs3s0.fsf@gitster.g>
In-Reply-To: <87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org>

Dragan Simic <dsimic@manjaro.org> writes:

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Thanks.