{"thread":{"id":"55451","subject":"[PATCH 0/9] git log: configurable default format for merge diffs","startedAt":"2021-04-07T22:56:24Z","lastAt":"2021-04-16T08:30:44Z","messageCount":47,"participants":["Sergey Organov","Ævar Arnfjörð Bjarmason","Junio C Hamano","Philip Oakley","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"421169","messageId":"20210407225608.14611-1-sorganov@gmail.com","threadId":"55451","inReplyTo":null,"subject":"[PATCH 0/9] git log: configurable default format for merge diffs","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:55:59Z","receivedAt":"2021-04-07T22:56:24Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"These patches introduce capability to configure the default format of\noutput of diffs for merge commits by means of new log.diffMerges\nconfiguration variable. The default format is then used by -m,\n--diff-merges=m, and new --diff-merges=def options.\n\nIn particular,\n\n git config log.diffMerges first-parent\n\nwill change -m option format from \"separate\" to \"first-parent\" that\nwill in turn cause, say,\n\n git show -m \n\nto output diff to the first parent only, instead of appending\ntypically large and surprising diff to the second parent at the end of\nthe output.\n\nSergey Organov (9):\n diff-merges: introduce --diff-merges=def\n diff-merges: refactor set_diff_merges()\n diff-merges: introduce log.diffMerges config variable\n diff-merges: adapt -m to enable default diff format\n t4013: add test for --diff-merges=def\n t4013: add tests for log.diffMerges config\n t9902: fix completion tests for log.d* to match log.diffMerges\n doc/diff-options: document new --diff-merges features\n doc/config: document log.diffMerges\n\n Documentation/config/log.txt | 5 +++\n Documentation/diff-options.txt | 15 ++++++---\n builtin/log.c | 2 ++\n diff-merges.c | 58 ++++++++++++++++++++++++----------\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 34 ++++++++++++++++++++\n t/t9902-completion.sh | 3 ++\n 7 files changed, 98 insertions(+), 21 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"421170","messageId":"20210407225608.14611-2-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 1/9] diff-merges: introduce --diff-merges=def","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:00Z","receivedAt":"2021-04-07T22:56:25Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Introduce the notion of default diff format for merges, and the option\n\"def\" to select it. The default is \"separate\" and can't yet be\nchanged, so effectively \"dev\" is just a synonym for \"separate\" for\nnow.\n\nThis is in preparation for introducing log.diffMerges configuration\noption that will let --diff-merges=def to be configured to any\nsupported format.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 146bb50316a6..0887a07cfc67 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -2,6 +2,8 @@\n \n #include \"revision.h\"\n \n+typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n+\n static void suppress(struct rev_info *revs)\n {\n \trevs->separate_merges = 0;\n@@ -19,6 +21,8 @@ static void set_separate(struct rev_info *revs)\n \trevs->separate_merges = 1;\n }\n \n+static diff_merges_setup_func_t set_to_default = set_separate;\n+\n static void set_first_parent(struct rev_info *revs)\n {\n \tset_separate(revs);\n@@ -66,6 +70,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n \t\tset_combined(revs);\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\tset_dense_combined(revs);\n+\telse if (!strcmp(optarg, \"def\"))\n+\t\tset_to_default(revs);\n \telse\n \t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n \n-- \n2.25.1\n\n"},{"id":"421171","messageId":"20210407225608.14611-3-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 2/9] diff-merges: refactor set_diff_merges()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:01Z","receivedAt":"2021-04-07T22:56:27Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Split set_diff_merges() into separate parsing and execution functions,\nthe former to be reused later in the series for parsing of\nconfiguration values.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 0887a07cfc67..93ede09fb36f 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -3,6 +3,9 @@\n #include \"revision.h\"\n \n typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n+static void set_separate(struct rev_info *revs);\n+\n+static diff_merges_setup_func_t set_to_default = set_separate;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -21,8 +24,6 @@ static void set_separate(struct rev_info *revs)\n \trevs->separate_merges = 1;\n }\n \n-static diff_merges_setup_func_t set_to_default = set_separate;\n-\n static void set_first_parent(struct rev_info *revs)\n {\n \tset_separate(revs);\n@@ -54,29 +55,35 @@ static void set_dense_combined(struct rev_info *revs)\n \trevs->dense_combined_merges = 1;\n }\n \n-static void set_diff_merges(struct rev_info *revs, const char *optarg)\n+static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n-\tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\")) {\n-\t\tsuppress(revs);\n-\t\t/* Return early to leave revs->merges_need_diff unset */\n-\t\treturn;\n-\t}\n-\n+\tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n+\t\treturn suppress;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n-\t\tset_first_parent(revs);\n+\t\treturn set_first_parent;\n \telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"separate\"))\n-\t\tset_separate(revs);\n+\t\treturn set_separate;\n \telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n-\t\tset_combined(revs);\n+\t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n-\t\tset_dense_combined(revs);\n+\t\treturn set_dense_combined;\n \telse if (!strcmp(optarg, \"def\"))\n-\t\tset_to_default(revs);\n-\telse\n+\t\treturn set_to_default;\n+\treturn NULL;\n+}\n+\n+static void set_diff_merges(struct rev_info *revs, const char *optarg)\n+{\n+\tdiff_merges_setup_func_t func = func_by_opt(optarg);\n+\n+\tif (!func)\n \t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n \n-\t/* The flag is cleared by set_xxx() functions, so don't move this up */\n-\trevs->merges_need_diff = 1;\n+\tfunc(revs);\n+\n+\t/* NOTE: the merges_need_diff flag is cleared by func() call */\n+\tif (func != suppress)\n+\t\trevs->merges_need_diff = 1;\n }\n \n /*\n-- \n2.25.1\n\n"},{"id":"421172","messageId":"20210407225608.14611-4-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 3/9] diff-merges: introduce log.diffMerges config variable","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:02Z","receivedAt":"2021-04-07T22:56:29Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"New log.diffMerges configuration variable sets the format that\n--diff-merges=def will be using. The default is \"separate\".\n\nSigned-off-by: Sergey Organov \n---\n builtin/log.c | 2 ++\n diff-merges.c | 11 +++++++++++\n diff-merges.h | 2 ++\n 3 files changed, 15 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8acd285dafd8..6102893fccb9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -481,6 +481,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"log.diffmerges\"))\n+\t\treturn diff_merges_config(value);\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 93ede09fb36f..ca4d94a9039d 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -90,6 +90,17 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n * Public functions. They are in the order they are called.\n */\n \n+int diff_merges_config(const char *value)\n+{\n+\tdiff_merges_setup_func_t func = func_by_opt(value);\n+\n+\tif (!func)\n+\t\treturn -1;\n+\n+\tset_to_default = func;\n+\treturn 0;\n+}\n+\n int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n {\n \tint argcount = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 659467c99a4f..09d9a6c9a4fb 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -9,6 +9,8 @@\n \n struct rev_info;\n \n+int diff_merges_config(const char *value);\n+\n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\n \n void diff_merges_suppress(struct rev_info *revs);\n-- \n2.25.1\n\n"},{"id":"421173","messageId":"20210407225608.14611-5-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 4/9] diff-merges: adapt -m to enable default diff format","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:03Z","receivedAt":"2021-04-07T22:56:29Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Let -m option (and --diff-merges=m) enable the default format instead\nof \"separate\", to be able to tune it with log.diffMerges option.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex ca4d94a9039d..f68e4376fd63 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -34,10 +34,10 @@ static void set_m(struct rev_info *revs)\n {\n \t/*\n \t * To \"diff-index\", \"-m\" means \"match missing\", and to the \"log\"\n-\t * family of commands, it means \"show full diff for merges\". Set\n+\t * family of commands, it means \"show default diff for merges\". Set\n \t * both fields appropriately.\n \t */\n-\tset_separate(revs);\n+\tset_to_default(revs);\n \trevs->match_missing = 1;\n }\n \n@@ -61,13 +61,13 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n \t\treturn suppress;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n-\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"separate\"))\n+\telse if (!strcmp(optarg, \"separate\"))\n \t\treturn set_separate;\n \telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n \t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\treturn set_dense_combined;\n-\telse if (!strcmp(optarg, \"def\"))\n+\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"def\"))\n \t\treturn set_to_default;\n \treturn NULL;\n }\n-- \n2.25.1\n\n"},{"id":"421174","messageId":"20210407225608.14611-6-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 5/9] t4013: add test for --diff-merges=def","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:04Z","receivedAt":"2021-04-07T22:56:31Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"This new option by default should match --diff-merges=separate, so\ntest this.\n\nSigned-off-by: Sergey Organov \n---\n t/t4013-diff-various.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 6cca8b84a6bf..275a6790896d 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n+test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '\n+\tgit log -p --diff-merges=separate master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log -p --diff-merges=def master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -S requires an argument' '\n \ttest_must_fail git log -S\n '\n-- \n2.25.1\n\n"},{"id":"421175","messageId":"20210407225608.14611-7-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 6/9] t4013: add tests for log.diffMerges config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:05Z","receivedAt":"2021-04-07T22:56:31Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Test that wrong values are denied.\n\nTest that the value of log.diffMerges properly affects both\n--diff-merges=def and -m.\n\nSigned-off-by: Sergey Organov \n---\n t/t4013-diff-various.sh | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 275a6790896d..ee4afca06ced 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -460,6 +460,32 @@ test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'deny wrong log.diffMerges config' '\n+\tgit config log.diffMerges wrong-value &&\n+\ttest_expect_code 128 git log &&\n+\tgit config --unset log.diffMerges\n+'\n+\n+test_expect_success 'git config log.diffMerges first-parent' '\n+\tgit log -p --diff-merges=first-parent master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit config log.diffMerges first-parent &&\n+\tgit log -p --diff-merges=def master >result &&\n+\tprocess_diffs result >actual &&\n+\tgit config --unset log.diffMerges &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git config log.diffMerges first-parent vs -m' '\n+\tgit log -p --diff-merges=first-parent master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit config log.diffMerges first-parent &&\n+\tgit log -p -m master >result &&\n+\tprocess_diffs result >actual &&\n+\tgit config --unset log.diffMerges &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -S requires an argument' '\n \ttest_must_fail git log -S\n '\n-- \n2.25.1\n\n"},{"id":"421176","messageId":"20210407225608.14611-8-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:06Z","receivedAt":"2021-04-07T22:56:35Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"There were 3 completion tests failures due to introduction of\nlog.diffMerges configuration variable that affected the result of\ncompletion of log.d. Fixed them accordingly.\n\nSigned-off-by: Sergey Organov \n---\n t/t9902-completion.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 04ce884ef5ac..4d732d6d4f81 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n \ttest_completion \"git config log.d\" <<-\\EOF\n \tlog.date Z\n \tlog.decorate Z\n+\tlog.diffMerges Z\n \tEOF\n '\n \n@@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n \ttest_completion \"git -c log.d\" <<-\\EOF\n \tlog.date=Z\n \tlog.decorate=Z\n+\tlog.diffMerges=Z\n \tEOF\n '\n \n@@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n \tlog.date=Z\n \tlog.decorate=Z\n+\tlog.diffMerges=Z\n \tEOF\n '\n \n-- \n2.25.1\n\n"},{"id":"421177","messageId":"20210407225608.14611-9-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 8/9] doc/diff-options: document new --diff-merges features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:07Z","receivedAt":"2021-04-07T22:56:36Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Document changes in -m and --diff-merges=m semantics, as well as new\n--diff-merges=def option.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/diff-options.txt | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex aa2b5c11f20b..09b07231b5a4 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -34,7 +34,7 @@ endif::git-diff[]\n endif::git-format-patch[]\n \n ifdef::git-log[]\n---diff-merges=(off|none|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n+--diff-merges=(off|none|def|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n --no-diff-merges::\n \tSpecify diff format to be used for merge commits. Default is\n \t{diff-merges-default} unless `--first-parent` is in use, in which case\n@@ -45,17 +45,24 @@ ifdef::git-log[]\n \tDisable output of diffs for merge commits. Useful to override\n \timplied value.\n +\n+--diff-merges=def:::\n+--diff-merges=m:::\n+-m:::\n+\tThis option makes diff output for merge commits to be shown in\n+\tthe default format. `-m` will produce the output only if `-p`\n+\tis given as well. The default format could be changed using\n+\t`log.diffMerges` configuration parameter, which default value\n+\tis `separate`.\n++\n --diff-merges=first-parent:::\n --diff-merges=1:::\n \tThis option makes merge commits show the full diff with\n \trespect to the first parent only.\n +\n --diff-merges=separate:::\n---diff-merges=m:::\n--m:::\n \tThis makes merge commits show the full diff with respect to\n \teach of the parents. Separate log entry and diff is generated\n-\tfor each parent. `-m` doesn't produce any output without `-p`.\n+\tfor each parent.\n +\n --diff-merges=combined:::\n --diff-merges=c:::\n-- \n2.25.1\n\n"},{"id":"421178","messageId":"20210407225608.14611-10-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH 9/9] doc/config: document log.diffMerges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-07T22:56:08Z","receivedAt":"2021-04-07T22:56:39Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Added documentation for the new log.diffMerges configuration option.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 208d5fdcaa68..456eb07800cb 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -24,6 +24,11 @@ log.excludeDecoration::\n \tthe config option can be overridden by the `--decorate-refs`\n \toption.\n \n+log.diffMerges::\n+\tSet default diff format to be used for merge commits. See\n+\t`--diff-merges` in linkgit:git-log[1] for details.\n+\tDefaults to `separate`.\n+\n log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\n \ta single is given. This has the same limitations as `--follow`,\n-- \n2.25.1\n\n"},{"id":"421181","messageId":"87y2dtitlp.fsf@evledraar.gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-8-sorganov@gmail.com","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-07T23:05:06Z","receivedAt":"2021-04-07T23:05:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 08 2021, Sergey Organov wrote:\n\n> There were 3 completion tests failures due to introduction of\n> log.diffMerges configuration variable that affected the result of\n> completion of log.d. Fixed them accordingly.\n>\n> Signed-off-by: Sergey Organov \n> ---\n> t/t9902-completion.sh | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 04ce884ef5ac..4d732d6d4f81 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n> \ttest_completion \"git config log.d\" <<-\\EOF\n> \tlog.date Z\n> \tlog.decorate Z\n> +\tlog.diffMerges Z\n> \tEOF\n> '\n> \n> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n> \ttest_completion \"git -c log.d\" <<-\\EOF\n> \tlog.date=Z\n> \tlog.decorate=Z\n> +\tlog.diffMerges=Z\n> \tEOF\n> '\n> \n> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n> \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n> \tlog.date=Z\n> \tlog.decorate=Z\n> +\tlog.diffMerges=Z\n> \tEOF\n> '\n\nCommits should be made in such a way as to not break the build/tests\npartway through a series, which it seems is happening until this fixup.\n\nHaving read this far most of what you have in this 9 patch series\ncould/should be squashed into something much smaller, e.g. tests being\nadded for code added in previous steps, let's add the tests along with\nthe code since this isn't such a large change.\n"},{"id":"421182","messageId":"87v98xitjh.fsf@evledraar.gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-7-sorganov@gmail.com","subject":"Re: [PATCH 6/9] t4013: add tests for log.diffMerges config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-07T23:06:26Z","receivedAt":"2021-04-07T23:06:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 08 2021, Sergey Organov wrote:\n\n> Test that wrong values are denied.\n>\n> Test that the value of log.diffMerges properly affects both\n> --diff-merges=def and -m.\n>\n> Signed-off-by: Sergey Organov \n> ---\n> t/t4013-diff-various.sh | 26 ++++++++++++++++++++++++++\n> 1 file changed, 26 insertions(+)\n>\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 275a6790896d..ee4afca06ced 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -460,6 +460,32 @@ test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'deny wrong log.diffMerges config' '\n> +\tgit config log.diffMerges wrong-value &&\n> +\ttest_expect_code 128 git log &&\n> +\tgit config --unset log.diffMerges\n\nDon't use \"git config\", but \"test_config\" at the start, then you don't\nneed the --unset at the end, it'll happen automatically. Ditto for the\nfollowing tests.\n\n> +'\n> +\n> +test_expect_success 'git config log.diffMerges first-parent' '\n> +\tgit log -p --diff-merges=first-parent master >result &&\n> +\tprocess_diffs result >expected &&\n> +\tgit config log.diffMerges first-parent &&\n> +\tgit log -p --diff-merges=def master >result &&\n> +\tprocess_diffs result >actual &&\n> +\tgit config --unset log.diffMerges &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'git config log.diffMerges first-parent vs -m' '\n> +\tgit log -p --diff-merges=first-parent master >result &&\n> +\tprocess_diffs result >expected &&\n> +\tgit config log.diffMerges first-parent &&\n> +\tgit log -p -m master >result &&\n> +\tprocess_diffs result >actual &&\n> +\tgit config --unset log.diffMerges &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'log -S requires an argument' '\n> \ttest_must_fail git log -S\n> '\n\n"},{"id":"421187","messageId":"xmqqh7khwtw5.fsf@gitster.g","threadId":"55451","inReplyTo":"87v98xitjh.fsf@evledraar.gmail.com","subject":"Re: [PATCH 6/9] t4013: add tests for log.diffMerges config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T23:35:06Z","receivedAt":"2021-04-07T23:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> +test_expect_success 'deny wrong log.diffMerges config' '\n>> +\tgit config log.diffMerges wrong-value &&\n>> +\ttest_expect_code 128 git log &&\n>> +\tgit config --unset log.diffMerges\n>\n> Don't use \"git config\", but \"test_config\" at the start, then you don't\n> need the --unset at the end, it'll happen automatically. Ditto for the\n> following tests.\n\nMore importantly, test_config arranges the unset to happen even if\na step in the middle (e.g. test_expect_code in the above example)\nfails. In the posted version, the control would not reach the\n\"git config --unset\" and leaves the configuration behind.\n\nAnd that is the biggest reason why the above should use test_config.\n\nThanks for a good suggestion.\n"},{"id":"421203","messageId":"f6b25ea6-88b1-c167-7fd4-440be8782fcb@iee.email","threadId":"55451","inReplyTo":"20210407225608.14611-2-sorganov@gmail.com","subject":"Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-04-08T11:48:11Z","receivedAt":"2021-04-08T11:48:15Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi,\n\nOn 07/04/2021 23:56, Sergey Organov wrote:\n> Introduce the notion of default diff format for merges, and the option\n> \"def\" to select it. The default is \"separate\" and can't yet be\n\"def\" feels a bit too short and sounds similar to \"define\" - why not\nspell out in full?\n> changed, so effectively \"dev\" is just a synonym for \"separate\" for\ndid you mean \"def\"?  i.e. s/dev/def/   (..spell out in full ;-)\n\n--\nPhilip\n> now.\n>\n> This is in preparation for introducing log.diffMerges configuration\n> option that will let --diff-merges=def to be configured to any\n> supported format.\n>\n> Signed-off-by: Sergey Organov \n> ---\n> diff-merges.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/diff-merges.c b/diff-merges.c\n> index 146bb50316a6..0887a07cfc67 100644\n> --- a/diff-merges.c\n> +++ b/diff-merges.c\n> @@ -2,6 +2,8 @@\n> \n> #include \"revision.h\"\n> \n> +typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n> +\n> static void suppress(struct rev_info *revs)\n> {\n> \trevs->separate_merges = 0;\n> @@ -19,6 +21,8 @@ static void set_separate(struct rev_info *revs)\n> \trevs->separate_merges = 1;\n> }\n> \n> +static diff_merges_setup_func_t set_to_default = set_separate;\n> +\n> static void set_first_parent(struct rev_info *revs)\n> {\n> \tset_separate(revs);\n> @@ -66,6 +70,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n> \t\tset_combined(revs);\n> \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n> \t\tset_dense_combined(revs);\n> +\telse if (!strcmp(optarg, \"def\"))\n> +\t\tset_to_default(revs);\n> \telse\n> \t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n> \n\n"},{"id":"421219","messageId":"87eefkdfho.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"f6b25ea6-88b1-c167-7fd4-440be8782fcb@iee.email","subject":"Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T14:21:07Z","receivedAt":"2021-04-08T14:21:13Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Hi,\n\nPhilip Oakley writes:\n> Hi,\n>\n> On 07/04/2021 23:56, Sergey Organov wrote:\n>> Introduce the notion of default diff format for merges, and the option\n>> \"def\" to select it. The default is \"separate\" and can't yet be\n> \"def\" feels a bit too short and sounds similar to \"define\" - why not\n> spell out in full?\n\nDunno, it just happened. No sound reason. Will change to \"default\" for\nthe next re-roll.\n\n>> changed, so effectively \"dev\" is just a synonym for \"separate\" for\n> did you mean \"def\"?  i.e. s/dev/def/   (..spell out in full ;-)\n\nYep, thanks!\n\n-- Sergey\n"},{"id":"421220","messageId":"87a6q8dfaj.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"87v98xitjh.fsf@evledraar.gmail.com","subject":"Re: [PATCH 6/9] t4013: add tests for log.diffMerges config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T14:25:24Z","receivedAt":"2021-04-08T14:25:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 08 2021, Sergey Organov wrote:\n\n[...]\n\n>> +test_expect_success 'deny wrong log.diffMerges config' '\n>> +\tgit config log.diffMerges wrong-value &&\n>> +\ttest_expect_code 128 git log &&\n>> +\tgit config --unset log.diffMerges\n>\n> Don't use \"git config\", but \"test_config\" at the start, then you don't\n> need the --unset at the end, it'll happen automatically. Ditto for the\n> following tests.\n\n[...]\n\nJunio C Hamano writes:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> +test_expect_success 'deny wrong log.diffMerges config' '\n>>> +\tgit config log.diffMerges wrong-value &&\n>>> +\ttest_expect_code 128 git log &&\n>>> +\tgit config --unset log.diffMerges\n>>\n>> Don't use \"git config\", but \"test_config\" at the start, then you don't\n>> need the --unset at the end, it'll happen automatically. Ditto for the\n>> following tests.\n>\n> More importantly, test_config arranges the unset to happen even if\n> a step in the middle (e.g. test_expect_code in the above example)\n> fails. In the posted version, the control would not reach the\n> \"git config --unset\" and leaves the configuration behind.\n>\n> And that is the biggest reason why the above should use test_config.\n\nYeah, thanks for pointing, – will fix for the next re-roll.\n\n-- Sergey Organov\n"},{"id":"421222","messageId":"875z0wdekf.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"87y2dtitlp.fsf@evledraar.gmail.com","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T14:41:04Z","receivedAt":"2021-04-08T14:41:15Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 08 2021, Sergey Organov wrote:\n>\n>> There were 3 completion tests failures due to introduction of\n>> log.diffMerges configuration variable that affected the result of\n>> completion of log.d. Fixed them accordingly.\n>>\n>> Signed-off-by: Sergey Organov \n>> ---\n>> t/t9902-completion.sh | 3 +++\n>> 1 file changed, 3 insertions(+)\n>>\n>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>> index 04ce884ef5ac..4d732d6d4f81 100755\n>> --- a/t/t9902-completion.sh\n>> +++ b/t/t9902-completion.sh\n>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n>> \ttest_completion \"git config log.d\" <<-\\EOF\n>> \tlog.date Z\n>> \tlog.decorate Z\n>> +\tlog.diffMerges Z\n>> \tEOF\n>> '\n>> \n>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n>> \ttest_completion \"git -c log.d\" <<-\\EOF\n>> \tlog.date=Z\n>> \tlog.decorate=Z\n>> +\tlog.diffMerges=Z\n>> \tEOF\n>> '\n>> \n>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n>> \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n>> \tlog.date=Z\n>> \tlog.decorate=Z\n>> +\tlog.diffMerges=Z\n>> \tEOF\n>> '\n>\n> Commits should be made in such a way as to not break the build/tests\n> partway through a series, which it seems is happening until this\n> fixup.\n\nYep.\n\nCould these tests be somehow written in a more robust manner, to be\nprotected against future additions of configuration variables that are\nunrelated to the features being tested? If so, I'd prefer to fix them as\na prerequisite to the series rather than adding fixes to unrelated \nexisting tests into my patches.\n\n> Having read this far most of what you have in this 9 patch series\n> could/should be squashed into something much smaller, e.g. tests being\n> added for code added in previous steps, let's add the tests along with\n> the code since this isn't such a large change.\n\nIn general, I try to make commits as small as possible, but if you\nprefer tests to be included with the code in the same commit, – that's\nfine with me too.\n\nWill meld new tests into code commits for the next re-roll.\n\nThanks!\n\n-- Sergey Organov\n"},{"id":"421245","messageId":"xmqq8s5svg8b.fsf@gitster.g","threadId":"55451","inReplyTo":"87eefkdfho.fsf@osv.gnss.ru","subject":"Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T17:27:48Z","receivedAt":"2021-04-08T17:27:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Hi,\n>\n> Philip Oakley writes:\n>> Hi,\n>>\n>> On 07/04/2021 23:56, Sergey Organov wrote:\n>>> Introduce the notion of default diff format for merges, and the option\n>>> \"def\" to select it. The default is \"separate\" and can't yet be\n>> \"def\" feels a bit too short and sounds similar to \"define\" - why not\n>> spell out in full?\n>\n> Dunno, it just happened. No sound reason. Will change to \"default\" for\n> the next re-roll.\n\nI do not immediately see the point of writing --diff-merges=default\non the command line in the first place. If what it calls for is the\ndefault, wouldn't it be easier to just leave it out?\n\nBut if we have to have it as one of the choice, please do not invent\nsuch an abbreviation, especially without taking the fully-spelled\nform.\n\nThanks.\n"},{"id":"421246","messageId":"87o8eo7k3d.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"xmqq8s5svg8b.fsf@gitster.g","subject":"Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T17:38:14Z","receivedAt":"2021-04-08T17:38:18Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Sergey Organov writes:\n>\n>> Hi,\n>>\n>> Philip Oakley writes:\n>>> Hi,\n>>>\n>>> On 07/04/2021 23:56, Sergey Organov wrote:\n>>>> Introduce the notion of default diff format for merges, and the option\n>>>> \"def\" to select it. The default is \"separate\" and can't yet be\n>>> \"def\" feels a bit too short and sounds similar to \"define\" - why not\n>>> spell out in full?\n>>\n>> Dunno, it just happened. No sound reason. Will change to \"default\" for\n>> the next re-roll.\n>\n> I do not immediately see the point of writing --diff-merges=default\n> on the command line in the first place. If what it calls for is the\n> default, wouldn't it be easier to just leave it out?\n\nIt does enable output of diffs for merge commits, so it's not the same\nas leaving it out. The \"default\" is the exact format it will use for the\noutput.\n\nOr do you mean using bare \"--diff-merges\", without \"=value\"? It is\nconsidered bad practice, right?\n\n>\n> But if we have to have it as one of the choice, please do not invent\n> such an abbreviation, especially without taking the fully-spelled\n> form.\n\nI think we have to, see above, and yes, I'll turn it to the full form.\n\nThanks,\n\n-- Sergey Organov\n"},{"id":"421258","messageId":"87sg40imit.fsf@evledraar.gmail.com","threadId":"55451","inReplyTo":"875z0wdekf.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-08T19:50:18Z","receivedAt":"2021-04-08T19:50:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 08 2021, Sergey Organov wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Thu, Apr 08 2021, Sergey Organov wrote:\n>>\n>>> There were 3 completion tests failures due to introduction of\n>>> log.diffMerges configuration variable that affected the result of\n>>> completion of log.d. Fixed them accordingly.\n>>>\n>>> Signed-off-by: Sergey Organov \n>>> ---\n>>> t/t9902-completion.sh | 3 +++\n>>> 1 file changed, 3 insertions(+)\n>>>\n>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>>> index 04ce884ef5ac..4d732d6d4f81 100755\n>>> --- a/t/t9902-completion.sh\n>>> +++ b/t/t9902-completion.sh\n>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n>>> \ttest_completion \"git config log.d\" <<-\\EOF\n>>> \tlog.date Z\n>>> \tlog.decorate Z\n>>> +\tlog.diffMerges Z\n>>> \tEOF\n>>> '\n>>> \n>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n>>> \ttest_completion \"git -c log.d\" <<-\\EOF\n>>> \tlog.date=Z\n>>> \tlog.decorate=Z\n>>> +\tlog.diffMerges=Z\n>>> \tEOF\n>>> '\n>>> \n>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n>>> \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n>>> \tlog.date=Z\n>>> \tlog.decorate=Z\n>>> +\tlog.diffMerges=Z\n>>> \tEOF\n>>> '\n>>\n>> Commits should be made in such a way as to not break the build/tests\n>> partway through a series, which it seems is happening until this\n>> fixup.\n>\n> Yep.\n>\n> Could these tests be somehow written in a more robust manner, to be\n> protected against future additions of configuration variables that are\n> unrelated to the features being tested? If so, I'd prefer to fix them as\n> a prerequisite to the series rather than adding fixes to unrelated \n> existing tests into my patches.\n\nHrm? I mean if you have a commit fixing up failing tests in an earlier\ncommit then that change should in one way or the other be made as part\nof that earlier change.\n\nYes we can skip the tests or something in the meantime, which we do\nsometimes as part of some really large changes, but these can just be\nsquashed, no?\n\n>> Having read this far most of what you have in this 9 patch series\n>> could/should be squashed into something much smaller, e.g. tests being\n>> added for code added in previous steps, let's add the tests along with\n>> the code since this isn't such a large change.\n>\n> In general, I try to make commits as small as possible, but if you\n> prefer tests to be included with the code in the same commit, – that's\n> fine with me too.\n>\n> Will meld new tests into code commits for the next re-roll.\n\nI'm probably the last person to give advice on this list about not\noverly splitting up ones commits :)\n\nHaving said that, some sage advice:\n\nIt's really helpful to split commits into discrete understandable pieces\nwhen it aids in reviewing/understanding the code.\n\nBut something like say your 8/9 is IMNSHO a step to far, you're just\nadding a feature earlier and then docs for it later. That doesn't help\nto review or understand the change, now you just need to look in two\nplaces for what's one logical change.\n\nDitto for e.g. the 5/9 here. That's just a test for a feature added\nearlier. So let's add it to the commit where we add that feature.\n\nThere *are* cases where it helps to split up these changes, but they're\nthings like adding tests for existing behavior before changing\nsomething, as an aid to demonstrate what the behavior was before &\nafter.\n\nIn those cases it's a lot better to split the commits, because nobody\nwants to waste time discerning what's a test for existing v.s. new\nbehavior.\n"},{"id":"421262","messageId":"87k0pc7cap.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"87sg40imit.fsf@evledraar.gmail.com","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T20:26:38Z","receivedAt":"2021-04-08T20:26:43Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Apr 08 2021, Sergey Organov wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> On Thu, Apr 08 2021, Sergey Organov wrote:\n>>>\n>>>> There were 3 completion tests failures due to introduction of\n>>>> log.diffMerges configuration variable that affected the result of\n>>>> completion of log.d. Fixed them accordingly.\n>>>>\n>>>> Signed-off-by: Sergey Organov \n>>>> ---\n>>>> t/t9902-completion.sh | 3 +++\n>>>> 1 file changed, 3 insertions(+)\n>>>>\n>>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>>>> index 04ce884ef5ac..4d732d6d4f81 100755\n>>>> --- a/t/t9902-completion.sh\n>>>> +++ b/t/t9902-completion.sh\n>>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n>>>> \ttest_completion \"git config log.d\" <<-\\EOF\n>>>> \tlog.date Z\n>>>> \tlog.decorate Z\n>>>> +\tlog.diffMerges Z\n>>>> \tEOF\n>>>> '\n>>>> \n>>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n>>>> \ttest_completion \"git -c log.d\" <<-\\EOF\n>>>> \tlog.date=Z\n>>>> \tlog.decorate=Z\n>>>> +\tlog.diffMerges=Z\n>>>> \tEOF\n>>>> '\n>>>> \n>>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n>>>> \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n>>>> \tlog.date=Z\n>>>> \tlog.decorate=Z\n>>>> +\tlog.diffMerges=Z\n>>>> \tEOF\n>>>> '\n>>>\n>>> Commits should be made in such a way as to not break the build/tests\n>>> partway through a series, which it seems is happening until this\n>>> fixup.\n>>\n>> Yep.\n>>\n>> Could these tests be somehow written in a more robust manner, to be\n>> protected against future additions of configuration variables that are\n>> unrelated to the features being tested? If so, I'd prefer to fix them as\n>> a prerequisite to the series rather than adding fixes to unrelated \n>> existing tests into my patches.\n>\n> Hrm? I mean if you have a commit fixing up failing tests in an earlier\n> commit then that change should in one way or the other be made as part\n> of that earlier change.\n>\n> Yes we can skip the tests or something in the meantime, which we do\n> sometimes as part of some really large changes, but these can just be\n> squashed, no?\n\nI mean I don't want this change at all.\n\nI didn't change completion mechanism, so completion tests should not\nsuddenly fail because of my changes. I did entirely unrelated change and\nnoticed the breakage only by accident, as tests even don't fail unless\nyou *install* git, not only make it. So, for example, just \"make test\"\ndoesn't fail, while \"make install; make test\" will.\n\nIt looks like something is wrong here, a bug or misfeature, or even two,\nand if it's fixed before these series, I won't need this in my series at\nall. Besides, that's yet another reason *not* to squash this change into\nan otherwise unrelated commit.\n\n-- Sergey Organov\n"},{"id":"421276","messageId":"20210408213736.GB2947267@szeder.dev","threadId":"55451","inReplyTo":"20210407225608.14611-4-sorganov@gmail.com","subject":"Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-08T21:37:36Z","receivedAt":"2021-04-08T21:37:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:\n> New log.diffMerges configuration variable sets the format that\n> --diff-merges=def will be using. The default is \"separate\".\n> \n> Signed-off-by: Sergey Organov \n> ---\n> builtin/log.c | 2 ++\n> diff-merges.c | 11 +++++++++++\n> diff-merges.h | 2 ++\n> 3 files changed, 15 insertions(+)\n\nPlease don't forget to document this new configuration variable.\n\n"},{"id":"421277","messageId":"20210408215115.GB1938@szeder.dev","threadId":"55451","inReplyTo":"20210408213736.GB2947267@szeder.dev","subject":"Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-08T21:51:15Z","receivedAt":"2021-04-08T21:51:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:\n> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:\n> > New log.diffMerges configuration variable sets the format that\n> > --diff-merges=def will be using. The default is \"separate\".\n> > \n> > Signed-off-by: Sergey Organov \n> > ---\n> > builtin/log.c | 2 ++\n> > diff-merges.c | 11 +++++++++++\n> > diff-merges.h | 2 ++\n> > 3 files changed, 15 insertions(+)\n> \n> Please don't forget to document this new configuration variable.\n\nOh, just noticed that you do document it in the last patch of the\nseries, and, similarly, you add new options early in this patch series\nand add the corresponding documentation in the second to last patch.\nPlease squash in those documentation updates into the corresponding\npatches.\n\n"},{"id":"421278","messageId":"xmqq7dlcsafd.fsf@gitster.g","threadId":"55451","inReplyTo":"20210408215115.GB1938@szeder.dev","subject":"Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T22:01:26Z","receivedAt":"2021-04-08T22:01:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:\n>> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:\n>> > New log.diffMerges configuration variable sets the format that\n>> > --diff-merges=def will be using. The default is \"separate\".\n>> > \n>> > Signed-off-by: Sergey Organov \n>> > ---\n>> > builtin/log.c | 2 ++\n>> > diff-merges.c | 11 +++++++++++\n>> > diff-merges.h | 2 ++\n>> > 3 files changed, 15 insertions(+)\n>> \n>> Please don't forget to document this new configuration variable.\n>\n> Oh, just noticed that you do document it in the last patch of the\n> series, and, similarly, you add new options early in this patch series\n> and add the corresponding documentation in the second to last patch.\n> Please squash in those documentation updates into the corresponding\n> patches.\n\nSince any new topic that adds new configuration variable or update\nthe description of an existing one would interact badly with the\nlast step of Emily's es/config-hooks topic, b58f84c4 (docs: unify\ngithooks and git-hook manpages, 2021-03-10), where the description\nof all the individual options are moved to a newly created file, and\nit is not practical to take all the new topics that touch the\ndocumentation for the configuration variables hostage to the topic\nthat seems to be dormant for quite a while, I'll discard the last\nstep from es/config-hooks topic for now. We really should get it\nmoving soon (or discard and reboot it later---it is getting in the\nway for other topics to keep it in my tree, either way).\n\nThanks.\n"},{"id":"421280","messageId":"20210408221343.GC2947267@szeder.dev","threadId":"55451","inReplyTo":"87k0pc7cap.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-04-08T22:13:43Z","receivedAt":"2021-04-08T22:13:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote:\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > On Thu, Apr 08 2021, Sergey Organov wrote:\n> >\n> >> Ævar Arnfjörð Bjarmason writes:\n> >>\n> >>> On Thu, Apr 08 2021, Sergey Organov wrote:\n> >>>\n> >>>> There were 3 completion tests failures due to introduction of\n> >>>> log.diffMerges configuration variable that affected the result of\n> >>>> completion of log.d. Fixed them accordingly.\n> >>>>\n> >>>> Signed-off-by: Sergey Organov \n> >>>> ---\n> >>>> t/t9902-completion.sh | 3 +++\n> >>>> 1 file changed, 3 insertions(+)\n> >>>>\n> >>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> >>>> index 04ce884ef5ac..4d732d6d4f81 100755\n> >>>> --- a/t/t9902-completion.sh\n> >>>> +++ b/t/t9902-completion.sh\n> >>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n> >>>> \ttest_completion \"git config log.d\" <<-\\EOF\n> >>>> \tlog.date Z\n> >>>> \tlog.decorate Z\n> >>>> +\tlog.diffMerges Z\n> >>>> \tEOF\n> >>>> '\n> >>>> \n> >>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n> >>>> \ttest_completion \"git -c log.d\" <<-\\EOF\n> >>>> \tlog.date=Z\n> >>>> \tlog.decorate=Z\n> >>>> +\tlog.diffMerges=Z\n> >>>> \tEOF\n> >>>> '\n> >>>> \n> >>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n> >>>> \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n> >>>> \tlog.date=Z\n> >>>> \tlog.decorate=Z\n> >>>> +\tlog.diffMerges=Z\n> >>>> \tEOF\n> >>>> '\n> >>>\n> >>> Commits should be made in such a way as to not break the build/tests\n> >>> partway through a series, which it seems is happening until this\n> >>> fixup.\n\nWell, actually no: it _starts_ to break with this patch, because\n'log.diffMerges' is not documented yet, and it will pass again with\nthe last patch in the series that adds that missing piece of\ndocumentation.\n\n> >> Yep.\n> >>\n> >> Could these tests be somehow written in a more robust manner, to be\n> >> protected against future additions of configuration variables that are\n> >> unrelated to the features being tested? If so, I'd prefer to fix them as\n> >> a prerequisite to the series rather than adding fixes to unrelated \n> >> existing tests into my patches.\n> >\n> > Hrm? I mean if you have a commit fixing up failing tests in an earlier\n> > commit then that change should in one way or the other be made as part\n> > of that earlier change.\n> >\n> > Yes we can skip the tests or something in the meantime, which we do\n> > sometimes as part of some really large changes, but these can just be\n> > squashed, no?\n> \n> I mean I don't want this change at all.\n\nYou'll definitely need this change, though.\n\n> I didn't change completion mechanism, so completion tests should not\n> suddenly fail because of my changes.\n\nWe auto-generate the list of supported configuration variables from\nthe documentation and use that list in our Bash completion script to\nlist possible configuration variables for 'git config ' and 'git\n-c '. And we want to make sure that this feature works as\nintended, so we have a couple of tests that try to complete real\nconfig variable sections and names. You are just unlucky to introduce\na new configuraton variable that happenes to start with the same\nprefix that is used in some of those tests.\n\n> I did entirely unrelated change and\n> noticed the breakage only by accident, as tests even don't fail unless\n> you *install* git, not only make it. So, for example, just \"make test\"\n> doesn't fail, while \"make install; make test\" will.\n\nIt might be related to a bug in the build process that doesn't update\nthat auto-generated list of supported configuration variables after\ne.g. 'Documentation/config/log.txt' was modified; see a proposed fix\nat:\n\n https://public-inbox.org/git/20210408212915.3060286-1-szeder.dev@gmail.com/\n\n> It looks like something is wrong here, a bug or misfeature, or even two,\n> and if it's fixed before these series, I won't need this in my series at\n> all. Besides, that's yet another reason *not* to squash this change into\n> an otherwise unrelated commit.\n\nThe introduction of the new configuration variable, its documentation\nand this test update should all go into a single patch. The whole\ntest suite must pass for every single commit.\n\n"},{"id":"421293","messageId":"87lf9s5qew.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"20210408215115.GB1938@szeder.dev","subject":"Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T23:04:39Z","receivedAt":"2021-04-08T23:04:47Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"SZEDER Gábor writes:\n\n> On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:\n>> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:\n>> > New log.diffMerges configuration variable sets the format that\n>> > --diff-merges=def will be using. The default is \"separate\".\n>> > \n>> > Signed-off-by: Sergey Organov \n>> > ---\n>> > builtin/log.c | 2 ++\n>> > diff-merges.c | 11 +++++++++++\n>> > diff-merges.h | 2 ++\n>> > 3 files changed, 15 insertions(+)\n>> \n>> Please don't forget to document this new configuration variable.\n>\n> Oh, just noticed that you do document it in the last patch of the\n> series, and, similarly, you add new options early in this patch series\n> and add the corresponding documentation in the second to last patch.\n> Please squash in those documentation updates into the corresponding\n> patches.\n\nSorry, I fail to see how to do it that way, as documentation has mutual\nreferences between --diff-merges=def and log.diffMerges, that are\nintroduced in different commits.\n\nThat said, after squashing tests into corresponding code commits, that\nhas been already requested, the documentation updates will be closer to\nthe code commits. Is it OK with you then to leave documentation changes\nas separate commits?\n\nAlso, Junio, please clarify what you prefer as maintainer, fine-grained\ncommits, or squash everything even remotely relevant into a single one?\n\nI honestly fail to see where the preferred margin is, and start to get a\nfeeling that I'd better squash everything together.\n\nThanks,\n\n-- Sergey Organov\n"},{"id":"421294","messageId":"87h7kg5qad.fsf@osv.gnss.ru","threadId":"55451","inReplyTo":"20210408221343.GC2947267@szeder.dev","subject":"Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-08T23:07:22Z","receivedAt":"2021-04-08T23:07:26Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"SZEDER Gábor writes:\n\n> On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote:\n\n[...]\n\n>\n>> It looks like something is wrong here, a bug or misfeature, or even two,\n>> and if it's fixed before these series, I won't need this in my series at\n>> all. Besides, that's yet another reason *not* to squash this change into\n>> an otherwise unrelated commit.\n>\n> The introduction of the new configuration variable, its documentation\n> and this test update should all go into a single patch. The whole\n> test suite must pass for every single commit.\n\nOK, fine, thanks for clarification!\n\n-- Sergey Organov\n"},{"id":"421552","messageId":"20210410171657.20159-2-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"[PATCH v1 1/5] diff-merges: introduce --diff-merges=default","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:53Z","receivedAt":"2021-04-10T17:17:32Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Introduce the notion of default diff format for merges, and the option\n\"default\" to select it. The default format is \"separate\" and can't yet\nbe changed, so effectively \"default\" is just a synonym for \"separate\"\nfor now. Add corresponding test to t4013.\n\nThis is in preparation for introducing log.diffMerges configuration\noption that will let --diff-merges=default to be configured to any\nsupported format.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 7 +++++++\n t/t4013-diff-various.sh | 8 ++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 146bb50316a6..7690580d7464 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -2,6 +2,11 @@\n \n #include \"revision.h\"\n \n+typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n+static void set_separate(struct rev_info *revs);\n+\n+static diff_merges_setup_func_t set_to_default = set_separate;\n+\n static void suppress(struct rev_info *revs)\n {\n \trevs->separate_merges = 0;\n@@ -66,6 +71,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n \t\tset_combined(revs);\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\tset_dense_combined(revs);\n+\telse if (!strcmp(optarg, \"default\"))\n+\t\tset_to_default(revs);\n \telse\n \t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n \ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 6cca8b84a6bf..8acb5b866900 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n+test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '\n+\tgit log -p --diff-merges=separate master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log -p --diff-merges=default master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -S requires an argument' '\n \ttest_must_fail git log -S\n '\n-- \n2.25.1\n\n"},{"id":"421553","messageId":"20210410171657.20159-1-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210407225608.14611-1-sorganov@gmail.com","subject":"[PATCH v1 0/5] git log: configurable default format for merge diffs","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:52Z","receivedAt":"2021-04-10T17:17:32Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"These patches introduce capability to configure the default format of\noutput of diffs for merge commits by means of new log.diffMerges\nconfiguration variable. The default format is then used by -m,\n--diff-merges=m, and new --diff-merges=default options.\n\nIn particular,\n\n git config log.diffMerges first-parent\n\nwill change -m option format from \"separate\" to \"first-parent\" that\nwill in turn cause, say,\n\n git show -m \n\nto output diff to the first parent only, instead of appending\ntypically large and surprising diff to the second parent at the end of\nthe output.\n\nUpdates in v1:\n\n * Renamed abbreviated value \"def\" to full \"default\"\n\n * Fixed tests to use \"test_config\" instead of \"git config\"\n\n * Meld all \"git config\" changes into single commit that includes\n code, documentation, and tests, as they are mutually\n interdependent.\n\nSergey Organov (5):\n diff-merges: introduce --diff-merges=default\n diff-merges: refactor set_diff_merges()\n diff-merges: adapt -m to enable default diff format\n diff-merges: introduce log.diffMerges config variable\n doc/diff-options: document new --diff-merges features\n\n Documentation/config/log.txt | 5 +++\n Documentation/diff-options.txt | 15 ++++++---\n builtin/log.c | 2 ++\n diff-merges.c | 58 ++++++++++++++++++++++++----------\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 31 ++++++++++++++++++\n t/t9902-completion.sh | 3 ++\n 7 files changed, 95 insertions(+), 21 deletions(-)\n\nInterdiff against v0:\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 09b07231b5a4..31e2bacf5252 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -34,7 +34,7 @@ endif::git-diff[]\n endif::git-format-patch[]\n \n ifdef::git-log[]\n---diff-merges=(off|none|def|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n+--diff-merges=(off|none|default|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n --no-diff-merges::\n \tSpecify diff format to be used for merge commits. Default is\n \t{diff-merges-default} unless `--first-parent` is in use, in which case\n@@ -45,7 +45,7 @@ ifdef::git-log[]\n \tDisable output of diffs for merge commits. Useful to override\n \timplied value.\n +\n---diff-merges=def:::\n+--diff-merges=default:::\n --diff-merges=m:::\n -m:::\n \tThis option makes diff output for merge commits to be shown in\ndiff --git a/diff-merges.c b/diff-merges.c\nindex f68e4376fd63..75630fb8e6b8 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -67,7 +67,7 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n \t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\treturn set_dense_combined;\n-\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"def\"))\n+\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"default\"))\n \t\treturn set_to_default;\n \treturn NULL;\n }\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex ee4afca06ced..87cab7867135 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -452,37 +452,34 @@ diff-tree --stat --compact-summary initial mode\n diff-tree -R --stat --compact-summary initial mode\n EOF\n \n-test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '\n+test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '\n \tgit log -p --diff-merges=separate master >result &&\n \tprocess_diffs result >expected &&\n-\tgit log -p --diff-merges=def master >result &&\n+\tgit log -p --diff-merges=default master >result &&\n \tprocess_diffs result >actual &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'deny wrong log.diffMerges config' '\n-\tgit config log.diffMerges wrong-value &&\n-\ttest_expect_code 128 git log &&\n-\tgit config --unset log.diffMerges\n+\ttest_config log.diffMerges wrong-value &&\n+\ttest_expect_code 128 git log\n '\n \n test_expect_success 'git config log.diffMerges first-parent' '\n \tgit log -p --diff-merges=first-parent master >result &&\n \tprocess_diffs result >expected &&\n-\tgit config log.diffMerges first-parent &&\n-\tgit log -p --diff-merges=def master >result &&\n+\ttest_config log.diffMerges first-parent &&\n+\tgit log -p --diff-merges=default master >result &&\n \tprocess_diffs result >actual &&\n-\tgit config --unset log.diffMerges &&\n \ttest_cmp expected actual\n '\n \n test_expect_success 'git config log.diffMerges first-parent vs -m' '\n \tgit log -p --diff-merges=first-parent master >result &&\n \tprocess_diffs result >expected &&\n-\tgit config log.diffMerges first-parent &&\n+\ttest_config log.diffMerges first-parent &&\n \tgit log -p -m master >result &&\n \tprocess_diffs result >actual &&\n-\tgit config --unset log.diffMerges &&\n \ttest_cmp expected actual\n '\n \n-- \n2.25.1\n\n"},{"id":"421554","messageId":"20210410171657.20159-3-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"[PATCH v1 2/5] diff-merges: refactor set_diff_merges()","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:54Z","receivedAt":"2021-04-10T17:17:32Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Split set_diff_merges() into separate parsing and execution functions,\nthe former to be reused for parsing of configuration values later in\nthe patch series.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 36 +++++++++++++++++++++---------------\n 1 file changed, 21 insertions(+), 15 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 7690580d7464..9918b6ac55e4 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -55,29 +55,35 @@ static void set_dense_combined(struct rev_info *revs)\n \trevs->dense_combined_merges = 1;\n }\n \n-static void set_diff_merges(struct rev_info *revs, const char *optarg)\n+static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n-\tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\")) {\n-\t\tsuppress(revs);\n-\t\t/* Return early to leave revs->merges_need_diff unset */\n-\t\treturn;\n-\t}\n-\n+\tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n+\t\treturn suppress;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n-\t\tset_first_parent(revs);\n+\t\treturn set_first_parent;\n \telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"separate\"))\n-\t\tset_separate(revs);\n+\t\treturn set_separate;\n \telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n-\t\tset_combined(revs);\n+\t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n-\t\tset_dense_combined(revs);\n+\t\treturn set_dense_combined;\n \telse if (!strcmp(optarg, \"default\"))\n-\t\tset_to_default(revs);\n-\telse\n+\t\treturn set_to_default;\n+\treturn NULL;\n+}\n+\n+static void set_diff_merges(struct rev_info *revs, const char *optarg)\n+{\n+\tdiff_merges_setup_func_t func = func_by_opt(optarg);\n+\n+\tif (!func)\n \t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n \n-\t/* The flag is cleared by set_xxx() functions, so don't move this up */\n-\trevs->merges_need_diff = 1;\n+\tfunc(revs);\n+\n+\t/* NOTE: the merges_need_diff flag is cleared by func() call */\n+\tif (func != suppress)\n+\t\trevs->merges_need_diff = 1;\n }\n \n /*\n-- \n2.25.1\n\n"},{"id":"421555","messageId":"20210410171657.20159-4-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"[PATCH v1 3/5] diff-merges: adapt -m to enable default diff format","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:55Z","receivedAt":"2021-04-10T17:17:35Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Let -m option (and --diff-merges=m) enable the default format instead\nof \"separate\", to be able to tune it with log.diffMerges option.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 9918b6ac55e4..a02f39828336 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -34,10 +34,10 @@ static void set_m(struct rev_info *revs)\n {\n \t/*\n \t * To \"diff-index\", \"-m\" means \"match missing\", and to the \"log\"\n-\t * family of commands, it means \"show full diff for merges\". Set\n+\t * family of commands, it means \"show default diff for merges\". Set\n \t * both fields appropriately.\n \t */\n-\tset_separate(revs);\n+\tset_to_default(revs);\n \trevs->match_missing = 1;\n }\n \n@@ -61,13 +61,13 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n \t\treturn suppress;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n-\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"separate\"))\n+\telse if (!strcmp(optarg, \"separate\"))\n \t\treturn set_separate;\n \telse if (!strcmp(optarg, \"c\") || !strcmp(optarg, \"combined\"))\n \t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\treturn set_dense_combined;\n-\telse if (!strcmp(optarg, \"default\"))\n+\telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"default\"))\n \t\treturn set_to_default;\n \treturn NULL;\n }\n-- \n2.25.1\n\n"},{"id":"421556","messageId":"20210410171657.20159-5-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"[PATCH v1 4/5] diff-merges: introduce log.diffMerges config variable","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:56Z","receivedAt":"2021-04-10T17:17:35Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"New log.diffMerges configuration variable sets the format that\n--diff-merges=default will be using. The default is \"separate\".\n\nt4013: add the following tests for log.diffMerges config:\n\n* Test that wrong values are denied.\n\n* Test that the value of log.diffMerges properly affects both\n--diff-merges=default and -m.\n\nt9902: fix completion tests for log.d* to match log.diffMerges.\n\nAdded documentation for log.diffMerges.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 5 +++++\n builtin/log.c | 2 ++\n diff-merges.c | 11 +++++++++++\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 23 +++++++++++++++++++++++\n t/t9902-completion.sh | 3 +++\n 6 files changed, 46 insertions(+)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 208d5fdcaa68..456eb07800cb 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -24,6 +24,11 @@ log.excludeDecoration::\n \tthe config option can be overridden by the `--decorate-refs`\n \toption.\n \n+log.diffMerges::\n+\tSet default diff format to be used for merge commits. See\n+\t`--diff-merges` in linkgit:git-log[1] for details.\n+\tDefaults to `separate`.\n+\n log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\n \ta single is given. This has the same limitations as `--follow`,\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8acd285dafd8..6102893fccb9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -481,6 +481,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"log.diffmerges\"))\n+\t\treturn diff_merges_config(value);\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/diff-merges.c b/diff-merges.c\nindex a02f39828336..75630fb8e6b8 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -90,6 +90,17 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)\n * Public functions. They are in the order they are called.\n */\n \n+int diff_merges_config(const char *value)\n+{\n+\tdiff_merges_setup_func_t func = func_by_opt(value);\n+\n+\tif (!func)\n+\t\treturn -1;\n+\n+\tset_to_default = func;\n+\treturn 0;\n+}\n+\n int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n {\n \tint argcount = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 659467c99a4f..09d9a6c9a4fb 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -9,6 +9,8 @@\n \n struct rev_info;\n \n+int diff_merges_config(const char *value);\n+\n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\n \n void diff_merges_suppress(struct rev_info *revs);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 8acb5b866900..87cab7867135 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -460,6 +460,29 @@ test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'deny wrong log.diffMerges config' '\n+\ttest_config log.diffMerges wrong-value &&\n+\ttest_expect_code 128 git log\n+'\n+\n+test_expect_success 'git config log.diffMerges first-parent' '\n+\tgit log -p --diff-merges=first-parent master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges first-parent &&\n+\tgit log -p --diff-merges=default master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git config log.diffMerges first-parent vs -m' '\n+\tgit log -p --diff-merges=first-parent master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges first-parent &&\n+\tgit log -p -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'log -S requires an argument' '\n \ttest_must_fail git log -S\n '\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 04ce884ef5ac..4d732d6d4f81 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '\n \ttest_completion \"git config log.d\" <<-\\EOF\n \tlog.date Z\n \tlog.decorate Z\n+\tlog.diffMerges Z\n \tEOF\n '\n \n@@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '\n \ttest_completion \"git -c log.d\" <<-\\EOF\n \tlog.date=Z\n \tlog.decorate=Z\n+\tlog.diffMerges=Z\n \tEOF\n '\n \n@@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '\n \ttest_completion \"git clone --config=log.d\" <<-\\EOF\n \tlog.date=Z\n \tlog.decorate=Z\n+\tlog.diffMerges=Z\n \tEOF\n '\n \n-- \n2.25.1\n\n"},{"id":"421557","messageId":"20210410171657.20159-6-sorganov@gmail.com","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"[PATCH v1 5/5] doc/diff-options: document new --diff-merges features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-04-10T17:16:57Z","receivedAt":"2021-04-10T17:17:37Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Document changes in -m and --diff-merges=m semantics, as well as new\n--diff-merges=default option.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/diff-options.txt | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex aa2b5c11f20b..31e2bacf5252 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -34,7 +34,7 @@ endif::git-diff[]\n endif::git-format-patch[]\n \n ifdef::git-log[]\n---diff-merges=(off|none|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n+--diff-merges=(off|none|default|first-parent|1|separate|m|combined|c|dense-combined|cc)::\n --no-diff-merges::\n \tSpecify diff format to be used for merge commits. Default is\n \t{diff-merges-default} unless `--first-parent` is in use, in which case\n@@ -45,17 +45,24 @@ ifdef::git-log[]\n \tDisable output of diffs for merge commits. Useful to override\n \timplied value.\n +\n+--diff-merges=default:::\n+--diff-merges=m:::\n+-m:::\n+\tThis option makes diff output for merge commits to be shown in\n+\tthe default format. `-m` will produce the output only if `-p`\n+\tis given as well. The default format could be changed using\n+\t`log.diffMerges` configuration parameter, which default value\n+\tis `separate`.\n++\n --diff-merges=first-parent:::\n --diff-merges=1:::\n \tThis option makes merge commits show the full diff with\n \trespect to the first parent only.\n +\n --diff-merges=separate:::\n---diff-merges=m:::\n--m:::\n \tThis makes merge commits show the full diff with respect to\n \teach of the parents. Separate log entry and diff is generated\n-\tfor each parent. `-m` doesn't produce any output without `-p`.\n+\tfor each parent.\n +\n --diff-merges=combined:::\n --diff-merges=c:::\n-- \n2.25.1\n\n"},{"id":"421609","messageId":"xmqqsg3whka6.fsf@gitster.g","threadId":"55451","inReplyTo":"20210410171657.20159-1-sorganov@gmail.com","subject":"Re: [PATCH v1 0/5] git log: configurable default format for merge diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T16:13:05Z","receivedAt":"2021-04-11T16:13:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> These patches introduce capability to configure the default format of\n> output of diffs for merge commits by means of new log.diffMerges\n> configuration variable. The default format is then used by -m,\n> --diff-merges=m, and new --diff-merges=default options.\n>\n> In particular,\n>\n> git config log.diffMerges first-parent\n>\n> will change -m option format from \"separate\" to \"first-parent\" that\n> will in turn cause, say,\n>\n> git show -m \n>\n> to output diff to the first parent only, instead of appending\n> typically large and surprising diff to the second parent at the end of\n> the output.\n\nI think that it is a good goal to free a short-and-sweet \"-m\" from\ngetting tied forever to the current \"two-tree diff for each of the\nparent\" (aka \"separate\"), and a configuration to change what the\n\"-m\" option means would be a good approach to do so. It would help\nthe interactive use by human end-users, which is the point of having\nshort-and-sweet options. Existing scripts may depend on the current\nbehaviour, so the configuration cannot be introduced right away, but\nover time they can be migrated to use the longer and more explicit\noption \"--diff-merges=separate\".\n\nBut I do not see much point in adding the \"--diff-merges=default\".\nWho is the target audience? Certainly not scripts that want to\navoid depending on the 'default' that can be different and easily\nvary per user.\n\nOr is the plan to deprecate and remove the short-and-sweet \"-m\"\noption and standardize on \"--diff-merges=