{"thread":{"id":"60222","subject":"[PATCH] diff --stat: add config option to limit filename width","startedAt":"2023-09-11T21:39:03Z","lastAt":"2023-09-19T01:27:13Z","messageCount":8,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481690","messageId":"87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org","threadId":"60222","inReplyTo":null,"subject":"[PATCH] diff --stat: add config option to limit filename width","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-11T15:39:44Z","receivedAt":"2023-09-11T21:39:03Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Add new configuration option diff.statNameWidth= that is equivalent\nto the command-line option --stat-name-width=, but it is ignored\nby format-patch. This follows the logic established by the already\nexisting configuration option diff.statGraphWidth=.\n\nLimiting the widths of names and graphs in the --stat output makes sense\nfor interactive work on wide terminals with many columns, hence the support\nfor these configuration options. They don't affect format-patch because\nit already adheres to the traditional 80-column standard.\n\nUpdate the documentation and add more tests to cover new configuration\noption diff.statNameWidth=. While there, perform a few minor code\nand whitespace cleanups here and there, as spotted.\n\nSigned-off-by: Dragan Simic \n---\n Documentation/config/diff.txt | 4 ++++\n Documentation/diff-options.txt | 17 +++++++-------\n builtin/diff.c | 1 +\n builtin/log.c | 1 +\n builtin/merge.c | 1 +\n builtin/rebase.c | 1 +\n diff.c | 11 +++++++--\n graph.c | 1 -\n t/t4052-stat-output.sh | 41 ++++++++++++++++++++++++++++------\n 9 files changed, 60 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\nindex 35a7bf86d7..9391c77e55 100644\n--- a/Documentation/config/diff.txt\n+++ b/Documentation/config/diff.txt\n@@ -52,6 +52,10 @@ directories with less than 10% of the total amount of changed files,\n and accumulating child directory counts in the parent directories:\n `files,10,cumulative`.\n \n+diff.statNameWidth::\n+\tLimit the width of the filename part in --stat output. If set, applies\n+\tto all commands generating --stat output except format-patch.\n+\n diff.statGraphWidth::\n \tLimit the width of the graph part in --stat output. If set, applies\n \tto all commands generating --stat output except format-patch.\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 9f33f88771..6603470dbe 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -210,14 +210,15 @@ have to use `--diff-algorithm=default` option.\n \tpart. Maximum width defaults to terminal width, or 80 columns\n \tif not connected to a terminal, and can be overridden by\n \t``. The width of the filename part can be limited by\n-\tgiving another width `` after a comma. The width\n-\tof the graph part can be limited by using\n-\t`--stat-graph-width=` (affects all commands generating\n-\ta stat graph) or by setting `diff.statGraphWidth=`\n-\t(does not affect `git format-patch`).\n-\tBy giving a third parameter ``, you can limit the\n-\toutput to the first `` lines, followed by `...` if\n-\tthere are more.\n+\tgiving another width `` after a comma or by setting\n+\t`diff.statNameWidth=`. The width of the graph part can be\n+\tlimited by using `--stat-graph-width=` or by setting\n+\t`diff.statGraphWidth=`. Using `--stat` or\n+\t`--stat-graph-width` affects all commands generating a stat graph,\n+\twhile setting `diff.statNameWidth` or `diff.statGraphWidth`\n+\tdoes not affect `git format-patch`.\n+\tBy giving a third parameter ``, you can limit the output to\n+\tthe first `` lines, followed by `...` if there are more.\n +\n These parameters can also be set individually with `--stat-width=`,\n `--stat-name-width=` and `--stat-count=`.\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0b313549c7..c0f564273a 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -475,6 +475,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \t/* Set up defaults that will apply to both no-index and regular diffs. */\n \trev.diffopt.stat_width = -1;\n+\trev.diffopt.stat_name_width = -1;\n \trev.diffopt.stat_graph_width = -1;\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 87088077d9..2901514778 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -175,6 +175,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \trev->verbose_header = 1;\n \trev->diffopt.flags.recursive = 1;\n \trev->diffopt.stat_width = -1; /* use full terminal width */\n+\trev->diffopt.stat_name_width = -1; /* respect statNameWidth config */\n \trev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config */\n \trev->abbrev_commit = default_abbrev_commit;\n \trev->show_root_diff = default_show_root;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex de68910177..fd6942d0ef 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -488,6 +488,7 @@ static void finish(struct commit *head_commit,\n \t\tstruct diff_options opts;\n \t\trepo_diff_setup(the_repository, &opts);\n \t\topts.stat_width = -1; /* use full terminal width */\n+\t\topts.stat_name_width = -1; /* respect statNameWidth config */\n \t\topts.stat_graph_width = -1; /* respect statGraphWidth config */\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..ed15accec9 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1804,6 +1804,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t/* We want color (if set), but no pager */\n \t\trepo_diff_setup(the_repository, &opts);\n \t\topts.stat_width = -1; /* use full terminal width */\n+\t\topts.stat_name_width = -1; /* respect statNameWidth config */\n \t\topts.stat_graph_width = -1; /* respect statGraphWidth config */\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\ndiff --git a/diff.c b/diff.c\nindex bccb018da4..353e3b2cc9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -65,6 +65,7 @@ int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n static int diff_relative;\n+static int diff_stat_name_width;\n static int diff_stat_graph_width;\n static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n@@ -410,6 +411,10 @@ int git_diff_ui_config(const char *var, const char *value,\n \t\tdiff_relative = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.statnamewidth\")) {\n+\t\tdiff_stat_name_width = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.statgraphwidth\")) {\n \t\tdiff_stat_graph_width = git_config_int(var, value, ctx->kvi);\n \t\treturn 0;\n@@ -2704,12 +2709,14 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tnumber_width = decimal_width(max_change) > number_width ?\n \t\tdecimal_width(max_change) : number_width;\n \n+\tif (options->stat_name_width == -1)\n+\t\toptions->stat_name_width = diff_stat_name_width;\n \tif (options->stat_graph_width == -1)\n \t\toptions->stat_graph_width = diff_stat_graph_width;\n \n \t/*\n-\t * Guarantee 3/8*16==6 for the graph part\n-\t * and 5/8*16==10 for the filename part\n+\t * Guarantee 3/8*16 == 6 for the graph part\n+\t * and 5/8*16 == 10 for the filename part\n \t */\n \tif (width < 16 + 6 + number_width)\n \t\twidth = 16 + 6 + number_width;\ndiff --git a/graph.c b/graph.c\nindex 2a9dc430fa..1ca34770ee 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -339,7 +339,6 @@ void graph_setup_line_prefix(struct diff_options *diffopt)\n \t\tdiffopt->output_prefix = diff_output_prefix_callback;\n }\n \n-\n struct git_graph *graph_init(struct rev_info *opt)\n {\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 3ee27e277d..beb2ec2a55 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -49,34 +49,63 @@ log -1 --stat\n EOF\n \n cat >expect.60 <<-'EOF'\n- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n EOF\n cat >expect.6030 <<-'EOF'\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n EOF\n-cat >expect2.60 <<-'EOF'\n+while read verb expect cmd args\n+do\n+\t# No width limit applied when statNameWidth is ignored\n+\tcase \"$expect\" in expect72|expect.6030)\n+\t\ttest_expect_success \"$cmd $verb statNameWidth config with long name\" '\n+\t\t\tgit -c diff.statNameWidth=30 $cmd $args >output &&\n+\t\t\tgrep \" | \" output >actual &&\n+\t\t\ttest_cmp $expect actual\n+\t\t';;\n+\tesac\n+\t# Maximum width limit still applied when statNameWidth is ignored\n+\tcase \"$expect\" in expect.60|expect.6030)\n+\t\ttest_expect_success \"$cmd --stat=width $verb statNameWidth config with long name\" '\n+\t\t\tgit -c diff.statNameWidth=30 $cmd $args --stat=60 >output &&\n+\t\t\tgrep \" | \" output >actual &&\n+\t\t\ttest_cmp $expect actual\n+\t\t';;\n+\tesac\n+done <<\\EOF\n+ignores expect72 format-patch -1 --stdout\n+ignores expect.60 format-patch -1 --stdout\n+respects expect.6030 diff HEAD^ HEAD --stat\n+respects expect.6030 show --stat\n+respects expect.6030 log -1 --stat\n+EOF\n+\n+cat >expect.40 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+cat >expect2.40 <<-'EOF'\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n EOF\n cat >expect2.6030 <<-'EOF'\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n EOF\n while read expect cmd args\n do\n \ttest_expect_success \"$cmd --stat=width: a long name is given more room when the bar is short\" '\n \t\tgit $cmd $args --stat=40 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp $expect.60 actual\n+\t\ttest_cmp $expect.40 actual\n \t'\n \n \ttest_expect_success \"$cmd --stat-width=width with long name\" '\n \t\tgit $cmd $args --stat-width=40 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp $expect.60 actual\n+\t\ttest_cmp $expect.40 actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat=...,name-width with long name\" '\n+\ttest_expect_success \"$cmd --stat=width,name-width with long name\" '\n \t\tgit $cmd $args --stat=60,30 >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp $expect.6030 actual\n@@ -94,7 +123,6 @@ expect show --stat\n expect log -1 --stat\n EOF\n \n-\n test_expect_success 'preparation for big change tests' '\n \t>abcd &&\n \tgit add abcd &&\n@@ -207,7 +235,6 @@ respects expect40 show --stat\n respects expect40 log -1 --stat\n EOF\n \n-\n cat >expect <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++\n EOF\n"},{"id":"481727","messageId":"xmqqil8gs3s0.fsf@gitster.g","threadId":"60222","inReplyTo":"87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-11T23:12:31Z","receivedAt":"2023-09-12T01:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Add new configuration option diff.statNameWidth= that is equivalent\n> to the command-line option --stat-name-width=, but it is ignored\n> by format-patch. This follows the logic established by the already\n> existing configuration option diff.statGraphWidth=.\n>\n> Limiting the widths of names and graphs in the --stat output makes sense\n> for interactive work on wide terminals with many columns, hence the support\n> for these configuration options. They don't affect format-patch because\n> it already adheres to the traditional 80-column standard.\n>\n> Update the documentation and add more tests to cover new configuration\n> option diff.statNameWidth=. While there, perform a few minor code\n> and whitespace cleanups here and there, as spotted.\n>\n> Signed-off-by: Dragan Simic \n> ---\n\nThe stat lines have (the entire display width),\n (what appears after '|') and (what\nappears before '|'), so I would worry about letting users specify\nall three to contradictory values, if there weren't an existing\ncommand line option already. But of course there already is a\ncommand line option, so somebody more clever than me must have\nthought about how to deal with such an impossible settings, and\nadding a configuration variable to specify the same impossible\nsettings to the system would not make things worse.\n\n> Documentation/config/diff.txt | 4 ++++\n> Documentation/diff-options.txt | 17 +++++++-------\n> builtin/diff.c | 1 +\n> builtin/log.c | 1 +\n> builtin/merge.c | 1 +\n> builtin/rebase.c | 1 +\n\nSomeday, as a follow-up after the dust from this topic settles, we\nwould probably want to look at how these rev.diffopt.* members are\ninitialized and refactor the common code out to a helper. It would\nallow us to instead of doing this ...\n\n> \t/* Set up defaults that will apply to both no-index and regular diffs. */\n> \trev.diffopt.stat_width = -1;\n> +\trev.diffopt.stat_name_width = -1;\n> \trev.diffopt.stat_graph_width = -1;\n> \trev.diffopt.flags.allow_external = 1;\n> \trev.diffopt.flags.allow_textconv = 1;\n\n... in many places, do so in a single place in the helper function,\nand these places will just call the helper:\n\n\tstd_graph_options(&rev.diffopt);\n\nI do not know offhand if \"stat graph options related members\" is a\ngood line to draw, or there are other things that are common outside\nthese .stat_foo members. If the latter and the helper function will\ninitialize the members other than the stat-graph settings, its name\nobviously needs a bit more thought, but you get the idae.\n\nThanks.\n"},{"id":"481739","messageId":"487bd30e5a4cdcea8697393eb36ce3f3@manjaro.org","threadId":"60222","inReplyTo":"xmqqil8gs3s0.fsf@gitster.g","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-12T02:11:28Z","receivedAt":"2023-09-12T04:27:53Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-09-12 01:12, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Add new configuration option diff.statNameWidth= that is \n>> equivalent\n>> to the command-line option --stat-name-width=, but it is \n>> ignored\n>> by format-patch. This follows the logic established by the already\n>> existing configuration option diff.statGraphWidth=.\n>> \n>> Limiting the widths of names and graphs in the --stat output makes \n>> sense\n>> for interactive work on wide terminals with many columns, hence the \n>> support\n>> for these configuration options. They don't affect format-patch \n>> because\n>> it already adheres to the traditional 80-column standard.\n>> \n>> Update the documentation and add more tests to cover new configuration\n>> option diff.statNameWidth=. While there, perform a few minor \n>> code\n>> and whitespace cleanups here and there, as spotted.\n>> \n>> Signed-off-by: Dragan Simic \n>> ---\n> \n> The stat lines have (the entire display width),\n> (what appears after '|') and (what\n> appears before '|'), so I would worry about letting users specify\n> all three to contradictory values, if there weren't an existing\n> command line option already. But of course there already is a\n> command line option, so somebody more clever than me must have\n> thought about how to deal with such an impossible settings, and\n> adding a configuration variable to specify the same impossible\n> settings to the system would not make things worse.\n\nGood point, but we're actually fine with adding diff.statNameWidth as a \nnew configuration option, because the real troubles with contradictory \nconfiguration values might arise if we ever add diff.statWidth later. \nHowever, we should still be fine at that point, because the code in \ndiff.c, starting around the line #2730, performs a reasonable amount of \nsanity checks and value adjustments.\n\nIf we ever add diff.statWidth later, a good thing to do would be to emit \nwarnings from the above-mentioned code in diff.c if it actually performs \nthe adjustments, to make the users aware of the contradictory values. I \nmight even have a look at that separately, if you're fine with adding \nsuch warnings.\n\n>> Documentation/config/diff.txt | 4 ++++\n>> Documentation/diff-options.txt | 17 +++++++-------\n>> builtin/diff.c | 1 +\n>> builtin/log.c | 1 +\n>> builtin/merge.c | 1 +\n>> builtin/rebase.c | 1 +\n> \n> Someday, as a follow-up after the dust from this topic settles, we\n> would probably want to look at how these rev.diffopt.* members are\n> initialized and refactor the common code out to a helper. It would\n> allow us to instead of doing this ...\n\nAnother good point. If you agree, I'd prefer to have my patch accepted \nand merged as-is, after which I'll have a look into unifying the \ninitialization of the rev.diffopt.* members. Such an approach should, \nin general, also be better in case any regressions are detected at some \npoint in the future.\n\nI'll also have a look into the NEEDSWORK note in diff.c that asks for \nusing utf8_strnwidth() to calculate the display width of line_prefix. I \nalready had a brief look at that, and it seems that it leaves enough \nspace for some rather nice related code cleanups.\n\n>> \t/* Set up defaults that will apply to both no-index and regular \n>> diffs. */\n>> \trev.diffopt.stat_width = -1;\n>> +\trev.diffopt.stat_name_width = -1;\n>> \trev.diffopt.stat_graph_width = -1;\n>> \trev.diffopt.flags.allow_external = 1;\n>> \trev.diffopt.flags.allow_textconv = 1;\n> \n> ... in many places, do so in a single place in the helper function,\n> and these places will just call the helper:\n> \n> \tstd_graph_options(&rev.diffopt);\n> \n> I do not know offhand if \"stat graph options related members\" is a\n> good line to draw, or there are other things that are common outside\n> these .stat_foo members. If the latter and the helper function will\n> initialize the members other than the stat-graph settings, its name\n> obviously needs a bit more thought, but you get the idae.\n\nSure, I'm willing to have a detailed look into all that, as I already \ndescribed above.\n"},{"id":"481776","messageId":"xmqqbke7pb8z.fsf@gitster.g","threadId":"60222","inReplyTo":"487bd30e5a4cdcea8697393eb36ce3f3@manjaro.org","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-12T17:11:40Z","receivedAt":"2023-09-12T17:11:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n>> Someday, as a follow-up after the dust from this topic settles, we\n>> would probably want to look at how these rev.diffopt.* members are\n>> initialized and refactor the common code out to a helper. It would\n>> allow us to instead of doing this ...\n>\n> Another good point. If you agree, I'd prefer to have my patch\n> accepted and merged as-is, ...\n\nThat is exactly what I meant by \"follow-up after the dust settles\".\n\nAll of the \"we should probably do this and that in the future\" are\ncalled #leftoverbits comments. Food for thought, something people\ncan use to find inspiration for areas they may want to work on, that\nhas no impact to how \"complete\" the current patch being discussed\nis.\n"},{"id":"481782","messageId":"6d286618ef9ad3824c4c40211557bfe7@manjaro.org","threadId":"60222","inReplyTo":"xmqqbke7pb8z.fsf@gitster.g","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-12T17:48:59Z","receivedAt":"2023-09-12T17:49:04Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-09-12 19:11, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>>> Someday, as a follow-up after the dust from this topic settles, we\n>>> would probably want to look at how these rev.diffopt.* members are\n>>> initialized and refactor the common code out to a helper. It would\n>>> allow us to instead of doing this ...\n>> \n>> Another good point. If you agree, I'd prefer to have my patch\n>> accepted and merged as-is, ...\n> \n> That is exactly what I meant by \"follow-up after the dust settles\".\n> \n> All of the \"we should probably do this and that in the future\" are\n> called #leftoverbits comments. Food for thought, something people\n> can use to find inspiration for areas they may want to work on, that\n> has no impact to how \"complete\" the current patch being discussed\n> is.\n\nSounds great, thank you. I already have a couple of code cleanups to \nwork on, which I'll do in the following days and send the patches.\n"},{"id":"481901","messageId":"7aceb7db8d3f4b569564ffd9d1e2e368@manjaro.org","threadId":"60222","inReplyTo":"487bd30e5a4cdcea8697393eb36ce3f3@manjaro.org","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-16T02:09:39Z","receivedAt":"2023-09-16T02:10:43Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-09-12 04:11, Dragan Simic wrote:\n> On 2023-09-12 01:12, Junio C Hamano wrote:\n>> Dragan Simic writes:\n>> \n>>> Add new configuration option diff.statNameWidth= that is \n>>> equivalent\n>>> to the command-line option --stat-name-width=, but it is \n>>> ignored\n>>> by format-patch. This follows the logic established by the already\n>>> existing configuration option diff.statGraphWidth=.\n>>> \n>>> Limiting the widths of names and graphs in the --stat output makes \n>>> sense\n>>> for interactive work on wide terminals with many columns, hence the \n>>> support\n>>> for these configuration options. They don't affect format-patch \n>>> because\n>>> it already adheres to the traditional 80-column standard.\n>>> \n>>> Update the documentation and add more tests to cover new \n>>> configuration\n>>> option diff.statNameWidth=. While there, perform a few minor \n>>> code\n>>> and whitespace cleanups here and there, as spotted.\n>>> \n>>> Signed-off-by: Dragan Simic \n>>> ---\n>> \n>> The stat lines have (the entire display width),\n>> (what appears after '|') and (what\n>> appears before '|'), so I would worry about letting users specify\n>> all three to contradictory values, if there weren't an existing\n>> command line option already. But of course there already is a\n>> command line option, so somebody more clever than me must have\n>> thought about how to deal with such an impossible settings, and\n>> adding a configuration variable to specify the same impossible\n>> settings to the system would not make things worse.\n> \n> Good point, but we're actually fine with adding diff.statNameWidth as\n> a new configuration option, because the real troubles with\n> contradictory configuration values might arise if we ever add\n> diff.statWidth later. However, we should still be fine at that point,\n> because the code in diff.c, starting around the line #2730, performs a\n> reasonable amount of sanity checks and value adjustments.\n> \n> If we ever add diff.statWidth later, a good thing to do would be to\n> emit warnings from the above-mentioned code in diff.c if it actually\n> performs the adjustments, to make the users aware of the contradictory\n> values. I might even have a look at that separately, if you're fine\n> with adding such warnings.\n\nJust checking, do you want me to perform any improvements to this patch, \nso you can have it pulled into one of your trees?\n\nI'll start working on a patch that adds the above-mentioned warnings, \nbut having those implemented properly and hashed out will surely take a \nfair amount of time. However, those warnings should be quite usable, if \nyou agree, although they're not critical at the moment.\n\n>>> Documentation/config/diff.txt | 4 ++++\n>>> Documentation/diff-options.txt | 17 +++++++-------\n>>> builtin/diff.c | 1 +\n>>> builtin/log.c | 1 +\n>>> builtin/merge.c | 1 +\n>>> builtin/rebase.c | 1 +\n>> \n>> Someday, as a follow-up after the dust from this topic settles, we\n>> would probably want to look at how these rev.diffopt.* members are\n>> initialized and refactor the common code out to a helper. It would\n>> allow us to instead of doing this ...\n> \n> Another good point. If you agree, I'd prefer to have my patch\n> accepted and merged as-is, after which I'll have a look into unifying\n> the initialization of the rev.diffopt.* members. Such an approach\n> should, in general, also be better in case any regressions are\n> detected at some point in the future.\n> \n> I'll also have a look into the NEEDSWORK note in diff.c that asks for\n> using utf8_strnwidth() to calculate the display width of line_prefix.\n> I already had a brief look at that, and it seems that it leaves enough\n> space for some rather nice related code cleanups.\n\nI'll also start working on these patches in the next few days, which \nshould result in some rather nice code cleanups, AFAICT so far.\n\n>>> \t/* Set up defaults that will apply to both no-index and regular \n>>> diffs. */\n>>> \trev.diffopt.stat_width = -1;\n>>> +\trev.diffopt.stat_name_width = -1;\n>>> \trev.diffopt.stat_graph_width = -1;\n>>> \trev.diffopt.flags.allow_external = 1;\n>>> \trev.diffopt.flags.allow_textconv = 1;\n>> \n>> ... in many places, do so in a single place in the helper function,\n>> and these places will just call the helper:\n>> \n>> \tstd_graph_options(&rev.diffopt);\n>> \n>> I do not know offhand if \"stat graph options related members\" is a\n>> good line to draw, or there are other things that are common outside\n>> these .stat_foo members. If the latter and the helper function will\n>> initialize the members other than the stat-graph settings, its name\n>> obviously needs a bit more thought, but you get the idae.\n> \n> Sure, I'm willing to have a detailed look into all that, as I already\n> described above.\n"},{"id":"481964","messageId":"xmqqttrrl9m2.fsf@gitster.g","threadId":"60222","inReplyTo":"7aceb7db8d3f4b569564ffd9d1e2e368@manjaro.org","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T16:38:45Z","receivedAt":"2023-09-18T16:46:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Just checking, do you want me to perform any improvements to this\n> patch, so you can have it pulled into one of your trees?\n\nI do not think of any outstanding issues I spotted on this change.\nThanks for pinging.\n"},{"id":"482000","messageId":"7da6c725af777a4859a99a679ac6ac32@manjaro.org","threadId":"60222","inReplyTo":"xmqqttrrl9m2.fsf@gitster.g","subject":"Re: [PATCH] diff --stat: add config option to limit filename width","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-19T01:27:07Z","receivedAt":"2023-09-19T01:27:13Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-09-18 18:38, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> Just checking, do you want me to perform any improvements to this\n>> patch, so you can have it pulled into one of your trees?\n> \n> I do not think of any outstanding issues I spotted on this change.\n> Thanks for pinging.\n\nGreat, thanks.\n"}]}