{"thread":{"id":"60254","subject":"[PATCH] diff --stat: set the width defaults in a helper function","startedAt":"2023-09-22T06:44:27Z","lastAt":"2023-09-22T17:54:04Z","messageCount":3,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482124","messageId":"166396f0a98e248fc3d1236757632c5d648ddc0b.1695364961.git.dsimic@manjaro.org","threadId":"60254","inReplyTo":null,"subject":"[PATCH] diff --stat: set the width defaults in a helper function","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-22T06:44:18Z","receivedAt":"2023-09-22T06:44:27Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Extract the commonly used initialization of the --stat-width=<width>,\n--stat-name-width=<width> and --stat-graph-with=<width> parameters to the\ninternal default values into a helper function, to avoid repeating the same\ninitialization code in a few places.\n\nAdd a couple of tests to additionally cover existing configuration options\ndiff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by\ngit-merge to generate --stat outputs.  This closes the gap in the tests that\nexisted previously for the --stat tests, reducing the chances for having\nany regressions introduced by this commit.\n\nWhile there, perform a bunch of small wording improvements and some minor\ncode cleanups in the improved unit test, as spotted, to make it a bit neater\nand to improve its test-level consistency.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n builtin/diff.c         |  4 +--\n builtin/log.c          |  6 ++--\n builtin/merge.c        |  4 +--\n builtin/rebase.c       |  4 +--\n diff.c                 |  7 +++++\n diff.h                 |  1 +\n t/t4052-stat-output.sh | 65 ++++++++++++++++++++++++++----------------\n 7 files changed, 53 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex c0f564273a..041559d6fb 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -474,9 +474,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \trepo_init_revisions(the_repository, &rev, 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+\tdiffstat_std(&rev.diffopt);\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 80e1be1645..f15401e966 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -176,17 +176,15 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \tif (default_follow)\n \t\trev->diffopt.flags.default_follow_renames = 1;\n \trev->verbose_header = 1;\n+\tdiffstat_std(&rev->diffopt);\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->diffopt.flags.allow_textconv = 1;\n \trev->abbrev_commit = default_abbrev_commit;\n \trev->show_root_diff = default_show_root;\n \trev->subject_prefix = fmt_patch_subject_prefix;\n \trev->patch_name_max = fmt_patch_name_max;\n \trev->show_signature = default_show_signature;\n \trev->encode_email_headers = default_encode_email_headers;\n-\trev->diffopt.flags.allow_textconv = 1;\n \n \tif (default_date_mode)\n \t\tparse_date_format(default_date_mode, &rev->date_mode);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex fd21c0d4f4..4120ec2dff 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -466,9 +466,7 @@ static void finish(struct commit *head_commit,\n \tif (new_head && show_diffstat) {\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\tdiffstat_std(&opts);\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b93dca95a6..ae85bc992b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1806,9 +1806,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \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\tdiffstat_std(&opts);\n \t\topts.output_format |=\n \t\t\tDIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n \t\topts.detect_rename = DIFF_DETECT_RENAME;\ndiff --git a/diff.c b/diff.c\nindex 353e3b2cc9..845e7f4e84 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6936,6 +6936,13 @@ void diff_queued_diff_prefetch(void *repository)\n \toid_array_clear(&to_fetch);\n }\n \n+void diffstat_std(struct diff_options *options)\n+{\n+\toptions->stat_width = -1;        /* use full terminal width */\n+\toptions->stat_name_width = -1;   /* respect diff.statNameWidth config */\n+\toptions->stat_graph_width = -1;  /* respect diff.statGraphWidth config */\n+}\n+\n void diffcore_std(struct diff_options *options)\n {\n \tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\ndiff --git a/diff.h b/diff.h\nindex caf1528bf0..9a64c53f5f 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -573,6 +573,7 @@ int git_config_rename(const char *var, const char *value);\n \n #define DIFF_PICKAXE_IGNORE_CASE\t32\n \n+void diffstat_std(struct diff_options *);\n void diffcore_std(struct diff_options *);\n void diffcore_fix_diff_index(void);\n \ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex beb2ec2a55..aa947d93cf 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n-# 120 character name\n-name=aaaaaaaaaa\n-name=$name$name$name$name$name$name$name$name$name$name$name$name\n+# 120-character name\n+printf -v name 'a%.0s' {1..120}\n test_expect_success 'preparation' '\n \t>\"$name\" &&\n \tgit add \"$name\" &&\n \tgit commit -m message &&\n \techo a >\"$name\" &&\n \tgit commit -m message \"$name\"\n '\n \n cat >expect72 <<-'EOF'\n  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n EOF\n-test_expect_success \"format-patch: small change with long name gives more space to the name\" '\n+test_expect_success \"format-patch: small change with long filename gives more space to the filename\" '\n \tgit format-patch -1 --stdout >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect72 actual\n '\n \n while read cmd args\n do\n \tcat >expect80 <<-'EOF'\n \t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n \tEOF\n-\ttest_expect_success \"$cmd: small change with long name gives more space to the name\" '\n+\ttest_expect_success \"$cmd: small change with long filename gives more space to the filename\" '\n \t\tgit $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect80 actual\n@@ -58,15 +57,15 @@ 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\ttest_expect_success \"$cmd $verb statNameWidth config with long filename\" '\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\ttest_expect_success \"$cmd --stat=width $verb statNameWidth config with long filename\" '\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@@ -93,25 +92,25 @@ cat >expect2.6030 <<-'EOF'\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+\ttest_expect_success \"$cmd --stat=width: a long filename 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.40 actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat-width=width with long name\" '\n+\ttest_expect_success \"$cmd --stat-width=width with long filename\" '\n \t\tgit $cmd $args --stat-width=40 >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp $expect.40 actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat=width,name-width with long name\" '\n+\ttest_expect_success \"$cmd --stat=width,name-width with long filename\" '\n \t\tgit $cmd $args --stat=60,30 >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp $expect.6030 actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat-name-width with long name\" '\n+\ttest_expect_success \"$cmd --stat-name-width with long filename\" '\n \t\tgit $cmd $args --stat-name-width=30 >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp $expect.6030 actual\n@@ -139,7 +138,7 @@ cat >expect72 <<'EOF'\n  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-test_expect_success \"format-patch --cover-letter ignores COLUMNS (big change)\" '\n+test_expect_success \"format-patch --cover-letter ignores COLUMNS envvar with big change\" '\n \tCOLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect72 actual\n@@ -159,15 +158,15 @@ cat >expect200-graph <<'EOF'\n EOF\n while read verb expect cmd args\n do\n-\ttest_expect_success \"$cmd $verb COLUMNS (big change)\" '\n+\ttest_expect_success \"$cmd $verb COLUMNS envvar with big change\" '\n \t\tCOLUMNS=200 git $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n \tcase \"$cmd\" in diff|show) continue;; esac\n \n-\ttest_expect_success \"$cmd --graph $verb COLUMNS (big change)\" '\n+\ttest_expect_success \"$cmd --graph $verb COLUMNS envvar with big change\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect-graph\" actual\n@@ -187,15 +186,15 @@ cat >expect40-graph <<'EOF'\n EOF\n while read verb expect cmd args\n do\n-\ttest_expect_success \"$cmd $verb not enough COLUMNS (big change)\" '\n+\ttest_expect_success \"$cmd $verb not large enough COLUMNS envvar with big change\" '\n \t\tCOLUMNS=40 git $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n \tcase \"$cmd\" in diff|show) continue;; esac\n \n-\ttest_expect_success \"$cmd --graph $verb not enough COLUMNS (big change)\" '\n+\ttest_expect_success \"$cmd --graph $verb not large enough COLUMNS envvar with big change\" '\n \t\tCOLUMNS=40 git $cmd $args --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect-graph\" actual\n@@ -255,21 +254,21 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat-graph-width with big change\" '\n+\ttest_expect_success \"$cmd --stat-graph-width=width with big change\" '\n \t\tgit $cmd $args --stat-graph-width=26 >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect actual\n \t'\n \n \tcase \"$cmd\" in diff|show) continue;; esac\n \n \ttest_expect_success \"$cmd --stat-width=width --graph with big change\" '\n \t\tgit $cmd $args --stat-width=40 --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect-graph actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat-graph-width --graph with big change\" '\n+\ttest_expect_success \"$cmd --stat-graph-width=width --graph with big change\" '\n \t\tgit $cmd $args --stat-graph-width=26 --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect-graph actual\n@@ -329,15 +328,15 @@ cat >expect200-graph <<'EOF'\n EOF\n while read verb expect cmd args\n do\n-\ttest_expect_success \"$cmd $verb COLUMNS (long filename)\" '\n+\ttest_expect_success \"$cmd $verb COLUMNS envvar with long filename\" '\n \t\tCOLUMNS=200 git $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n \tcase \"$cmd\" in diff|show) continue;; esac\n \n-\ttest_expect_success \"$cmd --graph $verb COLUMNS (long filename)\" '\n+\ttest_expect_success \"$cmd --graph $verb COLUMNS envvar with long filename\" '\n \t\tCOLUMNS=200 git $cmd $args --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect-graph\" actual\n@@ -358,41 +357,57 @@ EOF\n while read verb expect cmd args\n do\n \ttest_expect_success COLUMNS_CAN_BE_1 \\\n-\t\t\"$cmd $verb prefix greater than COLUMNS (big change)\" '\n+\t\t\"$cmd $verb prefix greater than COLUMNS envvar with big change\" '\n \t\tCOLUMNS=1 git $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n \n \tcase \"$cmd\" in diff|show) continue;; esac\n \n \ttest_expect_success COLUMNS_CAN_BE_1 \\\n-\t\t\"$cmd --graph $verb prefix greater than COLUMNS (big change)\" '\n+\t\t\"$cmd --graph $verb prefix greater than COLUMNS envvar with big change\" '\n \t\tCOLUMNS=1 git $cmd $args --graph >output &&\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n ignores expect72 format-patch -1 --stdout\n respects expect1 diff HEAD^ HEAD --stat\n respects expect1 show --stat\n respects expect1 log -1 --stat\n EOF\n \n cat >expect <<'EOF'\n  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-test_expect_success 'merge --stat respects COLUMNS (big change)' '\n-\tgit checkout -b branch HEAD^^ &&\n+test_expect_success 'merge --stat respects statGraphWidth config with big change' '\n+\tgit checkout -b branch1 HEAD^^ &&\n+\tgit -c diff.statGraphWidth=26 merge --stat --no-ff main^ >output &&\n+\tgrep \" | \" output >actual &&\n+\ttest_cmp expect40 actual\n+'\n+test_expect_success 'merge --stat respects COLUMNS envvar with big change' '\n+\tgit checkout -b branch2 HEAD^^ &&\n \tCOLUMNS=100 git merge --stat --no-ff main^ >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n '\n \n cat >expect <<'EOF'\n  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++\n EOF\n-test_expect_success 'merge --stat respects COLUMNS (long filename)' '\n+cat >expect.30 <<'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++\n+EOF\n+test_expect_success 'merge --stat respects statNameWidth config with long filename' '\n+\tgit switch branch1 &&\n+\tgit -c diff.statNameWidth=30 merge --stat --no-ff main >output &&\n+\tgrep \" | \" output >actual &&\n+\ttest_cmp expect.30 actual\n+'\n+test_expect_success 'merge --stat respects COLUMNS envvar with long filename' '\n+\tgit switch branch2 &&\n \tCOLUMNS=100 git merge --stat --no-ff main >output &&\n \tgrep \" | \" output >actual &&\n \ttest_cmp expect actual\n"},{"id":"482152","messageId":"xmqqjzsi2l7r.fsf@gitster.g","threadId":"60254","inReplyTo":"166396f0a98e248fc3d1236757632c5d648ddc0b.1695364961.git.dsimic@manjaro.org","subject":"Re: [PATCH] diff --stat: set the width defaults in a helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-22T17:04:24Z","receivedAt":"2023-09-22T17:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> Extract the commonly used initialization of the --stat-width=<width>,\n> --stat-name-width=<width> and --stat-graph-with=<width> parameters to the\n> internal default values into a helper function, to avoid repeating the same\n> initialization code in a few places.\n\nThanks.\n\nIf this is only about settings related to controlling widths of\nvarious elements on diffstat lines, isn't the \"std\" in name a bit\ntoo broad, though?  init_diffstat_widths(&rev.diffopt) or something\nlike that might be a better fit.  I dunno if it is a huge deal,\nthough.\n\n> Add a couple of tests to additionally cover existing configuration options\n> diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by\n> git-merge to generate --stat outputs.  This closes the gap in the tests that\n> existed previously for the --stat tests, reducing the chances for having\n> any regressions introduced by this commit.\n\nNice.\n\n> While there, perform a bunch of small wording improvements and some minor\n> code cleanups in the improved unit test, as spotted, to make it a bit neater\n> and to improve its test-level consistency.\n\nAlright.  The last category of changes need somebody else to review\nthem in addition to myself, as I expect that it would be somewhat\nsubjective and I tend to be change-averse.\n\nThe code changes all looked sensible.\n\n> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n> index beb2ec2a55..aa947d93cf 100755\n> --- a/t/t4052-stat-output.sh\n> +++ b/t/t4052-stat-output.sh\n> @@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY\"/lib-terminal.sh\n>  \n> -# 120 character name\n> -name=aaaaaaaaaa\n> -name=$name$name$name$name$name$name$name$name$name$name$name$name\n> +# 120-character name\n> +printf -v name 'a%.0s' {1..120}\n\nThis is a totally unnecessary and unacceptable change.  \"-v name\"\nmay be available in the built-in variant found in bash, but you\nwould likely find that it is missing from other shells.  {1..120} is\nalso a bash-ism.\n\nAnd because we are still calling the result a \"name\" (not\n\"filename\") ...\n\n>  cat >expect72 <<-'EOF'\n>   ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n>  EOF\n> -test_expect_success \"format-patch: small change with long name gives more space to the name\" '\n> +test_expect_success \"format-patch: small change with long filename gives more space to the filename\" '\n\n... I do not see the point of this change (and similar ones in the\nrest of the patch).  Even the configuration is called statNameWidth\nand not statFileNameWidth.  In the context of the tests that check\n\"stat-output\" (that is in the filename of this script), we should be\nable to use \"name\" consistently without causing any confusion, as it\nis unlikely to be mistaken with other kinds of \"name\".\n\n> -test_expect_success \"format-patch --cover-letter ignores COLUMNS (big change)\" '\n> +test_expect_success \"format-patch --cover-letter ignores COLUMNS envvar with big change\" '\n\nNot wrong per-se, but I wonder if it is necessary to stress that\nCOLUMNS is an environment variable that tells the programs how wide\na terminal they are showing their output.  A usual shell variable\nwould not affect the \"git\" process it runs, and COLUMNS without any\ndot in it cannot be our configuration variable, so even without deep\nknowledge of tradition, I thought it would be rather obvious.\n\nSame comment for \"statNameWidth config\"; with fewer number of bytes,\nit would be more descriptive to say \"diff.statNameWidth\".\n\n> -\ttest_expect_success \"$cmd --stat-graph-width with big change\" '\n> +\ttest_expect_success \"$cmd --stat-graph-width=width with big change\" '\n>  \t\tgit $cmd $args --stat-graph-width=26 >output &&\n\nThis may be a good change, especially if there are tests that feed\ndifferent parameters and if it helps clarifying which variant is\ntested, e.g. \"--stat=<width>,<name-width>\" vs \"--stat=<width>\".\n\nAh, wait, \"--stat-graph-width\" always takes a single value, so the\nabove justification does not quite apply.  But still, it is not\nmaking it worse, and because there is another test that is labeled\nwith \"--stat-width=width\", being consistent with it has value.\n\nOK.\n\n> -\ttest_expect_success \"$cmd --stat-graph-width --graph with big change\" '\n> +\ttest_expect_success \"$cmd --stat-graph-width=width --graph with big change\" '\n\nDitto.\n\nThanks.\n"},{"id":"482159","messageId":"fc9ba2f7a8080f7fd669dd8f718876bc@manjaro.org","threadId":"60254","inReplyTo":"xmqqjzsi2l7r.fsf@gitster.g","subject":"Re: [PATCH] diff --stat: set the width defaults in a helper function","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2023-09-22T17:52:14Z","receivedAt":"2023-09-22T17:54:04Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2023-09-22 19:04, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Extract the commonly used initialization of the --stat-width=<width>,\n>> --stat-name-width=<width> and --stat-graph-with=<width> parameters to \n>> the\n>> internal default values into a helper function, to avoid repeating the \n>> same\n>> initialization code in a few places.\n> \n> Thanks.\n> \n> If this is only about settings related to controlling widths of\n> various elements on diffstat lines, isn't the \"std\" in name a bit\n> too broad, though?  init_diffstat_widths(&rev.diffopt) or something\n> like that might be a better fit.  I dunno if it is a huge deal,\n> though.\n\nMakes sense to me, init_diffstat_widths(&rev.diffopt) fits better and \nit's more descriptive.  I'm a big fan of using self-descriptive naming, \nbut originally I wanted to follow the already present naming scheme, so \nI'm glad to see that you asked for a more descriptive function name.\n\n>> Add a couple of tests to additionally cover existing configuration \n>> options\n>> diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used \n>> by\n>> git-merge to generate --stat outputs.  This closes the gap in the \n>> tests that\n>> existed previously for the --stat tests, reducing the chances for \n>> having\n>> any regressions introduced by this commit.\n> \n> Nice.\n\nThanks.\n\n>> While there, perform a bunch of small wording improvements and some \n>> minor\n>> code cleanups in the improved unit test, as spotted, to make it a bit \n>> neater\n>> and to improve its test-level consistency.\n> \n> Alright.  The last category of changes need somebody else to review\n> them in addition to myself, as I expect that it would be somewhat\n> subjective and I tend to be change-averse.\n> \n> The code changes all looked sensible.\n> \n>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n>> index beb2ec2a55..aa947d93cf 100755\n>> --- a/t/t4052-stat-output.sh\n>> +++ b/t/t4052-stat-output.sh\n>> @@ -12,32 +12,31 @@ TEST_PASSES_SANITIZE_LEAK=true\n>>  . ./test-lib.sh\n>>  . \"$TEST_DIRECTORY\"/lib-terminal.sh\n>> \n>> -# 120 character name\n>> -name=aaaaaaaaaa\n>> -name=$name$name$name$name$name$name$name$name$name$name$name$name\n>> +# 120-character name\n>> +printf -v name 'a%.0s' {1..120}\n> \n> This is a totally unnecessary and unacceptable change.  \"-v name\"\n> may be available in the built-in variant found in bash, but you\n> would likely find that it is missing from other shells.  {1..120} is\n> also a bash-ism.\n\nMakes sense to stick with the simpler variant, which won't cause \ncompatibility issues.\n\n> And because we are still calling the result a \"name\" (not\n> \"filename\") ...\n> \n>>  cat >expect72 <<-'EOF'\n>>   ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 \n>> +\n>>  EOF\n>> -test_expect_success \"format-patch: small change with long name gives \n>> more space to the name\" '\n>> +test_expect_success \"format-patch: small change with long filename \n>> gives more space to the filename\" '\n> \n> ... I do not see the point of this change (and similar ones in the\n> rest of the patch).  Even the configuration is called statNameWidth\n> and not statFileNameWidth.  In the context of the tests that check\n> \"stat-output\" (that is in the filename of this script), we should be\n> able to use \"name\" consistently without causing any confusion, as it\n> is unlikely to be mistaken with other kinds of \"name\".\n\nOn second thought, I agree that sticking with just \"name\" is better in \nthis test.\n\n>> -test_expect_success \"format-patch --cover-letter ignores COLUMNS (big \n>> change)\" '\n>> +test_expect_success \"format-patch --cover-letter ignores COLUMNS \n>> envvar with big change\" '\n> \n> Not wrong per-se, but I wonder if it is necessary to stress that\n> COLUMNS is an environment variable that tells the programs how wide\n> a terminal they are showing their output.  A usual shell variable\n> would not affect the \"git\" process it runs, and COLUMNS without any\n> dot in it cannot be our configuration variable, so even without deep\n> knowledge of tradition, I thought it would be rather obvious.\n\nOn second thought, I agree that using just \"COLUMN\" suffices while also \nbeing more compact.\n\n> Same comment for \"statNameWidth config\"; with fewer number of bytes,\n> it would be more descriptive to say \"diff.statNameWidth\".\n\nOh, nice catch!  Makes sense.\n\n>> -\ttest_expect_success \"$cmd --stat-graph-width with big change\" '\n>> +\ttest_expect_success \"$cmd --stat-graph-width=width with big change\" \n>> '\n>>  \t\tgit $cmd $args --stat-graph-width=26 >output &&\n> \n> This may be a good change, especially if there are tests that feed\n> different parameters and if it helps clarifying which variant is\n> tested, e.g. \"--stat=<width>,<name-width>\" vs \"--stat=<width>\".\n> \n> Ah, wait, \"--stat-graph-width\" always takes a single value, so the\n> above justification does not quite apply.  But still, it is not\n> making it worse, and because there is another test that is labeled\n> with \"--stat-width=width\", being consistent with it has value.\n\nYes, I think that the improved consistency outweights the slight \nredundancy.\n\n> OK.\n> \n>> -\ttest_expect_success \"$cmd --stat-graph-width --graph with big \n>> change\" '\n>> +\ttest_expect_success \"$cmd --stat-graph-width=width --graph with big \n>> change\" '\n> \n> Ditto.\n> \n> Thanks.\n\nThanks, I'll prepare and send v2 of the patch, based on your feedback.\n"}]}