On 2023-09-12 01:12, Junio C Hamano wrote:
Dragan Simic <dsimic@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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.
