{"thread":{"id":"58858","subject":"[PATCH 0/5] diff-merges: more features","startedAt":"2022-11-27T09:37:36Z","lastAt":"2022-12-19T14:29:22Z","messageCount":39,"participants":["Sergey Organov","Junio C Hamano","Elijah Newren","Ævar Arnfjörð Bjarmason","Jeff King","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"468037","messageId":"20221127093721.31012-1-sorganov@gmail.com","threadId":"58858","inReplyTo":null,"subject":"[PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:16Z","receivedAt":"2022-11-27T09:37:36Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"1. --diff-merges=[no]-hide\n\nThe set of diff-merges options happened to be incomplete, and failed\nto implement exact semantics of -m option that hides output of diffs\nfor merge commits unless -p option is active as well.\n\nThe new \"hide\" option fixes this issue, so that now\n\n --diff-merges=on --diff-merges=hide\n\ncombo is the exact synonym for -m.\n\nThe log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\nvalues, and governs the default value for the hide bit. The\nconfiguration behaves as if \"--diff-merges=[no-]hide\" is inserted\nfirst in the command-line.\n\n2. log.diffMerges-m-imply-p\n\nHistorically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\noptions do. Simply fixing this inconsistency by unconditional\nmodification of '-m' semantics appeared to be a bad idea, as it broke\nsome legacy scripts/aliases. This patch rather provides configuration\nvariable to tweak '-m' behavior accordingly.\n\n3. log.diffMergesForce\n\nForce specific log format for -c, --cc, and --remerge-diff options\ninstead of their respective formats. The override is useful when some\nexternal tool hard-codes diff for merges format option.\n\n4. Support list of values for --diff-merges\n\nThis allows for shorter --diff-merges=on,hide forms.\n\n5. Issue warning for lone '-m'.\n\nLone '-m' is in use by scripts/aliases that aim at enabling diff\noutput for merge commits, but only if '-p' is then specified as well.\n\nAs '-m' may now be configured to imply '-p' (using\n'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,\nand suggest to instead use '--diff-merges=on,hide' that does not\ndepend on user configuration.\n\nThis is expected to give a provision for enabling\nlog.diffMerges-m-imply-p by default in the future.\n\nSergey Organov (5):\n diff-merges: implement [no-]hide option and log.diffMergesHide config\n diff-merges: implement log.diffMerges-m-imply-p config\n diff-merges: implement log.diffMergesForce config\n diff-merges: support list of values for --diff-merges\n diff-merges: issue warning on lone '-m' option\n\n Documentation/config/log.txt | 20 ++++\n Documentation/diff-options.txt | 20 +++-\n builtin/log.c | 6 +\n diff-merges.c | 108 +++++++++++++++---\n diff-merges.h | 6 +\n t/t4013-diff-various.sh | 89 ++++++++++++++-\n ...ges=first-parent_--diff-merges=hide_master | 34 ++++++\n t/t9902-completion.sh | 9 ++\n 8 files changed, 272 insertions(+), 20 deletions(-)\n create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n\n-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468038","messageId":"20221127093721.31012-2-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"[PATCH 1/5] diff-merges: implement [no-]hide option and log.diffMergesHide config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:17Z","receivedAt":"2022-11-27T09:37:40Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"The set of diff-merges options happened to be incomplete, and failed\nto implement exact semantics of -m option that hides output of diffs\nfor merge commits unless -p option is active as well.\n\nThe new \"hide\" option fixes this issue, so that now\n\n --diff-merges=on --diff-merges=hide\n\ncombo is the exact synonym for -m.\n\nThe log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\nvalues, and governs the default value for the hide bit. The\nconfiguration behaves as if \"--diff-merges=[no-]hide\" is inserted\nfirst in the command-line.\n\nNew log.diffMergesHide boolean configuration is implemented as well,\nfor the case when log.diffMerges is already in use for specifying the\nformat.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 6 +++\n Documentation/diff-options.txt | 13 +++--\n builtin/log.c | 2 +\n diff-merges.c | 40 ++++++++++++--\n diff-merges.h | 2 +\n t/t4013-diff-various.sh | 54 ++++++++++++++++++-\n ...ges=first-parent_--diff-merges=hide_master | 34 ++++++++++++\n t/t9902-completion.sh | 3 ++\n 8 files changed, 147 insertions(+), 7 deletions(-)\n create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex cbe34d759221..9c6be643bcf6 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -33,6 +33,12 @@ log.diffMerges::\n \tSet diff format to be used when `--diff-merges=on` is\n \tspecified, see `--diff-merges` in linkgit:git-log[1] for\n \tdetails. Defaults to `separate`.\n++\n+When used with 'hide' or 'no-hide', sets the default hiding of\n+diffs for merge commits when `-p` option is not used.\n+\n+log.diffMergesHide::\n+\t`true` implies `--diff-merges=hide` option.\n \n log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92c..351d51215717 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|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::\n+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::\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@@ -49,10 +49,11 @@ ifdef::git-log[]\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+\tthe default format. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n++\n+\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n +\n --diff-merges=first-parent:::\n --diff-merges=1:::\n@@ -72,6 +73,12 @@ ifdef::git-log[]\n \twith conflict markers and such. A diff is then shown between\n \tthat temporary tree and the actual merge commit.\n +\n+--diff-merges=hide:::\n+--diff-merges=no-hide:::\n+\tHide (do not hide) the diff for merge commits unless `-p` options is given\n+\tas well. The default `no-hide` could be changed using `log.diffMerges`\n+\tconfiguration parameter.\n++\n The output emitted when this option is used is subject to change, and\n so is its interaction with other options (unless explicitly\n documented).\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56e2d95e869d..e031021e53b2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -581,6 +581,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"log.diffmerges\"))\n \t\treturn diff_merges_config(value);\n+\tif (!strcmp(var, \"log.diffmergeshide\"))\n+\t\treturn diff_merges_hide_config(git_config_bool(var, 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 85cbefa5afd7..55fe5b70c102 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -7,6 +7,7 @@ static void set_separate(struct rev_info *revs);\n \n static diff_merges_setup_func_t set_to_default = set_separate;\n static int suppress_m_parsing;\n+static int hide = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -20,10 +21,15 @@ static void suppress(struct rev_info *revs)\n \trevs->remerge_diff = 0;\n }\n \n+static void set_need_diff(struct rev_info *revs)\n+{\n+\trevs->merges_need_diff = !hide;\n+}\n+\n static void common_setup(struct rev_info *revs)\n {\n \tsuppress(revs);\n-\trevs->merges_need_diff = 1;\n+\tset_need_diff(revs);\n }\n \n static void set_none(struct rev_info *revs)\n@@ -31,6 +37,18 @@ static void set_none(struct rev_info *revs)\n \tsuppress(revs);\n }\n \n+static void set_hide(struct rev_info *revs)\n+{\n+\thide = 1;\n+\tset_need_diff(revs);\n+}\n+\n+static void set_no_hide(struct rev_info *revs)\n+{\n+\thide = 0;\n+\tset_need_diff(revs);\n+}\n+\n static void set_separate(struct rev_info *revs)\n {\n \tcommon_setup(revs);\n@@ -69,6 +87,10 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n \t\treturn set_none;\n+\tif (!strcmp(optarg, \"hide\"))\n+\t\treturn set_hide;\n+\tif (!strcmp(optarg, \"no-hide\"))\n+\t\treturn set_no_hide;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n \tif (!strcmp(optarg, \"separate\"))\n@@ -105,7 +127,19 @@ int diff_merges_config(const char *value)\n \tif (!func)\n \t\treturn -1;\n \n-\tset_to_default = func;\n+\tif (func == set_hide)\n+\t\thide = 1;\n+\telse if (func == set_no_hide)\n+\t\thide = 0;\n+\telse\n+\t\tset_to_default = func;\n+\n+\treturn 0;\n+}\n+\n+int diff_merges_hide_config(int on)\n+{\n+\thide = on;\n \treturn 0;\n }\n \n@@ -122,7 +156,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n-\t\trevs->merges_need_diff = 0;\n+\t\tset_hide(revs);\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 19639689bb05..e86e5381693b 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -11,6 +11,8 @@ struct rev_info;\n \n int diff_merges_config(const char *value);\n \n+int diff_merges_hide_config(int hide);\n+\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex dfcf3a0aaae3..4fc8ba2fc59c 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -334,6 +334,7 @@ log --first-parent --diff-merges=off -p master\n log -p --first-parent master\n log -p --diff-merges=first-parent master\n log --diff-merges=first-parent master\n+log --diff-merges=first-parent --diff-merges=hide master\n log -m -p --first-parent master\n log -m -p master\n log --cc -m -p master\n@@ -460,7 +461,18 @@ EOF\n test_expect_success 'log -m matches pure log' '\n \tgit log master >result &&\n \tprocess_diffs result >expected &&\n-\tgit log -m >result &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'log --diff-merges=on matches -m only with --diff-merges=hide' '\n+\tgit log -m master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >actual &&\n+\t! test_cmp expected actual &&\n+\tgit log --diff-merges=on --diff-merges=hide master >result &&\n \tprocess_diffs result >actual &&\n \ttest_cmp expected actual\n '\n@@ -496,6 +508,46 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git config log.diffMerges hide: has effect' '\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >no-hide &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >hide &&\n+\t! test_cmp no-hide hide\n+'\n+\n+test_expect_success 'git config log.diffMerges no-hide: is the default' '\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >default &&\n+\ttest_config log.diffMerges no-hide &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >no-hide &&\n+\ttest_cmp default no-hide\n+'\n+\n+# As \"-m\" is synonym for \"--diff-merges=hide --diff-merges=on\", the\n+# \"log.diffMerges=hide\" configuration should have no effect on \"-m\"\n+test_expect_success 'git config log.diffMerges hide: has no effect on -m' '\n+\tgit log -m master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# As \"--cc\" implies \"-p\", the \"log.diffMerges=hide\" configuration\n+# should have no effect on \"--cc\"\n+test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\nnew file mode 100644\nindex 000000000000..596caec64298\n--- /dev/null\n+++ b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n@@ -0,0 +1,34 @@\n+$ git log --diff-merges=first-parent --diff-merges=hide master\n+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64\n+Merge: 9a6d494 c7a2ab9\n+Author: A U Thor \n+Date: Mon Jun 26 00:04:00 2006 +0000\n+\n+ Merge branch 'side'\n+\n+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a\n+Author: A U Thor \n+Date: Mon Jun 26 00:03:00 2006 +0000\n+\n+ Side\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \n+Date: Mon Jun 26 00:02:00 2006 +0000\n+\n+ Third\n+\n+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44\n+Author: A U Thor \n+Date: Mon Jun 26 00:01:00 2006 +0000\n+\n+ Second\n+ \n+ This is the second commit.\n+\n+commit 444ac553ac7612cc88969031b02b3767fb8a353a\n+Author: A U Thor \n+Date: Mon Jun 26 00:00:00 2006 +0000\n+\n+ Initial\n+$\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 43de868b8005..fc6b0722216a 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2497,6 +2497,7 @@ test_expect_success 'git config - variable name' '\n \tlog.date Z\n \tlog.decorate Z\n \tlog.diffMerges Z\n+\tlog.diffMergesHide Z\n \tEOF\n '\n \n@@ -2525,6 +2526,7 @@ test_expect_success 'git -c - variable name' '\n \tlog.date=Z\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n+\tlog.diffMergesHide=Z\n \tEOF\n '\n \n@@ -2547,6 +2549,7 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.date=Z\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n+\tlog.diffMergesHide=Z\n \tEOF\n '\n \n-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468039","messageId":"20221127093721.31012-3-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"[PATCH 2/5] diff-merges: implement log.diffMerges-m-imply-p config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:18Z","receivedAt":"2022-11-27T09:37:44Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Historically, `-m` doesn't imply `-p` whereas similar `-c` and `--cc`\noptions do. Simply fixing this inconsistency by unconditional\nmodification of `-m` semantics appeared to be a bad idea, as it broke\nsome legacy scripts.\n\nImplement \"log.diffMerges-m-imply-p\" boolean configuration variable\nthat allows user to enable implication of `-p` by `-m`.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 3 +++\n Documentation/diff-options.txt | 6 ++++--\n builtin/log.c | 2 ++\n diff-merges.c | 8 ++++++++\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 9 +++++++++\n t/t9902-completion.sh | 3 +++\n 7 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 9c6be643bcf6..265a57312e58 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -40,6 +40,9 @@ diffs for merge commits when `-p` option is not used.\n log.diffMergesHide::\n \t`true` implies `--diff-merges=hide` option.\n \n+log.diffMerges-m-imply-p::\n+\t`true` enables implication of `-p` by `-m`.\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/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 351d51215717..e64066af8d96 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -48,12 +48,14 @@ ifdef::git-log[]\n --diff-merges=on:::\n --diff-merges=m:::\n -m:::\n-\tThis option makes diff output for merge commits to be shown in\n+\tThese options make diff output for merge commits to be shown in\n \tthe default format. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n+\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.\n+\tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n+\tactive.\n +\n --diff-merges=first-parent:::\n --diff-merges=1:::\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e031021e53b2..332b5e478cc5 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -583,6 +583,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn diff_merges_config(value);\n \tif (!strcmp(var, \"log.diffmergeshide\"))\n \t\treturn diff_merges_hide_config(git_config_bool(var, value));\n+\tif (!strcmp(var, \"log.diffmerges-m-imply-p\"))\n+\t\treturn diff_merges_m_imply_p_config(git_config_bool(var, 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 55fe5b70c102..1fbf476d378e 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -8,6 +8,7 @@ static void set_separate(struct rev_info *revs);\n static diff_merges_setup_func_t set_to_default = set_separate;\n static int suppress_m_parsing;\n static int hide = 0;\n+static int m_imply_p = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -143,6 +144,12 @@ int diff_merges_hide_config(int on)\n \treturn 0;\n }\n \n+int diff_merges_m_imply_p_config(int on)\n+{\n+\tm_imply_p = on;\n+\treturn 0;\n+}\n+\n void diff_merges_suppress_m_parsing(void)\n {\n \tsuppress_m_parsing = 1;\n@@ -157,6 +164,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n \t\tset_hide(revs);\n+\t\trevs->merges_imply_patch = m_imply_p;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex e86e5381693b..9f0b3901fe82 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -13,6 +13,8 @@ int diff_merges_config(const char *value);\n \n int diff_merges_hide_config(int hide);\n \n+int diff_merges_m_imply_p_config(int on);\n+\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 4fc8ba2fc59c..1789dd6063c5 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -548,6 +548,15 @@ test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n+\tgit log -m -p master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges-m-imply-p true &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex fc6b0722216a..26a7e4ff877c 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '\n \tlog.decorate Z\n \tlog.diffMerges Z\n \tlog.diffMergesHide Z\n+\tlog.diffMerges-m-imply-p Z\n \tEOF\n '\n \n@@ -2527,6 +2528,7 @@ test_expect_success 'git -c - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n \n@@ -2550,6 +2552,7 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n \n-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468040","messageId":"20221127093721.31012-4-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"[PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:19Z","receivedAt":"2022-11-27T09:37:45Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Force specified log format for -c, --cc, and --remerge-diff options\ninstead of their respective formats. The override is useful when some\nexternal tool hard-codes diff for merges format option.\n\nUsing any of the above options twice or more will get back the\noriginal meaning of the option no matter what configuration says.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 11 +++++++++++\n builtin/log.c | 2 ++\n diff-merges.c | 32 ++++++++++++++++++++++++++------\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 18 ++++++++++++++++++\n t/t9902-completion.sh | 3 +++\n 6 files changed, 62 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 265a57312e58..7452c7fad638 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -43,6 +43,17 @@ log.diffMergesHide::\n log.diffMerges-m-imply-p::\n \t`true` enables implication of `-p` by `-m`.\n \n+log.diffMergesForce::\n+\tUse specified log format for -c, --cc, and --remerge-diff\n+\toptions instead of their respective formats when the option\n+\tappears on the command line one time. See `--diff-merges` in\n+\tlinkgit:git-log[1] for allowed values. Using 'off' or 'none'\n+\tdisables the override (default).\n++\n+The override is useful when external tool hard-codes one of the above\n+options. Using any of these options two (or more) times will get back\n+the original meaning of the options.\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 332b5e478cc5..1e8d0a2545a9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -585,6 +585,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn diff_merges_hide_config(git_config_bool(var, value));\n \tif (!strcmp(var, \"log.diffmerges-m-imply-p\"))\n \t\treturn diff_merges_m_imply_p_config(git_config_bool(var, value));\n+\tif (!strcmp(var, \"log.diffmergesforce\"))\n+\t\treturn diff_merges_force_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 1fbf476d378e..cedd7652bf42 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -6,6 +6,7 @@ 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+static diff_merges_setup_func_t force_func = NULL;\n static int suppress_m_parsing;\n static int hide = 0;\n static int m_imply_p = 0;\n@@ -150,6 +151,21 @@ int diff_merges_m_imply_p_config(int on)\n \treturn 0;\n }\n \n+int diff_merges_force_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+\tif (func == set_none)\n+\t\tforce_func = NULL;\n+\telse if (func != set_hide && func != set_no_hide)\n+\t\tforce_func = func;\n+\n+\treturn 0;\n+}\n+\n void diff_merges_suppress_m_parsing(void)\n {\n \tsuppress_m_parsing = 1;\n@@ -160,20 +176,18 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \tint argcount = 1;\n \tconst char *optarg;\n \tconst char *arg = argv[0];\n+\tdiff_merges_setup_func_t set_func = NULL;\n \n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n \t\tset_hide(revs);\n \t\trevs->merges_imply_patch = m_imply_p;\n \t} else if (!strcmp(arg, \"-c\")) {\n-\t\tset_combined(revs);\n-\t\trevs->merges_imply_patch = 1;\n+\t\tset_func = set_combined;\n \t} else if (!strcmp(arg, \"--cc\")) {\n-\t\tset_dense_combined(revs);\n-\t\trevs->merges_imply_patch = 1;\n+\t\tset_func = set_dense_combined;\n \t} else if (!strcmp(arg, \"--remerge-diff\")) {\n-\t\tset_remerge_diff(revs);\n-\t\trevs->merges_imply_patch = 1;\n+\t\tset_func = set_remerge_diff;\n \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n \t\tset_none(revs);\n \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\n@@ -183,6 +197,12 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t} else\n \t\treturn 0;\n \n+\tif (set_func != NULL) {\n+\t\t(force_func ? force_func : set_func)(revs);\n+\t\tforce_func = NULL;\n+\t\trevs->merges_imply_patch = 1;\n+\t}\n+\n \trevs->explicit_diff_merges = 1;\n \treturn argcount;\n }\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 9f0b3901fe82..6ef0cc87bb2a 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -15,6 +15,8 @@ int diff_merges_hide_config(int hide);\n \n int diff_merges_m_imply_p_config(int on);\n \n+int diff_merges_force_config(const char *value);\n+\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 1789dd6063c5..8a90d2dac360 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git config log.diffMergesForce has proper effect' '\n+\tgit log -m -p master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMergesForce on &&\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git config log.diffMergesForce override by duplicate' '\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMergesForce on &&\n+\tgit log --cc --cc master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 26a7e4ff877c..411e08b2fa1b 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '\n \tlog.decorate Z\n \tlog.diffMerges Z\n \tlog.diffMergesHide Z\n+\tlog.diffMergesForce Z\n \tlog.diffMerges-m-imply-p Z\n \tEOF\n '\n@@ -2528,6 +2529,7 @@ test_expect_success 'git -c - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMergesForce=Z\n \tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n@@ -2552,6 +2554,7 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMergesForce=Z\n \tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468041","messageId":"20221127093721.31012-5-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"[PATCH 4/5] diff-merges: support list of values for --diff-merges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:20Z","receivedAt":"2022-11-27T09:37:45Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Support comma-separated list of options in --diff-merges=. This allows\nfor shorter:\n\n --diff-merges=on,hide\n\ninstead of:\n\n --diff-merges=on --diff-merges=hide\n\nSigned-off-by: Sergey Organov \n---\n Documentation/diff-options.txt | 5 +++--\n diff-merges.c | 22 ++++++++++++++++++----\n t/t4013-diff-various.sh | 8 ++++++++\n 3 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex e64066af8d96..46c98c87e24f 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -38,7 +38,8 @@ ifdef::git-log[]\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-\t`first-parent` is the default.\n+\t`first-parent` is the default. Comma-separated list of\n+\tsupported values is accepted as well.\n +\n --diff-merges=(off|none):::\n --no-diff-merges:::\n@@ -53,7 +54,7 @@ ifdef::git-log[]\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.\n+\t`-m` is a shortcut for '--diff-merges=on,hide'.\n \tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n \tactive.\n +\ndiff --git a/diff-merges.c b/diff-merges.c\nindex cedd7652bf42..ddf9a411c49c 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -1,6 +1,7 @@\n #include \"diff-merges.h\"\n \n #include \"revision.h\"\n+#include \"strbuf.h\"\n \n typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n static void set_separate(struct rev_info *revs);\n@@ -110,12 +111,25 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\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+\tchar const delim = ',';\n+\tstruct strbuf **opts = strbuf_split_str(optarg, delim, -1);\n+\tstruct strbuf **p;\n \n-\tif (!func)\n-\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--diff-merges\", optarg);\n+\tfor (p = opts; *p; p++) {\n+\t\tdiff_merges_setup_func_t func;\n+\t\tchar *opt = (*p)->buf;\n+\t\tint len = (*p)->len;\n \n-\tfunc(revs);\n+\t\tif (opt[len - 1] == delim)\n+\t\t\topt[len - 1] = '\\0';\n+\t\tfunc = func_by_opt(opt);\n+\t\tif (!func) {\n+\t\t\tstrbuf_list_free(opts);\n+\t\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--diff-merges\", opt);\n+\t\t}\n+\t\tfunc(revs);\n+\t}\n+\tstrbuf_list_free(opts);\n }\n \n /*\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 8a90d2dac360..2c68d06074ed 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -485,6 +485,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'log --diff-merges=,' '\n+\tgit log --diff-merges=1,hide master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --diff-merges=1 --diff-merges=hide master >result &&\n+\tprocess_diffs result >actual &&\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-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468042","messageId":"20221127093721.31012-6-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"[PATCH 5/5] diff-merges: issue warning on lone '-m' option","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-27T09:37:21Z","receivedAt":"2022-11-27T09:37:50Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Lone '-m' is in use by scripts/aliases that aim at enabling diff\noutput for merge commits, but only if '-p' is then specified as well.\n\nAs '-m' may now be configured to imply '-p', using\n'log.diffMerges-m-imply-p', issue warning and suggest to instead use\n\n --diff-merges=on,hide\n\nthat does not depend on user configuration.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex ddf9a411c49c..b3b3c9e44ba8 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -11,6 +11,7 @@ static diff_merges_setup_func_t force_func = NULL;\n static int suppress_m_parsing;\n static int hide = 0;\n static int m_imply_p = 0;\n+static int got_m = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -196,6 +197,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t\tset_to_default(revs);\n \t\tset_hide(revs);\n \t\trevs->merges_imply_patch = m_imply_p;\n+\t\tgot_m = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_func = set_combined;\n \t} else if (!strcmp(arg, \"--cc\")) {\n@@ -259,5 +261,7 @@ void diff_merges_setup_revs(struct rev_info *revs)\n \tif (revs->merges_imply_patch || revs->merges_need_diff) {\n \t\tif (!revs->diffopt.output_format)\n \t\t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\n-\t}\n+\t} else if (got_m)\n+\t\twarning(_(\"legacy use of lone '-m' detected: please use '--diff-merges=on,hide' instead, as '-m' may imply '-p'\"));\n+\n }\n-- \n2.37.3.526.g5f84746cb16b\n\n"},{"id":"468055","messageId":"xmqqedtn957j.fsf@gitster.g","threadId":"58858","inReplyTo":"20221127093721.31012-4-sorganov@gmail.com","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-28T02:35:12Z","receivedAt":"2022-11-28T02:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> +\tif (set_func != NULL) {\n\nPlease write it like so:\n\n\tif (set_func) {\n\nI am not reviewing any new feature topic during -rc period (yet),\nbut this triggered CI failure at the tip of 'seen', so.\n\n"},{"id":"468063","messageId":"xmqqfse37c0n.fsf@gitster.g","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-28T07:51:04Z","receivedAt":"2022-11-28T07:51:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> 2. log.diffMerges-m-imply-p\n>\n> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n> options do. Simply fixing this inconsistency by unconditional\n> modification of '-m' semantics appeared to be a bad idea, as it broke\n> some legacy scripts/aliases. This patch rather provides configuration\n> variable to tweak '-m' behavior accordingly.\n\nI do not know how this can be a good idea. For those users who set\nthe configuration variables, those scripts and aliases get broken\nanyway, don't they? IOW, I am not sure how this is better than the\n\"modification of '-m'\" that is \"a bad idea\". I would understand why\nit may be a safer and more sensible solution, if the proposed\napproach were to find an unused letter $X and to introduce \"-$X\"\nthat is the same as \"-m\" but implies \"-p\", though.\n\n> 3. log.diffMergesForce\n>\n> Force specific log format for -c, --cc, and --remerge-diff options\n> instead of their respective formats. The override is useful when some\n> external tool hard-codes diff for merges format option.\n\nNot convinced it is a good idea for the same reason as above (not\nconvinced it is a bad idea, either, though).\n\n> 4. Support list of values for --diff-merges\n>\n> This allows for shorter --diff-merges=on,hide forms.\n\nGood, probably.\n\n"},{"id":"468107","messageId":"877czfgmye.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"xmqqfse37c0n.fsf@gitster.g","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-28T14:42:17Z","receivedAt":"2022-11-28T14:43:01Z","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>> 2. log.diffMerges-m-imply-p\n>>\n>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n>> options do. Simply fixing this inconsistency by unconditional\n>> modification of '-m' semantics appeared to be a bad idea, as it broke\n>> some legacy scripts/aliases. This patch rather provides configuration\n>> variable to tweak '-m' behavior accordingly.\n>\n> I do not know how this can be a good idea. For those users who set\n> the configuration variables, those scripts and aliases get broken\n> anyway, don't they? IOW, I am not sure how this is better than the\n> \"modification of '-m'\" that is \"a bad idea\".\n\nThe history behind this is that before the patch #1 of these series\nthere was no way to get exact semantics of current '-m' option using new\n--diff-merges option, so there was no sensible way to \"fix\" these\nscripts/aliases. That was in fact the primary objection for the new '-m'\nsemantics.\n\nNow, when one stomps on such script/alias (by explicitly enabling the\nnew configuration option), they can fix it by replacing '-m' with\n'--diff-merges=on,hide', that, along with the last patch of these series\nthat produces warning when lone '-m' is detected, looks to me as a way\nto eventually get rid of the legacy and surprising '-m' semantics.\n\n> I would understand why it may be a safer and more sensible solution,\n> if the proposed approach were to find an unused letter $X and to\n> introduce \"-$X\" that is the same as \"-m\" but implies \"-p\", though.\n\nI've in fact considered this as well, and '-d' was free for such use\nlast time I've checked. It was very tempting to just add '-d' that\nalways shows diff to first parent for everything, be it merge or not,\nand call it a day, but it won't fix '-m', which inconsistent behavior\nstill surprises people, and besides started the whole --diff-merges\nbusiness in the first place.\n\n>\n>> 3. log.diffMergesForce\n>>\n>> Force specific log format for -c, --cc, and --remerge-diff options\n>> instead of their respective formats. The override is useful when some\n>> external tool hard-codes diff for merges format option.\n>\n> Not convinced it is a good idea for the same reason as above (not\n> convinced it is a bad idea, either, though).\n\nHere the intention was entirely different though. I'm using magit and it\ndoes hard-code --cc in one place that I'd like to be able to override.\nIf I got this problem, I figured chances are high somebody else will get\nit as well, and that's the rationale.\n\nThen I figure everybody here has own favorite format for merge commits,\nand this toy gives you exactly this: whatever option is used (once), you\nget what you prefer instead, or use an option second time to enforce it.\n\nFor stable scripting, we for rather long time now have --diff-merges=c,\nand --diff-merges=cc, as well as slightly more recent\n--diff-merges=remerge, that are not affected by the configuration in\nquestion.\n\nThat said, it looks useful to me, but I won't insist on the feature\nshould you guys object.\n\n>\n>> 4. Support list of values for --diff-merges\n>>\n>> This allows for shorter --diff-merges=on,hide forms.\n>\n> Good, probably.\n\nWas useless before 'hide' though, as the rest of options just override\neach other entirely.\n\nThanks,\n-- Sergey Organov\n"},{"id":"468109","messageId":"8735a3gmuq.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"xmqqedtn957j.fsf@gitster.g","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-28T14:44:29Z","receivedAt":"2022-11-28T14:45:31Z","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>> +\tif (set_func != NULL) {\n>\n> Please write it like so:\n>\n> \tif (set_func) {\n\nOK, will do.\n\n>\n> I am not reviewing any new feature topic during -rc period (yet),\n> but this triggered CI failure at the tip of 'seen', so.\n\nThanks! Do we now have tool for auto-check for these issues? I still use\none from Linux kernel, and it didn't object to this form.\n\n-- Sergey Organov\n"},{"id":"468155","messageId":"CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-29T04:50:45Z","receivedAt":"2022-11-29T04:51:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n>\n> 1. --diff-merges=[no]-hide\n\nThis seems problematic to me. Currently, all the options to\ndiff-merges are exclusive of each other; the user is picking one of\nthem to determine how to format diffs for merges. Now you are\nintroducing the ability to combine various options, leading users to\nthink that perhaps they can run with all three of\n`--diff-merges=combined-dense --diff-merges=remerge\n--diff-merges=separate` or other nonsensical combinations. Shouldn't\nthis [no-]hide stuff be a separate flag rather than reusing\n--diff-merges?\n\n> The set of diff-merges options happened to be incomplete, and failed\n> to implement exact semantics of -m option that hides output of diffs\n> for merge commits unless -p option is active as well.\n>\n> The new \"hide\" option fixes this issue, so that now\n>\n> --diff-merges=on --diff-merges=hide\n>\n> combo is the exact synonym for -m.\n\nWhy is completeness important here? Perhaps I should state this\nanother way: when would users ever want to use this new \"hide\" option?\n I got through your cover letter not knowing the answer to this, but\nwas hoping it'd at least be covered in one of your commit messages or\ndocumentation changes. Maybe it was there, but I somehow missed it.\n\nIs the only goal some sense of developer completeness for these\noptions, or are these end-user-facing options of utility to actual end\nusers? I'm hoping the latter, but if so, can that be documented and\nexplained somewhere? I'm pretty sure this is explained somewhere in\nan old mailing list discussion, but where?\n\n> The log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\n> values, and governs the default value for the hide bit. The\n> configuration behaves as if \"--diff-merges=[no-]hide\" is inserted\n> first in the command-line.\n>\n> 2. log.diffMerges-m-imply-p\n>\n> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n> options do. Simply fixing this inconsistency by unconditional\n> modification of '-m' semantics appeared to be a bad idea, as it broke\n> some legacy scripts/aliases. This patch rather provides configuration\n> variable to tweak '-m' behavior accordingly.\n\n> 3. log.diffMergesForce\n>\n> Force specific log format for -c, --cc, and --remerge-diff options\n> instead of their respective formats. The override is useful when some\n> external tool hard-codes diff for merges format option.\n\nWhy just these three options and not -m (or --diff-merges=separate)?\n\nAlso, I read this and didn't quite fully grasp the intent; your\nexplanation in response to Junio seemed much more enlightening.\nPerhaps the wording/explanation could be cleaned up a bit? I'll\ncomment more on that specific patch...\n\n> 4. Support list of values for --diff-merges\n>\n> This allows for shorter --diff-merges=on,hide forms.\n\nAnd thus making users think they can pass\n--diff-merges=combined-dense,remerge,separate and suspecting that\nit'll do something useful? Seems like this is reinforcing a mistake\nto me.\n\n\n> 5. Issue warning for lone '-m'.\n>\n> Lone '-m' is in use by scripts/aliases that aim at enabling diff\n> output for merge commits, but only if '-p' is then specified as well.\n>\n> As '-m' may now be configured to imply '-p' (using\n> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,\n> and suggest to instead use '--diff-merges=on,hide' that does not\n> depend on user configuration.\n>\n> This is expected to give a provision for enabling\n> log.diffMerges-m-imply-p by default in the future.\n>\n> Sergey Organov (5):\n> diff-merges: implement [no-]hide option and log.diffMergesHide config\n> diff-merges: implement log.diffMerges-m-imply-p config\n> diff-merges: implement log.diffMergesForce config\n> diff-merges: support list of values for --diff-merges\n> diff-merges: issue warning on lone '-m' option\n>\n> Documentation/config/log.txt | 20 ++++\n> Documentation/diff-options.txt | 20 +++-\n> builtin/log.c | 6 +\n> diff-merges.c | 108 +++++++++++++++---\n> diff-merges.h | 6 +\n> t/t4013-diff-various.sh | 89 ++++++++++++++-\n> ...ges=first-parent_--diff-merges=hide_master | 34 ++++++\n> t/t9902-completion.sh | 9 ++\n> 8 files changed, 272 insertions(+), 20 deletions(-)\n> create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n>\n> --\n> 2.37.3.526.g5f84746cb16b\n>\n"},{"id":"468156","messageId":"CABPp-BFHQ8KwNK=FKGc96iQYqr9xT--WH7kg5R-CzCaAiWiRZg@mail.gmail.com","threadId":"58858","inReplyTo":"20221127093721.31012-4-sorganov@gmail.com","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-29T05:10:23Z","receivedAt":"2022-11-29T05:10:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n>\n> Force specified log format for -c, --cc, and --remerge-diff options\n> instead of their respective formats. The override is useful when some\n> external tool hard-codes diff for merges format option.\n>\n> Using any of the above options twice or more will get back the\n> original meaning of the option no matter what configuration says.\n>\n> Signed-off-by: Sergey Organov \n> ---\n> Documentation/config/log.txt | 11 +++++++++++\n> builtin/log.c | 2 ++\n> diff-merges.c | 32 ++++++++++++++++++++++++++------\n> diff-merges.h | 2 ++\n> t/t4013-diff-various.sh | 18 ++++++++++++++++++\n> t/t9902-completion.sh | 3 +++\n> 6 files changed, 62 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\n> index 265a57312e58..7452c7fad638 100644\n> --- a/Documentation/config/log.txt\n> +++ b/Documentation/config/log.txt\n> @@ -43,6 +43,17 @@ log.diffMergesHide::\n> log.diffMerges-m-imply-p::\n> `true` enables implication of `-p` by `-m`.\n>\n> +log.diffMergesForce::\n> + Use specified log format for -c, --cc, and --remerge-diff\n> + options instead of their respective formats when the option\n> + appears on the command line one time. See `--diff-merges` in\n> + linkgit:git-log[1] for allowed values. Using 'off' or 'none'\n> + disables the override (default).\n> ++\n> +The override is useful when external tool hard-codes one of the above\n> +options. Using any of these options two (or more) times will get back\n> +the original meaning of the options.\n\nI didn't quite understand your intent here from this explanation.\nWhen you pointed out to Junio that you wanted to override magit's\nhard-coded `git log --cc` and turn it into `git log -m -p`, then it\nsuddenly made more sense. And the two or more times I guess is your\nescape hatch to allow users to say \"I *really* do want this other\nformat, so `git log --cc --cc` will get it for me.\".\n\nMaybe something like:\n\nOverride -c, --cc, --remerge-diff options and use the specified\ndiff-generation scheme for merges instead. However, this config\nsetting can in turn be overridden by specifying an alternate option\nmultiple times (e.g. `git log --cc --cc`). Overriding the\ndiff-generation scheme for merges can be useful when an external tool\nhas a hard-coded command line it calls such as `git log --cc`. See\n`--diff-merges` in linkgit:git-log[1] for allowed values. Using 'off'\nor 'none' disables the override (default).\n\nHowever:\n * This feels like we're trying to workaround bugs or inflexibility\nin other tools with code in Git. This feels like a slippery slope\nissue and/or fixing the wrong tool.\n * Why is this just for -c, --cc, and --remerge-diff, and not for\nalso overriding -m? It seems odd that one would be left out,\nespecially since tools are more likely to have hard-coded it than\n--remerge-diff, given that -m has been around for a long time and\n--remerge-diff is new.\n\n> +\n> log.follow::\n> If `true`, `git log` will act as if the `--follow` option was used when\n> a single is given. This has the same limitations as `--follow`,\n[...]\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 1789dd6063c5..8a90d2dac360 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n> test_cmp expected actual\n> '\n>\n> +test_expect_success 'git config log.diffMergesForce has proper effect' '\n> + git log -m -p master >result &&\n> + process_diffs result >expected &&\n> + test_config log.diffMergesForce on &&\n\nI think the default for `on` is bad; it made sense at the time, but I\nthink we have a better option now. Perhaps we switch to it, perhaps\nwe don't, but if there's _any_ chance at all we change the default for\n\"on\" (which I think there definitely is), then you should really use\nthe option that matches the actual mode you are using rather than a\nsynonym for it; doing so future-proofs this testcase.\n\n> + git log --cc master >result &&\n> + process_diffs result >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'git config log.diffMergesForce override by duplicate' '\n> + git log --cc master >result &&\n> + process_diffs result >expected &&\n> + test_config log.diffMergesForce on &&\n\nMatters less here, but just in case \"--cc\" were to become the default,\nit'd be nice to explicitly use something else like separate here.\n\n> + git log --cc --cc master >result &&\n> + process_diffs result >actual &&\n> + test_cmp expected actual\n> +'\n"},{"id":"468193","messageId":"xmqqbkop3hjb.fsf@gitster.g","threadId":"58858","inReplyTo":"8735a3gmuq.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-29T15:30:00Z","receivedAt":"2022-11-29T15:30:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Junio C Hamano writes:\n>\n>> Sergey Organov writes:\n>>\n>>> +\tif (set_func != NULL) {\n>>\n>> Please write it like so:\n>>\n>> \tif (set_func) {\n>\n> OK, will do.\n>\n>>\n>> I am not reviewing any new feature topic during -rc period (yet),\n>> but this triggered CI failure at the tip of 'seen', so.\n>\n> Thanks! Do we now have tool for auto-check for these issues? I still use\n> one from Linux kernel, and it didn't object to this form.\n\nI noticed it when I pushed to GitHub, which ran the CI ;-)\n\nIf you have your own fork at GitHub of https://github.com/git/git/, I\nthink preparing a pull request against it triggers the CI.\n"},{"id":"468198","messageId":"221129.86tu2hiqre.gmgdl@evledraar.gmail.com","threadId":"58858","inReplyTo":"xmqqbkop3hjb.fsf@gitster.g","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-29T17:59:37Z","receivedAt":"2022-11-29T18:01:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 30 2022, Junio C Hamano wrote:\n\n> Sergey Organov writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> Sergey Organov writes:\n>>>\n>>>> +\tif (set_func != NULL) {\n>>>\n>>> Please write it like so:\n>>>\n>>> \tif (set_func) {\n>>\n>> OK, will do.\n>>\n>>>\n>>> I am not reviewing any new feature topic during -rc period (yet),\n>>> but this triggered CI failure at the tip of 'seen', so.\n>>\n>> Thanks! Do we now have tool for auto-check for these issues? I still use\n>> one from Linux kernel, and it didn't object to this form.\n>\n> I noticed it when I pushed to GitHub, which ran the CI ;-)\n>\n> If you have your own fork at GitHub of https://github.com/git/git/, I\n> think preparing a pull request against it triggers the CI.\n\n...in this case though there's no reason to wait for the glacially slow\nGitHub CI. You just need spatch installed and:\n\n\tmake coccicheck\n\nSergey: If you've hacked on the (I'm assuming linux) kernel you likely\nhave that installed already, they use it too (being the canonical and I\nbelieve original use-case for it).\n"},{"id":"468200","messageId":"Y4ZUAsT7UKguvXiz@coredump.intra.peff.net","threadId":"58858","inReplyTo":"8735a3gmuq.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-29T18:48:34Z","receivedAt":"2022-11-29T18:48:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote:\n\n> >> +\tif (set_func != NULL) {\n> >\n> > Please write it like so:\n> >\n> > \tif (set_func) {\n>[...]\n> Thanks! Do we now have tool for auto-check for these issues? I still use\n> one from Linux kernel, and it didn't object to this form.\n\nRunning \"make coccicheck\" will find this, but of course you need to have\ncoccinelle installed. Note that if it finds anything, \"make\" won't\nreport an error. You have to check for non-empty files in\ncontrib/coccinelle/*.patch.\n\n-Peff\n"},{"id":"468246","messageId":"87zgc8tx8p.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"CABPp-BFHQ8KwNK=FKGc96iQYqr9xT--WH7kg5R-CzCaAiWiRZg@mail.gmail.com","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-30T12:58:30Z","receivedAt":"2022-11-30T12:58:39Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n>>\n>> Force specified log format for -c, --cc, and --remerge-diff options\n>> instead of their respective formats. The override is useful when some\n>> external tool hard-codes diff for merges format option.\n>>\n>> Using any of the above options twice or more will get back the\n>> original meaning of the option no matter what configuration says.\n>>\n>> Signed-off-by: Sergey Organov \n>> ---\n>> Documentation/config/log.txt | 11 +++++++++++\n>> builtin/log.c | 2 ++\n>> diff-merges.c | 32 ++++++++++++++++++++++++++------\n>> diff-merges.h | 2 ++\n>> t/t4013-diff-various.sh | 18 ++++++++++++++++++\n>> t/t9902-completion.sh | 3 +++\n>> 6 files changed, 62 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\n>> index 265a57312e58..7452c7fad638 100644\n>> --- a/Documentation/config/log.txt\n>> +++ b/Documentation/config/log.txt\n>> @@ -43,6 +43,17 @@ log.diffMergesHide::\n>> log.diffMerges-m-imply-p::\n>> `true` enables implication of `-p` by `-m`.\n>>\n>> +log.diffMergesForce::\n>> + Use specified log format for -c, --cc, and --remerge-diff\n>> + options instead of their respective formats when the option\n>> + appears on the command line one time. See `--diff-merges` in\n>> + linkgit:git-log[1] for allowed values. Using 'off' or 'none'\n>> + disables the override (default).\n>> ++\n>> +The override is useful when external tool hard-codes one of the above\n>> +options. Using any of these options two (or more) times will get back\n>> +the original meaning of the options.\n>\n> I didn't quite understand your intent here from this explanation.\n> When you pointed out to Junio that you wanted to override magit's\n> hard-coded `git log --cc` and turn it into `git log -m -p`, then it\n> suddenly made more sense. And the two or more times I guess is your\n> escape hatch to allow users to say \"I *really* do want this other\n> format, so `git log --cc --cc` will get it for me.\".\n>\n> Maybe something like:\n>\n> Override -c, --cc, --remerge-diff options and use the specified\n> diff-generation scheme for merges instead. However, this config\n> setting can in turn be overridden by specifying an alternate option\n> multiple times (e.g. `git log --cc --cc`). Overriding the\n> diff-generation scheme for merges can be useful when an external tool\n> has a hard-coded command line it calls such as `git log --cc`. See\n> `--diff-merges` in linkgit:git-log[1] for allowed values. Using 'off'\n> or 'none' disables the override (default).\n\nThanks for suggestion, I'll take this into consideration should we agree\nto actually let this feature in.\n\n>\n> However:\n> * This feels like we're trying to workaround bugs or inflexibility\n> in other tools with code in Git. This feels like a slippery slope\n> issue and/or fixing the wrong tool.\n\nYep, that's why I said in my another answer to Junio that I won't insist\non it if you guys object, even though it does look useful for me.\n\n> * Why is this just for -c, --cc, and --remerge-diff, and not for\n> also overriding -m? It seems odd that one would be left out,\n> especially since tools are more likely to have hard-coded it than\n> --remerge-diff, given that -m has been around for a long time and\n> --remerge-diff is new.\n\n'-m' is rather the first one that got an override support, see\n'log.diffMerges'.\n\n[As for --remerge-diff, as a side note, I'd call it something like --rd\nfor short, as we have --diff-merges=remerge anyway. And then I'll think\nabout adding --pd (pure-diff) or --fpd (first-parent-diff) ;-)]\n\n>\n>> +\n>> log.follow::\n>> If `true`, `git log` will act as if the `--follow` option was used when\n>> a single is given. This has the same limitations as `--follow`,\n> [...]\n>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>> index 1789dd6063c5..8a90d2dac360 100755\n>> --- a/t/t4013-diff-various.sh\n>> +++ b/t/t4013-diff-various.sh\n>> @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n>> test_cmp expected actual\n>> '\n>>\n>> +test_expect_success 'git config log.diffMergesForce has proper effect' '\n>> + git log -m -p master >result &&\n>> + process_diffs result >expected &&\n>> + test_config log.diffMergesForce on &&\n>\n> I think the default for `on` is bad; it made sense at the time, but I\n> think we have a better option now.\n\nWe probably disagree about what a better option actually is, but the\npoint is valid anyway.\n\n> Perhaps we switch to it, perhaps we don't, but if there's _any_ chance\n> at all we change the default for \"on\" (which I think there definitely\n> is), then you should really use the option that matches the actual\n> mode you are using rather than a synonym for it; doing so\n> future-proofs this testcase.\n\nYep, agreed. Thanks for the catch!\n\n>\n>> + git log --cc master >result &&\n>> + process_diffs result >actual &&\n>> + test_cmp expected actual\n>> +'\n>> +\n>> +test_expect_success 'git config log.diffMergesForce override by duplicate' '\n>> + git log --cc master >result &&\n>> + process_diffs result >expected &&\n>> + test_config log.diffMergesForce on &&\n>\n> Matters less here, but just in case \"--cc\" were to become the default,\n> it'd be nice to explicitly use something else like separate here.\n\nYes, thanks!\n\n>\n>> + git log --cc --cc master >result &&\n>> + process_diffs result >actual &&\n>> + test_cmp expected actual\n>> +'\n\n-- Sergey Organov\n"},{"id":"468247","messageId":"87v8mwtx47.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"221129.86tu2hiqre.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-30T13:01:12Z","receivedAt":"2022-11-30T13:02:21Z","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 Wed, Nov 30 2022, Junio C Hamano wrote:\n>\n>> Sergey Organov writes:\n>>\n>>> Junio C Hamano writes:\n>>>\n>>>> Sergey Organov writes:\n>>>>\n>>>>> +\tif (set_func != NULL) {\n>>>>\n>>>> Please write it like so:\n>>>>\n>>>> \tif (set_func) {\n>>>\n>>> OK, will do.\n>>>\n>>>>\n>>>> I am not reviewing any new feature topic during -rc period (yet),\n>>>> but this triggered CI failure at the tip of 'seen', so.\n>>>\n>>> Thanks! Do we now have tool for auto-check for these issues? I still use\n>>> one from Linux kernel, and it didn't object to this form.\n>>\n>> I noticed it when I pushed to GitHub, which ran the CI ;-)\n>>\n>> If you have your own fork at GitHub of https://github.com/git/git/, I\n>> think preparing a pull request against it triggers the CI.\n>\n> ...in this case though there's no reason to wait for the glacially slow\n> GitHub CI. You just need spatch installed and:\n>\n> \tmake coccicheck\n>\n> Sergey: If you've hacked on the (I'm assuming linux) kernel you likely\n> have that installed already, they use it too (being the canonical and I\n> believe original use-case for it).\n\nThanks a lot! I'll definitely give it a try even though I didn't use it\nwhile hacking on the Linux kernel.\n\n-- Sergey Organov\n"},{"id":"468248","messageId":"87r0xktx2q.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"Y4ZUAsT7UKguvXiz@coredump.intra.peff.net","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-30T13:02:05Z","receivedAt":"2022-11-30T13:03:25Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote:\n>\n>> >> +\tif (set_func != NULL) {\n>> >\n>> > Please write it like so:\n>> >\n>> > \tif (set_func) {\n>>[...]\n>> Thanks! Do we now have tool for auto-check for these issues? I still use\n>> one from Linux kernel, and it didn't object to this form.\n>\n> Running \"make coccicheck\" will find this, but of course you need to have\n> coccinelle installed. Note that if it finds anything, \"make\" won't\n> report an error. You have to check for non-empty files in\n> contrib/coccinelle/*.patch.\n\nThank you! I'll give it a try.\n\n-- Sergey Organov\n"},{"id":"468249","messageId":"87lenstwfh.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-11-30T13:16:02Z","receivedAt":"2022-11-30T13:16:09Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n>>\n>> 1. --diff-merges=[no]-hide\n>\n> This seems problematic to me. Currently, all the options to\n> diff-merges are exclusive of each other; the user is picking one of\n> them to determine how to format diffs for merges. Now you are\n> introducing the ability to combine various options, leading users to\n> think that perhaps they can run with all three of\n> `--diff-merges=combined-dense --diff-merges=remerge\n> --diff-merges=separate` or other nonsensical combinations. Shouldn't\n> this [no-]hide stuff be a separate flag rather than reusing\n> --diff-merges?\n\nYes, it's a precedent indeed, but I don't see any actual problem here.\nUnlike git silently dropping changes on rebase, this can cause no\ndamage. I think I can emphasize that we now have \"formats\" and \"flags\"\nin the documentation, where \"formats\" are mutually exclusive (the latest\nspecified wins), while \"flags\" are cumulative.\n\n>\n>> The set of diff-merges options happened to be incomplete, and failed\n>> to implement exact semantics of -m option that hides output of diffs\n>> for merge commits unless -p option is active as well.\n>>\n>> The new \"hide\" option fixes this issue, so that now\n>>\n>> --diff-merges=on --diff-merges=hide\n>>\n>> combo is the exact synonym for -m.\n>\n> Why is completeness important here? Perhaps I should state this\n> another way: when would users ever want to use this new \"hide\" option?\n> I got through your cover letter not knowing the answer to this, but\n> was hoping it'd at least be covered in one of your commit messages or\n> documentation changes. Maybe it was there, but I somehow missed it.\n>\n> Is the only goal some sense of developer completeness for these\n> options, or are these end-user-facing options of utility to actual end\n> users? I'm hoping the latter, but if so, can that be documented and\n> explained somewhere? I'm pretty sure this is explained somewhere in\n> an old mailing list discussion, but where?\n\nCompleteness is essential as I want '--diff-merges' to provide all the\nneeded capabilities, and one of them was actually missing, that is there\nin the '-m' semantics, exactly as I said in the descriptions.\n\n>\n>> The log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\n>> values, and governs the default value for the hide bit. The\n>> configuration behaves as if \"--diff-merges=[no-]hide\" is inserted\n>> first in the command-line.\n>>\n>> 2. log.diffMerges-m-imply-p\n>>\n>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n>> options do. Simply fixing this inconsistency by unconditional\n>> modification of '-m' semantics appeared to be a bad idea, as it broke\n>> some legacy scripts/aliases. This patch rather provides configuration\n>> variable to tweak '-m' behavior accordingly.\n>\n>> 3. log.diffMergesForce\n>>\n>> Force specific log format for -c, --cc, and --remerge-diff options\n>> instead of their respective formats. The override is useful when some\n>> external tool hard-codes diff for merges format option.\n>\n> Why just these three options and not -m (or --diff-merges=separate)?\n\nAs I said in my answer to your other mail, '-m' is already configurable,\nso it is not needed to be included.\n\nNone of --diff-merges= options are affected by diffMergesForce, only 3\nspecific options from the documentation.\n\n>\n> Also, I read this and didn't quite fully grasp the intent; your\n> explanation in response to Junio seemed much more enlightening.\n> Perhaps the wording/explanation could be cleaned up a bit? I'll\n> comment more on that specific patch...\n\nYeah, thanks, I got your suggestion you put in another mail.\n\n>\n>> 4. Support list of values for --diff-merges\n>>\n>> This allows for shorter --diff-merges=on,hide forms.\n>\n> And thus making users think they can pass\n> --diff-merges=combined-dense,remerge,separate and suspecting that\n> it'll do something useful? Seems like this is reinforcing a mistake\n> to me.\n\nYes, they can. For now it's useful only for 'hide', but we might add\nmore flags in the future. It's also harmless, so I don't see it as a\nserious issue.\n\nThanks,\n-- Sergey Organov\n\n"},{"id":"468250","messageId":"xmqqtu2gvaf9.fsf@gitster.g","threadId":"58858","inReplyTo":"221129.86tu2hiqre.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-30T13:28:26Z","receivedAt":"2022-11-30T13:28:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> If you have your own fork at GitHub of https://github.com/git/git/, I\n>> think preparing a pull request against it triggers the CI.\n>\n> ...in this case though there's no reason to wait for the glacially slow\n> GitHub CI. You just need spatch installed and:\n>\n> \tmake coccicheck\n\nAnd you have to wait for the glacially slow coccicheck run locally,\nthough ;-).\n"},{"id":"468300","messageId":"CABPp-BEY-Xma8=MbXpw+En7TayzhYOmYYAHD6hudOQv=GnOT=Q@mail.gmail.com","threadId":"58858","inReplyTo":"87lenstwfh.fsf@osv.gnss.ru","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-12-01T02:21:00Z","receivedAt":"2022-12-01T02:23:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov wrote:\n>\n> Elijah Newren writes:\n>\n> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n> >>\n> >> 1. --diff-merges=[no]-hide\n> >\n> > This seems problematic to me. Currently, all the options to\n> > diff-merges are exclusive of each other; the user is picking one of\n> > them to determine how to format diffs for merges. Now you are\n> > introducing the ability to combine various options, leading users to\n> > think that perhaps they can run with all three of\n> > `--diff-merges=combined-dense --diff-merges=remerge\n> > --diff-merges=separate` or other nonsensical combinations. Shouldn't\n> > this [no-]hide stuff be a separate flag rather than reusing\n> > --diff-merges?\n>\n> Yes, it's a precedent indeed, but I don't see any actual problem here.\n> Unlike git silently dropping changes on rebase, this can cause no\n> damage.\n\nSure, read-only options for querying things won't cause future damage.\nThat doesn't mean that the UI for commands like diff/log/grep/etc are\nunimportant, though, and certainly doesn't excuse intentionally\ncreating bad UI for them.\n\n> I think I can emphasize that we now have \"formats\" and \"flags\"\n> in the documentation, where \"formats\" are mutually exclusive (the latest\n> specified wins), while \"flags\" are cumulative.\n\nWhy not just give it a different flag name, so that \"formats\" and\n\"flags\" are clearly separated without even needing a lengthy\nexplanation? That'd be much simpler to understand and explain.\n\n> >> The set of diff-merges options happened to be incomplete, and failed\n> >> to implement exact semantics of -m option that hides output of diffs\n> >> for merge commits unless -p option is active as well.\n> >>\n> >> The new \"hide\" option fixes this issue, so that now\n> >>\n> >> --diff-merges=on --diff-merges=hide\n> >>\n> >> combo is the exact synonym for -m.\n> >\n> > Why is completeness important here? Perhaps I should state this\n> > another way: when would users ever want to use this new \"hide\" option?\n> > I got through your cover letter not knowing the answer to this, but\n> > was hoping it'd at least be covered in one of your commit messages or\n> > documentation changes. Maybe it was there, but I somehow missed it.\n> >\n> > Is the only goal some sense of developer completeness for these\n> > options, or are these end-user-facing options of utility to actual end\n> > users? I'm hoping the latter, but if so, can that be documented and\n> > explained somewhere? I'm pretty sure this is explained somewhere in\n> > an old mailing list discussion, but where?\n>\n> Completeness is essential as I want '--diff-merges' to provide all the\n> needed capabilities, and one of them was actually missing, that is there\n> in the '-m' semantics, exactly as I said in the descriptions.\n\nI ask you why a user would want to use this option, and you simply\nassert that it's a \"needed capabilit[y]\"? Could you explain *why*\nit's needed or helpful for users instead of just repeating the\nassertion that it is needed?\n\nIf I can't figure out why it's needed or useful for users despite\nhaving read your cover letter, commit messages, underlying source code\nand documentation, and this full thread, then there may well be\nsomething wrong with me...but it seems likely that many users will\nalso have difficulty figuring out why this option is useful.\n"},{"id":"468309","messageId":"875yevh3dk.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"CABPp-BEY-Xma8=MbXpw+En7TayzhYOmYYAHD6hudOQv=GnOT=Q@mail.gmail.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-01T09:36:39Z","receivedAt":"2022-12-01T09:36:49Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov wrote:\n>>\n>> Elijah Newren writes:\n>>\n>> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov wrote:\n>> >>\n>> >> 1. --diff-merges=[no]-hide\n>> >\n>> > This seems problematic to me. Currently, all the options to\n>> > diff-merges are exclusive of each other; the user is picking one of\n>> > them to determine how to format diffs for merges. Now you are\n>> > introducing the ability to combine various options, leading users to\n>> > think that perhaps they can run with all three of\n>> > `--diff-merges=combined-dense --diff-merges=remerge\n>> > --diff-merges=separate` or other nonsensical combinations. Shouldn't\n>> > this [no-]hide stuff be a separate flag rather than reusing\n>> > --diff-merges?\n>>\n>> Yes, it's a precedent indeed, but I don't see any actual problem here.\n>> Unlike git silently dropping changes on rebase, this can cause no\n>> damage.\n>\n> Sure, read-only options for querying things won't cause future damage.\n> That doesn't mean that the UI for commands like diff/log/grep/etc are\n> unimportant, though, and certainly doesn't excuse intentionally\n> creating bad UI for them.\n\nI just don't see it as a bad UI, sorry.\n\n>\n>> I think I can emphasize that we now have \"formats\" and \"flags\"\n>> in the documentation, where \"formats\" are mutually exclusive (the latest\n>> specified wins), while \"flags\" are cumulative.\n>\n> Why not just give it a different flag name, so that \"formats\" and\n> \"flags\" are clearly separated without even needing a lengthy\n> explanation? That'd be much simpler to understand and explain.\n\nWhat I did is the best I was able to think of. The --diff-merges\nintroduces namespace for everything related to formats of output of\ndiffs for merge commits, and 'hide' fits there perfectly. User doesn't\nneed to look elsewhere to figure entire set of capabilities.\n\nI honestly don't see how to improve the UI by introducing yet another\noption, especially provided the letter has its own drawbacks.\n\n>\n>> >> The set of diff-merges options happened to be incomplete, and failed\n>> >> to implement exact semantics of -m option that hides output of diffs\n>> >> for merge commits unless -p option is active as well.\n>> >>\n>> >> The new \"hide\" option fixes this issue, so that now\n>> >>\n>> >> --diff-merges=on --diff-merges=hide\n>> >>\n>> >> combo is the exact synonym for -m.\n>> >\n>> > Why is completeness important here? Perhaps I should state this\n>> > another way: when would users ever want to use this new \"hide\" option?\n>> > I got through your cover letter not knowing the answer to this, but\n>> > was hoping it'd at least be covered in one of your commit messages or\n>> > documentation changes. Maybe it was there, but I somehow missed it.\n>> >\n>> > Is the only goal some sense of developer completeness for these\n>> > options, or are these end-user-facing options of utility to actual end\n>> > users? I'm hoping the latter, but if so, can that be documented and\n>> > explained somewhere? I'm pretty sure this is explained somewhere in\n>> > an old mailing list discussion, but where?\n>>\n>> Completeness is essential as I want '--diff-merges' to provide all the\n>> needed capabilities, and one of them was actually missing, that is there\n>> in the '-m' semantics, exactly as I said in the descriptions.\n>\n> I ask you why a user would want to use this option, and you simply\n> assert that it's a \"needed capabilit[y]\"? Could you explain *why*\n> it's needed or helpful for users instead of just repeating the\n> assertion that it is needed?\n\nI'm trying to explain that the use-case(s) is(are) at least the same as\nfor existing '-m' option, and '-m' is used in practice, so it must be\nuseful, right? Who am I to judge? So I don't.\n\nFor me one of the goals is to let people replace '-m' in scripts/aliases\nwith '--diff-merges=on,hide' and eventually let '-m' behave better as a\nshort user option, similar to '-c/--cc/--remrege-diff'. And then it\nmight happen that 'hide' is actually useful elsewhere (see below for a\ntry), as it often happens once particular functionality is properly\nfactored out of given use-case.\n\n> If I can't figure out why it's needed or useful for users despite\n> having read your cover letter, commit messages, underlying source code\n> and documentation, and this full thread, then there may well be\n> something wrong with me...but it seems likely that many users will\n> also have difficulty figuring out why this option is useful.\n\nThen they are still free not to use it, and I doubt they will try to\nfind why it's useful in the commit messages anyway, so do we need to put\nsomething into the documentation then? What do you suggest in addition\nto the functional explanation of the 'hide' that is already there?\n\nThat said, what comes to mind, as a use-case, I figure you might try to\ndefine an alias for 'diff log' that will use 'remerge' format by default\nonce diffs are requested using '-p':\n\n$ git config alias.lr 'log --diff-merges=remerge,hide'\n\nand check if this 'git lr' is useful.\n\nThanks,\n-- Sergey Organov\n"},{"id":"468753","messageId":"kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58858","inReplyTo":"20221127093721.31012-1-sorganov@gmail.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-12-07T23:55:21Z","receivedAt":"2022-12-07T23:55:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"We covered this series in Review Club, thanks for coming Sergey! For\nthose who are interested, the notes are here:\n\n https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing\n\nand reviewers will send feedback to the list separately anyway. I mostly\nhad comments on the design, so I'll leave most of my comments here.\n\nCommenting on the cover letter as a whole, on first read, it wasn't\nobvious to me what this series was trying to achieve because the CL\npresents the 5 patches individually instead of a cohesive story. From my\nunderstanding, the story is as follows: we want '-m' (enable\ndiff-merges) to also imply '-p', but we can't just change the default\nbehavior, so we do the following instead:\n\n- Add a config option that gives the behavior that we want (2/5).\n- Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and\n encouraging users to use that instead (1,4,5/5).\n\nPatch 3/5 is completely separate. There was some resistance to it during\nReview Club, but if we still want this, it might be worth splitting off\ninto its own series so that we can keep the discussions separate.\n\nDuring the discussion, it also appeared that this \"modification of '-m'\nsemantics\" refers to a patch that changed the default but got reverted\ndue to breaking legacy scripts. It would be extremely useful to include\na link to that previous patch and the discussion around its revert,\nespecially given the discussion about whether users actually need\n'-diff-merges=hide' ([1] and elsewhere).\n\n[1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/\n\nSergey Organov writes:\n\n> 1. --diff-merges=[no]-hide\n>\n> The set of diff-merges options happened to be incomplete, and failed\n> to implement exact semantics of -m option that hides output of diffs\n> for merge commits unless -p option is active as well.\n>\n> The new \"hide\" option fixes this issue, so that now\n>\n> --diff-merges=on --diff-merges=hide\n>\n> combo is the exact synonym for -m.\n>\n> The log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\n> values, and governs the default value for the hide bit. The\n> configuration behaves as if \"--diff-merges=[no-]hide\" is inserted\n> first in the command-line.\n\nI had the same concerns as Elijah, which is that this behavior is\nprobably clearer as a separate flag (like \"--hide-diff-merges\"), which\nis more consistent with how '--diff-options' is used today, which means\nthat:\n\na) it is easier to explain to users\nb) the implementation is simpler (I'll comment on Patch 1 code\n separately)\nc) it makes Patch 4 obsolete\n\nBut I'm not convinced that we actually want this behavior at all. I\ndon't see why a user would use a flag that says \"do nothing unless\nother flags are given\". I don't find the 'alias use case' compelling,\nbecause the user still has to choose whether to pass '-p', so at that\npoint they could just add a different alias.\n\nI haven't dug through the code/ML to figure out whether '-m' requiring\n'-p' was an intentional feature or not, but if you could find the old\nthread where you changed the default (and it got reverted), that would\nhelp the discussion a lot :)\n\n> 2. log.diffMerges-m-imply-p\n>\n> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n> options do. Simply fixing this inconsistency by unconditional\n> modification of '-m' semantics appeared to be a bad idea, as it broke\n> some legacy scripts/aliases. This patch rather provides configuration\n> variable to tweak '-m' behavior accordingly.\n\nI thought that Junio's suggestion to implement a flag that acts like\n'-m' with '-p' [2] was quite a good one (maybe '-M' or\n'--diff-merges=show'), since I think that very few users would actually\nset this config, but the ones that would actually use it can just\nreplace '-m' with '-M'.\n\n[2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/\n\n> 3. log.diffMergesForce\n>\n> Force specific log format for -c, --cc, and --remerge-diff options\n> instead of their respective formats. The override is useful when some\n> external tool hard-codes diff for merges format option.\n\nThis might be better off as its own series, since the change isn't\nrelated to '-m', but I'm worried about the precedent that this sets.\nTo my knowledge, CLI options always overwrite config, but this is the\nopposite. I would prefer not to do this, especially if the use case is\nto work around an external tool (since it is arguably the tool that is\nbroken).\n\n> 5. Issue warning for lone '-m'.\n>\n> Lone '-m' is in use by scripts/aliases that aim at enabling diff\n> output for merge commits, but only if '-p' is then specified as well.\n>\n> As '-m' may now be configured to imply '-p' (using\n> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,\n> and suggest to instead use '--diff-merges=on,hide' that does not\n> depend on user configuration.\n>\n> This is expected to give a provision for enabling\n> log.diffMerges-m-imply-p by default in the future.\n\nSince '-m' without '-p' is a mistake in most cases, I wonder if we\nshould just emit this warning today (maybe via the advise() API). Even\nif we don't keep '--diff-options=hide', deprecating lone '-m' and giving\na warning seems good to keep.\n"},{"id":"468758","messageId":"kl6lfsdqep25.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58858","inReplyTo":"20221127093721.31012-2-sorganov@gmail.com","subject":"Re: [PATCH 1/5] diff-merges: implement [no-]hide option and log.diffMergesHide config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-12-08T00:06:58Z","receivedAt":"2022-12-08T00:07:04Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n> @@ -49,10 +49,11 @@ ifdef::git-log[]\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> +\tthe default format. The default format could be changed using\n> \t`log.diffMerges` configuration parameter, which default value\n> \tis `separate`.\n> ++\n> +\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n> +\n\nI found this description difficult to parse, since\n\na) it wasn't clear that multiple \"--diff-merges\" would all be respected\nb) I had to read the --diff-merges=hide documentation to understand what\n this means\n\nKeeping the plain english description would help, something like:\n\n `-m` only produces the output if `-p` is also given, i.e. it is a\n shortcut for '--diff-merges=on --diff-merges=hide'.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 56e2d95e869d..e031021e53b2 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -581,6 +581,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> \t}\n> \tif (!strcmp(var, \"log.diffmerges\"))\n> \t\treturn diff_merges_config(value);\n> +\tif (!strcmp(var, \"log.diffmergeshide\"))\n> +\t\treturn diff_merges_hide_config(git_config_bool(var, value));\n> \tif (!strcmp(var, \"log.showroot\")) {\n> \t\tdefault_show_root = git_config_bool(var, value);\n> \t\treturn 0;\n\nSince we have log.diffmergeshide that is different from log.diffmerges,\nit seems like it would be more consistent to have '--diff-merges-hide'\nas a separate flag.\n\n> @@ -69,6 +87,10 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n> {\n> \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n> \t\treturn set_none;\n> +\tif (!strcmp(optarg, \"hide\"))\n> +\t\treturn set_hide;\n> +\tif (!strcmp(optarg, \"no-hide\"))\n> +\t\treturn set_no_hide;\n> \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n> \t\treturn set_first_parent;\n> \tif (!strcmp(optarg, \"separate\"))\n> @@ -105,7 +127,19 @@ int diff_merges_config(const char *value)\n> \tif (!func)\n> \t\treturn -1;\n> \n> -\tset_to_default = func;\n> +\tif (func == set_hide)\n> +\t\thide = 1;\n> +\telse if (func == set_no_hide)\n> +\t\thide = 0;\n> +\telse\n> +\t\tset_to_default = func;\n> +\n> +\treturn 0;\n> +}\n\nThe code is also simpler if we took a separate CLI flag, e.g. we could\nget rid of this special casing of \"(func == X)\".\n"},{"id":"468785","messageId":"87tu26arzy.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-08T14:29:21Z","receivedAt":"2022-12-08T14:29:28Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> We covered this series in Review Club, thanks for coming Sergey! For\n> those who are interested, the notes are here:\n\nThank you all guys for the review and for valuable discussion!\n\n[...]\n\n> During the discussion, it also appeared that this \"modification of '-m'\n> semantics\" refers to a patch that changed the default but got reverted\n> due to breaking legacy scripts. It would be extremely useful to include\n> a link to that previous patch and the discussion around its revert,\n> especially given the discussion about whether users actually need\n> '-diff-merges=hide' ([1] and elsewhere).\n\nThe last time '-m' issue appeared on the list, it all started here:\n\nhttps://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/\n\nIn particular, the final patch and its revert is deeper down this tread:\n\nhttps://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t\n\nand\n\nhttps://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n\nWhere do you prefer these references to be put, in the cover letter, in\nthe commit message, or in both places?\n\n-- Sergey Organov\n"},{"id":"468792","messageId":"874ju5c1iz.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-08T16:18:12Z","receivedAt":"2022-12-08T16:18:24Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> We covered this series in Review Club, thanks for coming Sergey! For\n> those who are interested, the notes are here:\n>\n> https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing\n>\n> and reviewers will send feedback to the list separately anyway. I mostly\n> had comments on the design, so I'll leave most of my comments here.\n>\n> Commenting on the cover letter as a whole, on first read, it wasn't\n> obvious to me what this series was trying to achieve because the CL\n> presents the 5 patches individually instead of a cohesive story. From my\n> understanding, the story is as follows: we want '-m' (enable\n> diff-merges) to also imply '-p', but we can't just change the default\n> behavior, so we do the following instead:\n>\n> - Add a config option that gives the behavior that we want (2/5).\n> - Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and\n> encouraging users to use that instead (1,4,5/5).\n\nWell, very close, but not exactly. I'd rather say:\n\n1. Provide exact semantics of '-m' trough --diff-merges UI by\n introducing 'hide' option, after which '--diff-merges=on,hide' and\n '-m' have exactly the same behavior.\n\n2. Add a config option that gives the new behavior we want to '-m': to\n rather be a synonym for '--diff-merges=on[,hide] -p\". (Please notice\n that 'hide' is not needed here as it's immediately canceled by '-p'.)\n\nSo, essentially, after (1) is there, the config option turns '-m'\nmeaning from default '--diff-merges=on,hide' to desired\n'--diff-merges=on -p'.\n\nPlease also notice that at this point we may instead decide to just switch\n'-m' meaning to new semantics, either without config at all, or with\nconfig that'd rather restore previous semantics. In fact, the primary\nreason why previously such patch has been reverted was absence of (1),\nand so with (2) maybe I was just overly cautious.\n\n> Patch 3/5 is completely separate. There was some resistance to it during\n> Review Club, but if we still want this, it might be worth splitting off\n> into its own series so that we can keep the discussions separate.\n\nOK, I'll cut it off for now.\n\n>\n> During the discussion, it also appeared that this \"modification of '-m'\n> semantics\" refers to a patch that changed the default but got reverted\n> due to breaking legacy scripts. It would be extremely useful to include\n> a link to that previous patch and the discussion around its revert,\n> especially given the discussion about whether users actually need\n> '-diff-merges=hide' ([1] and elsewhere).\n\nYep, please see references I've sent in my previous answer to this\nmessage.\n\n>\n> [1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/\n>\n> Sergey Organov writes:\n>\n>> 1. --diff-merges=[no]-hide\n>>\n>> The set of diff-merges options happened to be incomplete, and failed\n>> to implement exact semantics of -m option that hides output of diffs\n>> for merge commits unless -p option is active as well.\n>>\n>> The new \"hide\" option fixes this issue, so that now\n>>\n>> --diff-merges=on --diff-merges=hide\n>>\n>> combo is the exact synonym for -m.\n>>\n>> The log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\n>> values, and governs the default value for the hide bit. The\n>> configuration behaves as if \"--diff-merges=[no-]hide\" is inserted\n>> first in the command-line.\n>\n> I had the same concerns as Elijah, which is that this behavior is\n> probably clearer as a separate flag (like \"--hide-diff-merges\"), which\n> is more consistent with how '--diff-options' is used today, which means\n> that:\n>\n> a) it is easier to explain to users\n> b) the implementation is simpler (I'll comment on Patch 1 code\n> separately)\n> c) it makes Patch 4 obsolete\n\nI'd postpone implementation/design discussion till we get to agreement\nof the need for this option in the first place.\n\n> But I'm not convinced that we actually want this behavior at all. I\n> don't see why a user would use a flag that says \"do nothing unless\n> other flags are given\". don't find the 'alias use case' compelling,\n> because the user still has to choose whether to pass '-p', so at that\n> point they could just add a different alias.\n\nIf one travels back the history, they will find that originally all -m,\n-c, and --cc were behaving exactly this way: \"do nothing, unless diffs\nare actually requested\", i.e. they specified only diff format to be used\nonce requested, and did not request the output themselves. I prefer to\nstay on the safe side, and assume that such behavior is still useful,\neven though -c/--cc turned to imply '-p' eventually, as doing the same\nto '-m' caused so much desire and resistance simultaneously.\n\nOTOH, --diff-merges= does two things: specifies the format and\nrequests the output for merge commits. It could have been design\nmistake, even though it has been discussed at the time of introduction,\nbut now the only way to get another behavior is to turn off one of the\nactions they do: turn off requesting the actual output for merge\ncommits, and that's what proposed 'hide' means.\n\n> I haven't dug through the code/ML to figure out whether '-m' requiring\n> '-p' was an intentional feature or not, but if you could find the old\n> thread where you changed the default (and it got reverted), that would\n> help the discussion a lot :)\n\nYep, I gave the links in my first answer to this message.\n\n>\n>> 2. log.diffMerges-m-imply-p\n>>\n>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\n>> options do. Simply fixing this inconsistency by unconditional\n>> modification of '-m' semantics appeared to be a bad idea, as it broke\n>> some legacy scripts/aliases. This patch rather provides configuration\n>> variable to tweak '-m' behavior accordingly.\n>\n> I thought that Junio's suggestion to implement a flag that acts like\n> '-m' with '-p' [2] was quite a good one (maybe '-M' or\n> '--diff-merges=show'), since I think that very few users would actually\n> set this config, but the ones that would actually use it can just\n> replace '-m' with '-M'.\n\nIntroducing new short convenience option(s) is out of scope of these\nseries, and suggested --diff-merges=show, as I see it, is essentially\n\"--diff-merges=on -p\" that I find hard to explain inside the\n'--diff-merges=' context which name suggests it affects merge commits\nonly.\n\nThat said, saying: \"we have slightly broken '-m' that we refrain to fix,\nso let's introduce '-M' instead of fixing '-m'\" does not sound very\nconvincing either.\n\n>\n> [2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/\n>\n>> 3. log.diffMergesForce\n>>\n>> Force specific log format for -c, --cc, and --remerge-diff options\n>> instead of their respective formats. The override is useful when some\n>> external tool hard-codes diff for merges format option.\n>\n> This might be better off as its own series, since the change isn't\n> related to '-m',\n\nYep, I'm sorry to mix it into the series, -- the only excuse is that it\nlooked very relevant to me when I did it :)\n\n> but I'm worried about the precedent that this sets.\n> To my knowledge, CLI options always overwrite config, but this is the\n> opposite. I would prefer not to do this, especially if the use case is\n> to work around an external tool (since it is arguably the tool that is\n> broken).\n\nThe tool was only an initial motivation for the patch. From following a\nfew discussions on the list I got feeling that every person has their\nown preference for --diff-merges format, and rarely wants to see\nanything else. This config essentially gives them a way to say \"please\nuse my preferred format everywhere, unless I explicitly say otherwise\",\nin a centralized manner.\n\n>\n>> 5. Issue warning for lone '-m'.\n>>\n>> Lone '-m' is in use by scripts/aliases that aim at enabling diff\n>> output for merge commits, but only if '-p' is then specified as well.\n>>\n>> As '-m' may now be configured to imply '-p' (using\n>> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,\n>> and suggest to instead use '--diff-merges=on,hide' that does not\n>> depend on user configuration.\n>>\n>> This is expected to give a provision for enabling\n>> log.diffMerges-m-imply-p by default in the future.\n>\n> Since '-m' without '-p' is a mistake in most cases, I wonder if we\n> should just emit this warning today (maybe via the advise() API). Even\n> if we don't keep '--diff-options=hide', deprecating lone '-m' and giving\n> a warning seems good to keep.\n\nI'd tend to rather not.\n\nActually, as far as I'm aware, the only actual use that has been\ndetected was \"--fist-parent -m\", and that use case was exactly what you\nguys don't find useful: specify default format for merge commits. In\nthis particular case it is the diff to the first parent, and dates back\nto the days before --diff-merges, when using history traversal option\n--first-parent was the only way to get diffs with respect to the first\nparent for merges.\n\nMaybe we should instead flag the \"--first-parent -m\" as a warning, as\nproducing a warning for lone \"-m\" without these patches is effectively\ngetting users out of using lone \"-m\" instead of fixing the latter.\n\nI rather see the bright future as using \"-m\" all the time, as it's now\nextremely configurable.\n\nThanks,\n-- \nSergey Organov\n"},{"id":"468795","messageId":"87zgbxahm5.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"kl6lfsdqep25.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 1/5] diff-merges: implement [no-]hide option and log.diffMergesHide config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-08T18:13:38Z","receivedAt":"2022-12-08T18:13:51Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Sergey Organov writes:\n>\n>> @@ -49,10 +49,11 @@ ifdef::git-log[]\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>> +\tthe default format. The default format could be changed using\n>> \t`log.diffMerges` configuration parameter, which default value\n>> \tis `separate`.\n>> ++\n>> +\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n>> +\n>\n> I found this description difficult to parse, since\n>\n> a) it wasn't clear that multiple \"--diff-merges\" would all be respected\n> b) I had to read the --diff-merges=hide documentation to understand what\n> this means\n>\n> Keeping the plain english description would help, something like:\n>\n> `-m` only produces the output if `-p` is also given, i.e. it is a\n> shortcut for '--diff-merges=on --diff-merges=hide'.\n\nThanks, I'll review the documentation if we decide all this stuff is\nuseful.\n\n>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 56e2d95e869d..e031021e53b2 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -581,6 +581,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n>> \t}\n>> \tif (!strcmp(var, \"log.diffmerges\"))\n>> \t\treturn diff_merges_config(value);\n>> +\tif (!strcmp(var, \"log.diffmergeshide\"))\n>> +\t\treturn diff_merges_hide_config(git_config_bool(var, value));\n>> \tif (!strcmp(var, \"log.showroot\")) {\n>> \t\tdefault_show_root = git_config_bool(var, value);\n>> \t\treturn 0;\n>\n> Since we have log.diffmergeshide that is different from log.diffmerges,\n> it seems like it would be more consistent to have '--diff-merges-hide'\n> as a separate flag.\n\nI'd rather drop log.diffmergeshide, as log.diffMerges=hide does the same\nthing. I'm just not sure if multiple instances of the same\nlog.diffMerges config are simple enough to manage through \"git config\"\ninterface.\n\nThis is independent of --diff-merges-hide suggestion, that, if\nimplemented, I'd prefer to read as --diff-merges-flags=[no-]hide, to\nprovide space for future flags, even though I still prefer a syntax\ninside existing \"--diff-merges\" namespace. Something like:\n\n --diff-merges=[,[,...]]\n\n: on|off|c|cc|remerge|...\n: [no-]hide|...\n\n>\n>> @@ -69,6 +87,10 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n>> {\n>> \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n>> \t\treturn set_none;\n>> +\tif (!strcmp(optarg, \"hide\"))\n>> +\t\treturn set_hide;\n>> +\tif (!strcmp(optarg, \"no-hide\"))\n>> +\t\treturn set_no_hide;\n>> \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n>> \t\treturn set_first_parent;\n>> \tif (!strcmp(optarg, \"separate\"))\n>> @@ -105,7 +127,19 @@ int diff_merges_config(const char *value)\n>> \tif (!func)\n>> \t\treturn -1;\n>> \n>> -\tset_to_default = func;\n>> +\tif (func == set_hide)\n>> +\t\thide = 1;\n>> +\telse if (func == set_no_hide)\n>> +\t\thide = 0;\n>> +\telse\n>> +\t\tset_to_default = func;\n>> +\n>> +\treturn 0;\n>> +}\n>\n> The code is also simpler if we took a separate CLI flag, e.g. we could\n> get rid of this special casing of \"(func == X)\".\n\nI foresee complications elsewhere. Overall complexity won't be that\ndifferent either way, I think.\n\nThanks,\n-- \nSergey Organov\n"},{"id":"468804","messageId":"kl6lcz8tebtk.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58858","inReplyTo":"87tu26arzy.fsf@osv.gnss.ru","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-12-08T23:05:11Z","receivedAt":"2022-12-08T23:05:26Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n> The last time '-m' issue appeared on the list, it all started here:\n>\n> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/\n>\n> In particular, the final patch and its revert is deeper down this tread:\n>\n> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t\n>\n> and\n>\n> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n\nThanks, these provide extremely helpful context :) In particular:\n\n- Junio describes this \"do nothing unless -p\" is given behavior as an\n accident [1].\n- Jonathan Nieder notes that this change accidentally broke scripts\n where \"-m\" probably wasn't doing anything useful, but we wanted to\n avoid breaking the scripts for backwards compatibility anyway [2].\n\nI got the sense that the closest thing to an intentional use case of\n\"-m\" is for users who thought that \"-m\" would affect path limiting [3],\nalthough it doesn't actually do that. So what I've reads so far suggests\nthat \"do nothing unless -p\" (aka --diff-merges=hide) is not actually\nuseful, and we should just drop it.\n\nWe could keep the warning for \"-m\" without \"-p\" (Patch 5), and recommend\n\"--diff-merges=(on|m)\".\n\n[1] https://lore.kernel.org/git/xmqqwnsl93m3.fsf@gitster.g/\n[2] https://lore.kernel.org/git/YQtYEftByY8cNMml@google.com/\n[3] https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n>\n> Where do you prefer these references to be put, in the cover letter, in\n> the commit message, or in both places?\n>\n> -- Sergey Organov\n"},{"id":"468805","messageId":"kl6la63xebqz.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58858","inReplyTo":"87tu26arzy.fsf@osv.gnss.ru","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-12-08T23:06:44Z","receivedAt":"2022-12-08T23:06:49Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Sergey Organov writes:\n\n> Where do you prefer these references to be put, in the cover letter, in\n> the commit message, or in both places?\n\nWherever it might be relevant as motivation behind the change, so\nprobably both, but especially the cover letter :)\n\n>\n> -- Sergey Organov\n"},{"id":"468858","messageId":"87ilijt2c3.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"kl6lcz8tebtk.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 0/5] diff-merges: more features","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-10T20:45:32Z","receivedAt":"2022-12-10T20:45:40Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Glen Choo writes:\n\n> Sergey Organov writes:\n>\n>> The last time '-m' issue appeared on the list, it all started here:\n>>\n>> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/\n>>\n>> In particular, the final patch and its revert is deeper down this tread:\n>>\n>> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t\n>>\n>> and\n>>\n>> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n>\n> Thanks, these provide extremely helpful context :) In particular:\n>\n> - Junio describes this \"do nothing unless -p\" is given behavior as an\n> accident [1].\n\nI rather read it as Junio saying that \"-m does not imply -p\" is\nhistorical accident, and yes, it is, provided '-c/--cc' were fixed at\nsome point, and '-m' was not, so in fact I figure Junio meant: \"-m\ndiffers from -c/--cc in not implying '-p'\" is historical accident. And\nthen he suggests to leave it as is, with which I disagree.\n\nIn addition, in all the discussions, I believe Junio at least once said\nhe does see valid usages for current hush '-m':\n\n\nBut \"stash list\" example shows that \"log --first-parent -m\" without\n\"-p\" in a script has a valid reason, and a change that hurts those\nwho correctly used a command and an option in a way they were\nintended to do _is_ problematic.\n\n\nhere:\n\nhttps://lore.kernel.org/git/xmqqy29chim6.fsf@gitster.g/\n\n> - Jonathan Nieder notes that this change accidentally broke scripts\n> where \"-m\" probably wasn't doing anything useful, but we wanted to\n> avoid breaking the scripts for backwards compatibility anyway [2].\n>\n> I got the sense that the closest thing to an intentional use case of\n> \"-m\" is for users who thought that \"-m\" would affect path limiting [3],\n> although it doesn't actually do that.\n\nI don't think so. Dunno why you got such feeling. It's rather that for\nsome time \"--first-parent -m\" was the only way to produce *most* useful\nform of \"-m\" format: show diff with respect to the *first* parent only,\nwhereas without \"--first-parent\" \"-m\" produced diff output for *every*\nparent in turn(!) giving extremely confusing result. Please notice how\n--first-parent appears in most of those discussions.\n\nOverall, the (simplified) history of '-m' goes like this, as far as I\ncan tell:\n\n0. Original '-m', documented only for 'diff-tree'. The diffs were\n produced to all the parents that was probably very logical for\n plumbing 'diff-tree', as it reflects symmetric nature of merge\n commits in the DAG, that is the core of git data model.\n\n However, while \"git log -p\" does not produce patches for merge\n commits (apparently to get rid of often large output), '-m' in fact\n enforces the output for merge commits, in the format produced by\n 'diff-tree -m', i.e., diffs to all the parents in turn.\n\n [The latter was probably the first mistake, it should have rather\n produced the diff with respect to the first parent that is more\n suitable for \"git log\" being porcelain, to show changes introduced by\n the commit to the mainline, exactly as for non-merge commits]\n\n1. '-c' is introduced, then '--cc' is introduced, with semantics similar\n to '-m' with respect to '-p', but different kinds of output.\n\n At this point we have consistent behavior of '-m', '-c', and '--cc'\n with respect to '-p', none of which produce any output unless '-p' is\n specified as well.\n\n2. '-c' and '--cc' are changed to imply '-p'[0]. '-m' is left alone,\n supposedly forgotten as being undocumented for \"git log\" and of\n limited use, due to its large and surprising output.\n\n [I think that was the second mistake, forgetting to change '-m'\n accordingly]\n\n3. '-m' is changed to produce diff with respect to the first parent only\n when '--first-parent' is specified [1]. '-m' finally starts to\n (sometimes) give useful output, and starts to be actually used,\n but only together with '--first-parent' most of times.\n\n BTW, this is the first time '-m' has been documented as part of \"git\n log\": \"This patch properly documents the -m switch, which has so far\n been mentioned only as a fairly special diff-tree flag.\"\n\n [I think there are more mistakes here: not changing '-m' to imply\n '-p' at this point, and not producing single-parent diff even without\n '--first-parent']\n\n4. \"--first-parent\" is suggested to imply \"-m\"(!) to let \"--first-parent\n -p\" to produce diff for merge commits[2]. That in turn needed an\n option that will negate implied \"-m\", and that's where\n --[no-]diff-merges was suggested.\n\n Please notice that if '-m' implied '-p' (as it should) at this point,\n there should be little needed for these patches, as just saying \"git\n log --first-parent -m\" would produce required result. So, mistakes\n above caused a need to fix them.\n\n5. At this point, in the referenced discussion I suggested\n '--diff-merges=on|off' instead of '--[no-]diff-merges', to allow for\n further extensions.\n\n6. '--diff-merges=' option actually born to provide some missing\n functionality and to get rid of inconsistencies[3].\n\n7. '-m' format becomes configurable using new \"log.diffMerges\"\n configuration[4] so that we can make it conveniently useful even\n without \"--first-parent\". This was immediately implemented in\n generalized manner to allow to configure '-m' to produce not only\n single-parent diff, but any supported format.\n\n8. As a reply to yet another request on the mailing list \"why '-m'\n produces no output\", I tried \"-m imply -p\" patch series[5], which\n were accepted, but then the last patch only(!) from the series, that\n actually introduced required behavior, has been reverted.\n\n This left me feel I got some unfinished job to do.\n\n9. These patch series, trying \"-m imply -p\" again, now more carefully.\n\n> So what I've reads so far suggests that \"do nothing unless -p\" (aka\n> --diff-merges=hide) is not actually useful, and we should just drop\n> it.\n\nAgain, I've tried exactly this before, and that was first accepted, and\nthen reverted, that's why --diff-merges=hide has been introduced in\nthese series, to address the issues raised during revert request.\n\nReferences:\n\n[0]: Showing merges easier with \"git log\":\n\nhttps://lore.kernel.org/git/1440110591-12941-1-git-send-email-gitster@pobox.com/\n\n[1]: git log -p -m: Document, honor --first-parent\n\nhttps://lore.kernel.org/git/20100210011149.GR9553@machine.or.cz/\n\n[2]: making --first-parent imply -m:\n\nhttps://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/\n\n[3]: git-log: implement new --diff-merge options:\n\nhttps://lore.kernel.org/git/20201221152000.13134-1-sorganov@gmail.com/\n\n[4]: git log: configurable default format for merge diffs\n\nhttps://lore.kernel.org/git/20210413114118.25693-1-sorganov@gmail.com/\n\n[5]: diff-merges: let -m imply -p\n\nhttps://lore.kernel.org/git/20210520214703.27323-1-sorganov@gmail.com/\n\nThanks,\n-- Sergey Organov\n"},{"id":"469216","messageId":"20221217132955.108542-1-sorganov@gmail.com","threadId":"58858","inReplyTo":"kl6lilimepli.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"[PATCH v1 0/5] diff-merges: more features to fix '-m'","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:50Z","receivedAt":"2022-12-17T13:30:17Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"These patch series is yet another attempt to improve '-m' behavior by\nfirst improving --diff-merges to be able to provide current '-m' semantics,\nand then introducing a configuration variable to tweak '-m' behavior.\n\nBelow is some historic context of the issue, followed by more thorough\ndescription of the patch series.\n\nThe last attempt to fix '-m' option for \"git log\" to imply '-p', to\nmake it consistent with similar options (-c/--cc), was called by the\nrequest on the mailing list, here:\n\nhttps://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/\n\nHowever, the suggested (and accepted at first) patch series:\n\nhttps://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t\n\nappeared to have two problems:\n\n* --diff-merges options are incomplete and have no way to provide\n exact existing '-m' semantics.\n\n* implying '-p' by '-m' by default broke some legacy uses of\n \"git log --firt-parent -m\".\n\nDue to this, the last patch of those patch series has been later\nreverted:\n\nhttps://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n\neffectively leaving the issue unresolved.\n\nThese patch series in turn consists of the following patches:\n\n1. --diff-merges=[no]-hide\n\nThe set of diff-merges options happened to be incomplete, and failed\nto implement exact semantics of -m option that hides output of diffs\nfor merge commits unless -p option is active as well.\n\nThe new \"hide\" option fixes this issue, so that now\n\n --diff-merges=on --diff-merges=hide\n\ncombo is the exact synonym for -m.\n\nThe log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\nvalues, and governs the default value for the hide bit. The\nconfiguration behaves as if \"--diff-merges=[no-]hide\" is inserted\nfirst in the command-line.\n\nA new log.diffMergesHide configuration is introduced as well, to aid\nin avoiding of multiple log.diffMerges configuration entries when both\nformat and hide flag are to be configured.\n\n2. log.diffMerges-m-imply-p\n\nHistorically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'\noptions do. Simply fixing this inconsistency by unconditional\nmodification of '-m' semantics appeared to be a bad idea, as it broke\nsome legacy scripts/aliases. This patch rather provides configuration\nvariable to tweak '-m' behavior accordingly.\n\n3. Support list of values for --diff-merges\n\nThis allows for shorter --diff-merges=on,hide forms.\n\n4. Issue warning for lone '-m'.\n\nLone '-m' is in use by scripts/aliases that aim at enabling diff\noutput for merge commits, but only if '-p' is then specified as well.\n\nAs '-m' may now be configured to imply '-p' (using\n'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,\nand suggest to instead use '--diff-merges=on,hide' that does not\ndepend on user configuration.\n\nThis is expected to give a provision for enabling\nlog.diffMerges-m-imply-p by default in the future.\n\n5. Improve documentation for --diff-merges.\n\nUpdates in v1:\n\n * Added background context to the patch series foreword.\n\n * Documentation improvements added in a separate commit.\n\n * Remove commit unrelated to '-m' behavior.\n\n * Fix style: (p != NULL) -> (p).\n\n * Fix documentation placement of 'hide' option.\n\n * Fix log.diffMergesForce tests to avoid dependency on current\n defaults.\n\nSergey Organov (5):\n diff-merges: implement [no-]hide option and log.diffMergesHide config\n diff-merges: implement log.diffMerges-m-imply-p config\n diff-merges: support list of values for --diff-merges\n diff-merges: issue warning on lone '-m' option\n diff-merges: improve --diff-merges documentation\n\n Documentation/config/log.txt | 9 ++\n Documentation/diff-options.txt | 104 +++++++++++-------\n builtin/log.c | 4 +\n diff-merges.c | 76 +++++++++++--\n diff-merges.h | 4 +\n t/t4013-diff-various.sh | 71 +++++++++++-\n ...ges=first-parent_--diff-merges=hide_master | 34 ++++++\n t/t9902-completion.sh | 6 +\n 8 files changed, 257 insertions(+), 51 deletions(-)\n create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n\nInterdiff against v0:\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 7452c7fad638..265a57312e58 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -43,17 +43,6 @@ log.diffMergesHide::\n log.diffMerges-m-imply-p::\n \t`true` enables implication of `-p` by `-m`.\n \n-log.diffMergesForce::\n-\tUse specified log format for -c, --cc, and --remerge-diff\n-\toptions instead of their respective formats when the option\n-\tappears on the command line one time. See `--diff-merges` in\n-\tlinkgit:git-log[1] for allowed values. Using 'off' or 'none'\n-\tdisables the override (default).\n-+\n-The override is useful when external tool hard-codes one of the above\n-options. Using any of these options two (or more) times will get back\n-the original meaning of the options.\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/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 46c98c87e24f..a3fbdb85a8b8 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -34,76 +34,86 @@ endif::git-diff[]\n endif::git-format-patch[]\n \n ifdef::git-log[]\n---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::\n+-m::\n+\tShow diffs for merge commits in the default format.\n+\tShortcut for '--diff-merges=on,hide' unless\n+\t`log.diffMerges-m-imply-p` configuration is active, in which\n+\tcase it's a shortcut for '--diff-merges=on -p'.\n+\n+-c::\n+\tShortcut for '--diff-merges=combined -p'.\n+\n+--cc::\n+\tShortcut for '--diff-merges=dense-combined -p'.\n+\n+--remerge-diff::\n+\tShortcut for '--diff-merges=remerge -p'.\n+\n --no-diff-merges::\n-\tSpecify diff format to be used for merge commits. Default is\n+\tSynonym for '--diff-merges=off'.\n+\n+--diff-merges=(|)[,(|),...]::\n+\tSpecify diff format and flags to be used for merge commits. Default is\n \t{diff-merges-default} unless `--first-parent` is in use, in which case\n-\t`first-parent` is the default. Comma-separated list of\n-\tsupported values is accepted as well.\n+\t`first-parent` is the default.\n +\n---diff-merges=(off|none):::\n---no-diff-merges:::\n+The last format specified has precedence, whereas flags are\n+cumulative. Comma-separated list is handy to provide flag(s) along\n+with format, e.g.: `--diff-merges=first-parent,hide` is handy form of\n+`--diff-merges=first-parent --diff-merges=hide`.\n++\n+The following formats are supported:\n++\n+--\n+off, none::\n \tDisable output of diffs for merge commits. Useful to override\n \timplied value.\n +\n---diff-merges=on:::\n---diff-merges=m:::\n--m:::\n-\tThese options make diff output for merge commits to be shown in\n-\tthe default format. The default format could be changed using\n+on, m::\n+\tMake diff output for merge commits to be shown in the default\n+\tformat. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on,hide'.\n-\tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n-\tactive.\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-\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.\n-+\n---diff-merges=remerge:::\n---diff-merges=r:::\n---remerge-diff:::\n-\tWith this option, two-parent merge commits are remerged to\n-\tcreate a temporary tree object -- potentially containing files\n-\twith conflict markers and such. A diff is then shown between\n-\tthat temporary tree and the actual merge commit.\n-+\n---diff-merges=hide:::\n---diff-merges=no-hide:::\n-\tHide (do not hide) the diff for merge commits unless `-p` options is given\n-\tas well. The default `no-hide` could be changed using `log.diffMerges`\n-\tconfiguration parameter.\n+first-parent, 1::\n+\tShow full diff with respect to first parent. This is the same\n+\tformat as `--patch` produces for non-merge commits.\n++\n+separate::\n+\tShow full diff with respect to each of parents.\n+\tSeparate log entry and diff is generated for each parent.\n++\n+remerge, r::\n+\tRemerge two-parent merge commits to create a temporary tree\n+\tobject--potentially containing files with conflict markers\n+\tand such. A diff is then shown between that temporary tree\n+\tand the actual merge commit.\n +\n The output emitted when this option is used is subject to change, and\n so is its interaction with other options (unless explicitly\n documented).\n +\n---diff-merges=combined:::\n---diff-merges=c:::\n--c:::\n-\tWith this option, diff output for a merge commit shows the\n-\tdifferences from each of the parents to the merge result\n-\tsimultaneously instead of showing pairwise diff between a\n-\tparent and the result one at a time. Furthermore, it lists\n-\tonly files which were modified from all parents. `-c` implies\n-\t`-p`.\n+combined, c::\n+\tShow differences from each of the parents to the merge\n+\tresult simultaneously instead of showing pairwise diff between\n+\ta parent and the result one at a time. Furthermore, it lists\n+\tonly files which were modified from all parents.\n +\n---diff-merges=dense-combined:::\n---diff-merges=cc:::\n---cc:::\n-\tWith this option the output produced by\n-\t`--diff-merges=combined` is further compressed by omitting\n-\tuninteresting hunks whose contents in the parents have only\n-\ttwo variants and the merge result picks one of them without\n-\tmodification. `--cc` implies `-p`.\n+dense-combined, cc::\n+\tFurther compress output produced by `--diff-merges=combined`\n+\tby omitting uninteresting hunks whose contents in the parents\n+\thave only two variants and the merge result picks one of them\n+\twithout modification.\n+--\n++\n+The following flags are supported:\n++\n+--\n+[no-]hide::\n+\tHide diff for merge commits unless `-p` options is given as\n+\twell. The default `no-hide` could be changed using\n+\t`log.diffMerges` configuration parameter.\n+--\n \n --combined-all-paths::\n \tThis flag causes combined diffs (used for merge commits) to\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 1e8d0a2545a9..332b5e478cc5 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -585,8 +585,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn diff_merges_hide_config(git_config_bool(var, value));\n \tif (!strcmp(var, \"log.diffmerges-m-imply-p\"))\n \t\treturn diff_merges_m_imply_p_config(git_config_bool(var, value));\n-\tif (!strcmp(var, \"log.diffmergesforce\"))\n-\t\treturn diff_merges_force_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 b3b3c9e44ba8..1f4e43e16940 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -7,7 +7,6 @@ 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-static diff_merges_setup_func_t force_func = NULL;\n static int suppress_m_parsing;\n static int hide = 0;\n static int m_imply_p = 0;\n@@ -166,21 +165,6 @@ int diff_merges_m_imply_p_config(int on)\n \treturn 0;\n }\n \n-int diff_merges_force_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-\tif (func == set_none)\n-\t\tforce_func = NULL;\n-\telse if (func != set_hide && func != set_no_hide)\n-\t\tforce_func = func;\n-\n-\treturn 0;\n-}\n-\n void diff_merges_suppress_m_parsing(void)\n {\n \tsuppress_m_parsing = 1;\n@@ -191,7 +175,6 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \tint argcount = 1;\n \tconst char *optarg;\n \tconst char *arg = argv[0];\n-\tdiff_merges_setup_func_t set_func = NULL;\n \n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n@@ -199,11 +182,14 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t\trevs->merges_imply_patch = m_imply_p;\n \t\tgot_m = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n-\t\tset_func = set_combined;\n+\t\tset_combined(revs);\n+\t\trevs->merges_imply_patch = 1;\n \t} else if (!strcmp(arg, \"--cc\")) {\n-\t\tset_func = set_dense_combined;\n+\t\tset_dense_combined(revs);\n+\t\trevs->merges_imply_patch = 1;\n \t} else if (!strcmp(arg, \"--remerge-diff\")) {\n-\t\tset_func = set_remerge_diff;\n+\t\tset_remerge_diff(revs);\n+\t\trevs->merges_imply_patch = 1;\n \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n \t\tset_none(revs);\n \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\n@@ -213,12 +199,6 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t} else\n \t\treturn 0;\n \n-\tif (set_func != NULL) {\n-\t\t(force_func ? force_func : set_func)(revs);\n-\t\tforce_func = NULL;\n-\t\trevs->merges_imply_patch = 1;\n-\t}\n-\n \trevs->explicit_diff_merges = 1;\n \treturn argcount;\n }\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 6ef0cc87bb2a..9f0b3901fe82 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -15,8 +15,6 @@ int diff_merges_hide_config(int hide);\n \n int diff_merges_m_imply_p_config(int on);\n \n-int diff_merges_force_config(const char *value);\n-\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 2c68d06074ed..a07513e67fd6 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -565,24 +565,6 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'git config log.diffMergesForce has proper effect' '\n-\tgit log -m -p master >result &&\n-\tprocess_diffs result >expected &&\n-\ttest_config log.diffMergesForce on &&\n-\tgit log --cc master >result &&\n-\tprocess_diffs result >actual &&\n-\ttest_cmp expected actual\n-'\n-\n-test_expect_success 'git config log.diffMergesForce override by duplicate' '\n-\tgit log --cc master >result &&\n-\tprocess_diffs result >expected &&\n-\ttest_config log.diffMergesForce on &&\n-\tgit log --cc --cc master >result &&\n-\tprocess_diffs result >actual &&\n-\ttest_cmp expected actual\n-'\n-\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 411e08b2fa1b..26a7e4ff877c 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2498,7 +2498,6 @@ test_expect_success 'git config - variable name' '\n \tlog.decorate Z\n \tlog.diffMerges Z\n \tlog.diffMergesHide Z\n-\tlog.diffMergesForce Z\n \tlog.diffMerges-m-imply-p Z\n \tEOF\n '\n@@ -2529,7 +2528,6 @@ test_expect_success 'git -c - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n-\tlog.diffMergesForce=Z\n \tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n@@ -2554,7 +2552,6 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n-\tlog.diffMergesForce=Z\n \tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n-- \n2.25.1\n\n"},{"id":"469217","messageId":"20221217132955.108542-2-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"[PATCH v1 1/5] diff-merges: implement [no-]hide option and log.diffMergesHide config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:51Z","receivedAt":"2022-12-17T13:30:19Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"The set of diff-merges options happened to be incomplete, and failed\nto implement exact semantics of -m option that hides output of diffs\nfor merge commits unless -p option is active as well.\n\nThe new \"hide\" option fixes this issue, so that now\n\n --diff-merges=on --diff-merges=hide\n\ncombo is the exact synonym for -m.\n\nThe log.diffMerges configuration also accepts \"hide\" and \"no-hide\"\nvalues, and governs the default value for the hide bit. The\nconfiguration behaves as if \"--diff-merges=[no-]hide\" is inserted\nfirst in the command-line.\n\nNew log.diffMergesHide boolean configuration is implemented as well,\nfor the case when log.diffMerges is already in use for specifying the\nformat.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 6 +++\n Documentation/diff-options.txt | 13 +++--\n builtin/log.c | 2 +\n diff-merges.c | 40 ++++++++++++--\n diff-merges.h | 2 +\n t/t4013-diff-various.sh | 54 ++++++++++++++++++-\n ...ges=first-parent_--diff-merges=hide_master | 34 ++++++++++++\n t/t9902-completion.sh | 3 ++\n 8 files changed, 147 insertions(+), 7 deletions(-)\n create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex cbe34d759221..9c6be643bcf6 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -33,6 +33,12 @@ log.diffMerges::\n \tSet diff format to be used when `--diff-merges=on` is\n \tspecified, see `--diff-merges` in linkgit:git-log[1] for\n \tdetails. Defaults to `separate`.\n++\n+When used with 'hide' or 'no-hide', sets the default hiding of\n+diffs for merge commits when `-p` option is not used.\n+\n+log.diffMergesHide::\n+\t`true` implies `--diff-merges=hide` option.\n \n log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92c..b062bdab04d9 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|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::\n+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::\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@@ -49,10 +49,11 @@ ifdef::git-log[]\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+\tthe default format. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n++\n+\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n +\n --diff-merges=first-parent:::\n --diff-merges=1:::\n@@ -94,6 +95,12 @@ documented).\n \tuninteresting hunks whose contents in the parents have only\n \ttwo variants and the merge result picks one of them without\n \tmodification. `--cc` implies `-p`.\n++\n+--diff-merges=hide:::\n+--diff-merges=no-hide:::\n+\tHide (do not hide) the diff for merge commits unless `-p` options is given\n+\tas well. The default `no-hide` could be changed using `log.diffMerges`\n+\tconfiguration parameter.\n \n --combined-all-paths::\n \tThis flag causes combined diffs (used for merge commits) to\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 56e2d95e869d..e031021e53b2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -581,6 +581,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"log.diffmerges\"))\n \t\treturn diff_merges_config(value);\n+\tif (!strcmp(var, \"log.diffmergeshide\"))\n+\t\treturn diff_merges_hide_config(git_config_bool(var, 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 85cbefa5afd7..55fe5b70c102 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -7,6 +7,7 @@ static void set_separate(struct rev_info *revs);\n \n static diff_merges_setup_func_t set_to_default = set_separate;\n static int suppress_m_parsing;\n+static int hide = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -20,10 +21,15 @@ static void suppress(struct rev_info *revs)\n \trevs->remerge_diff = 0;\n }\n \n+static void set_need_diff(struct rev_info *revs)\n+{\n+\trevs->merges_need_diff = !hide;\n+}\n+\n static void common_setup(struct rev_info *revs)\n {\n \tsuppress(revs);\n-\trevs->merges_need_diff = 1;\n+\tset_need_diff(revs);\n }\n \n static void set_none(struct rev_info *revs)\n@@ -31,6 +37,18 @@ static void set_none(struct rev_info *revs)\n \tsuppress(revs);\n }\n \n+static void set_hide(struct rev_info *revs)\n+{\n+\thide = 1;\n+\tset_need_diff(revs);\n+}\n+\n+static void set_no_hide(struct rev_info *revs)\n+{\n+\thide = 0;\n+\tset_need_diff(revs);\n+}\n+\n static void set_separate(struct rev_info *revs)\n {\n \tcommon_setup(revs);\n@@ -69,6 +87,10 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n \t\treturn set_none;\n+\tif (!strcmp(optarg, \"hide\"))\n+\t\treturn set_hide;\n+\tif (!strcmp(optarg, \"no-hide\"))\n+\t\treturn set_no_hide;\n \tif (!strcmp(optarg, \"1\") || !strcmp(optarg, \"first-parent\"))\n \t\treturn set_first_parent;\n \tif (!strcmp(optarg, \"separate\"))\n@@ -105,7 +127,19 @@ int diff_merges_config(const char *value)\n \tif (!func)\n \t\treturn -1;\n \n-\tset_to_default = func;\n+\tif (func == set_hide)\n+\t\thide = 1;\n+\telse if (func == set_no_hide)\n+\t\thide = 0;\n+\telse\n+\t\tset_to_default = func;\n+\n+\treturn 0;\n+}\n+\n+int diff_merges_hide_config(int on)\n+{\n+\thide = on;\n \treturn 0;\n }\n \n@@ -122,7 +156,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n-\t\trevs->merges_need_diff = 0;\n+\t\tset_hide(revs);\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex 19639689bb05..e86e5381693b 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -11,6 +11,8 @@ struct rev_info;\n \n int diff_merges_config(const char *value);\n \n+int diff_merges_hide_config(int hide);\n+\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex dfcf3a0aaae3..4fc8ba2fc59c 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -334,6 +334,7 @@ log --first-parent --diff-merges=off -p master\n log -p --first-parent master\n log -p --diff-merges=first-parent master\n log --diff-merges=first-parent master\n+log --diff-merges=first-parent --diff-merges=hide master\n log -m -p --first-parent master\n log -m -p master\n log --cc -m -p master\n@@ -460,7 +461,18 @@ EOF\n test_expect_success 'log -m matches pure log' '\n \tgit log master >result &&\n \tprocess_diffs result >expected &&\n-\tgit log -m >result &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'log --diff-merges=on matches -m only with --diff-merges=hide' '\n+\tgit log -m master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >actual &&\n+\t! test_cmp expected actual &&\n+\tgit log --diff-merges=on --diff-merges=hide master >result &&\n \tprocess_diffs result >actual &&\n \ttest_cmp expected actual\n '\n@@ -496,6 +508,46 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git config log.diffMerges hide: has effect' '\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >no-hide &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >hide &&\n+\t! test_cmp no-hide hide\n+'\n+\n+test_expect_success 'git config log.diffMerges no-hide: is the default' '\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >default &&\n+\ttest_config log.diffMerges no-hide &&\n+\tgit log --diff-merges=on master >result &&\n+\tprocess_diffs result >no-hide &&\n+\ttest_cmp default no-hide\n+'\n+\n+# As \"-m\" is synonym for \"--diff-merges=hide --diff-merges=on\", the\n+# \"log.diffMerges=hide\" configuration should have no effect on \"-m\"\n+test_expect_success 'git config log.diffMerges hide: has no effect on -m' '\n+\tgit log -m master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# As \"--cc\" implies \"-p\", the \"log.diffMerges=hide\" configuration\n+# should have no effect on \"--cc\"\n+test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges hide &&\n+\tgit log --cc master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\nnew file mode 100644\nindex 000000000000..596caec64298\n--- /dev/null\n+++ b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master\n@@ -0,0 +1,34 @@\n+$ git log --diff-merges=first-parent --diff-merges=hide master\n+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64\n+Merge: 9a6d494 c7a2ab9\n+Author: A U Thor \n+Date: Mon Jun 26 00:04:00 2006 +0000\n+\n+ Merge branch 'side'\n+\n+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a\n+Author: A U Thor \n+Date: Mon Jun 26 00:03:00 2006 +0000\n+\n+ Side\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \n+Date: Mon Jun 26 00:02:00 2006 +0000\n+\n+ Third\n+\n+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44\n+Author: A U Thor \n+Date: Mon Jun 26 00:01:00 2006 +0000\n+\n+ Second\n+ \n+ This is the second commit.\n+\n+commit 444ac553ac7612cc88969031b02b3767fb8a353a\n+Author: A U Thor \n+Date: Mon Jun 26 00:00:00 2006 +0000\n+\n+ Initial\n+$\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 43de868b8005..fc6b0722216a 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2497,6 +2497,7 @@ test_expect_success 'git config - variable name' '\n \tlog.date Z\n \tlog.decorate Z\n \tlog.diffMerges Z\n+\tlog.diffMergesHide Z\n \tEOF\n '\n \n@@ -2525,6 +2526,7 @@ test_expect_success 'git -c - variable name' '\n \tlog.date=Z\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n+\tlog.diffMergesHide=Z\n \tEOF\n '\n \n@@ -2547,6 +2549,7 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.date=Z\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n+\tlog.diffMergesHide=Z\n \tEOF\n '\n \n-- \n2.25.1\n\n"},{"id":"469218","messageId":"20221217132955.108542-3-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"[PATCH v1 2/5] diff-merges: implement log.diffMerges-m-imply-p config","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:52Z","receivedAt":"2022-12-17T13:30:22Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Historically, `-m` doesn't imply `-p` whereas similar `-c` and `--cc`\noptions do. Simply fixing this inconsistency by unconditional\nmodification of `-m` semantics appeared to be a bad idea, as it broke\nsome legacy scripts.\n\nImplement \"log.diffMerges-m-imply-p\" boolean configuration variable\nthat allows user to enable implication of `-p` by `-m`.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/config/log.txt | 3 +++\n Documentation/diff-options.txt | 6 ++++--\n builtin/log.c | 2 ++\n diff-merges.c | 8 ++++++++\n diff-merges.h | 2 ++\n t/t4013-diff-various.sh | 9 +++++++++\n t/t9902-completion.sh | 3 +++\n 7 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 9c6be643bcf6..265a57312e58 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -40,6 +40,9 @@ diffs for merge commits when `-p` option is not used.\n log.diffMergesHide::\n \t`true` implies `--diff-merges=hide` option.\n \n+log.diffMerges-m-imply-p::\n+\t`true` enables implication of `-p` by `-m`.\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/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex b062bdab04d9..fe15693492a2 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -48,12 +48,14 @@ ifdef::git-log[]\n --diff-merges=on:::\n --diff-merges=m:::\n -m:::\n-\tThis option makes diff output for merge commits to be shown in\n+\tThese options make diff output for merge commits to be shown in\n \tthe default format. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'\n+\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.\n+\tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n+\tactive.\n +\n --diff-merges=first-parent:::\n --diff-merges=1:::\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e031021e53b2..332b5e478cc5 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -583,6 +583,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn diff_merges_config(value);\n \tif (!strcmp(var, \"log.diffmergeshide\"))\n \t\treturn diff_merges_hide_config(git_config_bool(var, value));\n+\tif (!strcmp(var, \"log.diffmerges-m-imply-p\"))\n+\t\treturn diff_merges_m_imply_p_config(git_config_bool(var, 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 55fe5b70c102..1fbf476d378e 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -8,6 +8,7 @@ static void set_separate(struct rev_info *revs);\n static diff_merges_setup_func_t set_to_default = set_separate;\n static int suppress_m_parsing;\n static int hide = 0;\n+static int m_imply_p = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -143,6 +144,12 @@ int diff_merges_hide_config(int on)\n \treturn 0;\n }\n \n+int diff_merges_m_imply_p_config(int on)\n+{\n+\tm_imply_p = on;\n+\treturn 0;\n+}\n+\n void diff_merges_suppress_m_parsing(void)\n {\n \tsuppress_m_parsing = 1;\n@@ -157,6 +164,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \tif (!suppress_m_parsing && !strcmp(arg, \"-m\")) {\n \t\tset_to_default(revs);\n \t\tset_hide(revs);\n+\t\trevs->merges_imply_patch = m_imply_p;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\ndiff --git a/diff-merges.h b/diff-merges.h\nindex e86e5381693b..9f0b3901fe82 100644\n--- a/diff-merges.h\n+++ b/diff-merges.h\n@@ -13,6 +13,8 @@ int diff_merges_config(const char *value);\n \n int diff_merges_hide_config(int hide);\n \n+int diff_merges_m_imply_p_config(int on);\n+\n void diff_merges_suppress_m_parsing(void);\n \n int diff_merges_parse_opts(struct rev_info *revs, const char **argv);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 4fc8ba2fc59c..1789dd6063c5 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -548,6 +548,15 @@ test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '\n+\tgit log -m -p master >result &&\n+\tprocess_diffs result >expected &&\n+\ttest_config log.diffMerges-m-imply-p true &&\n+\tgit log -m master >result &&\n+\tprocess_diffs result >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # -m in \"git diff-index\" means \"match missing\", that differs\n # from its meaning in \"git diff\". Let's check it in diff-index.\n # The line in the output for removed file should disappear when\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex fc6b0722216a..26a7e4ff877c 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '\n \tlog.decorate Z\n \tlog.diffMerges Z\n \tlog.diffMergesHide Z\n+\tlog.diffMerges-m-imply-p Z\n \tEOF\n '\n \n@@ -2527,6 +2528,7 @@ test_expect_success 'git -c - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n \n@@ -2550,6 +2552,7 @@ test_expect_success 'git clone --config= - variable name' '\n \tlog.decorate=Z\n \tlog.diffMerges=Z\n \tlog.diffMergesHide=Z\n+\tlog.diffMerges-m-imply-p=Z\n \tEOF\n '\n \n-- \n2.25.1\n\n"},{"id":"469219","messageId":"20221217132955.108542-4-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"[PATCH v1 3/5] diff-merges: support list of values for --diff-merges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:53Z","receivedAt":"2022-12-17T13:30:23Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Support comma-separated list of options in --diff-merges=. This allows\nfor shorter:\n\n --diff-merges=on,hide\n\ninstead of:\n\n --diff-merges=on --diff-merges=hide\n\nSigned-off-by: Sergey Organov \n---\n Documentation/diff-options.txt | 5 +++--\n diff-merges.c | 22 ++++++++++++++++++----\n t/t4013-diff-various.sh | 8 ++++++++\n 3 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex fe15693492a2..977f9135b0d6 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -38,7 +38,8 @@ ifdef::git-log[]\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-\t`first-parent` is the default.\n+\t`first-parent` is the default. Comma-separated list of\n+\tsupported values is accepted as well.\n +\n --diff-merges=(off|none):::\n --no-diff-merges:::\n@@ -53,7 +54,7 @@ ifdef::git-log[]\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.\n+\t`-m` is a shortcut for '--diff-merges=on,hide'.\n \tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n \tactive.\n +\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 1fbf476d378e..bb2797ff8cc5 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -1,6 +1,7 @@\n #include \"diff-merges.h\"\n \n #include \"revision.h\"\n+#include \"strbuf.h\"\n \n typedef void (*diff_merges_setup_func_t)(struct rev_info *);\n static void set_separate(struct rev_info *revs);\n@@ -109,12 +110,25 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\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+\tchar const delim = ',';\n+\tstruct strbuf **opts = strbuf_split_str(optarg, delim, -1);\n+\tstruct strbuf **p;\n \n-\tif (!func)\n-\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--diff-merges\", optarg);\n+\tfor (p = opts; *p; p++) {\n+\t\tdiff_merges_setup_func_t func;\n+\t\tchar *opt = (*p)->buf;\n+\t\tint len = (*p)->len;\n \n-\tfunc(revs);\n+\t\tif (opt[len - 1] == delim)\n+\t\t\topt[len - 1] = '\\0';\n+\t\tfunc = func_by_opt(opt);\n+\t\tif (!func) {\n+\t\t\tstrbuf_list_free(opts);\n+\t\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--diff-merges\", opt);\n+\t\t}\n+\t\tfunc(revs);\n+\t}\n+\tstrbuf_list_free(opts);\n }\n \n /*\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 1789dd6063c5..a07513e67fd6 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -485,6 +485,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'log --diff-merges=,' '\n+\tgit log --diff-merges=1,hide master >result &&\n+\tprocess_diffs result >expected &&\n+\tgit log --diff-merges=1 --diff-merges=hide master >result &&\n+\tprocess_diffs result >actual &&\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-- \n2.25.1\n\n"},{"id":"469220","messageId":"20221217132955.108542-5-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"[PATCH v1 4/5] diff-merges: issue warning on lone '-m' option","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:54Z","receivedAt":"2022-12-17T13:30:26Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Lone '-m' is in use by scripts/aliases that aim at enabling diff\noutput for merge commits, but only if '-p' is then specified as well.\n\nAs '-m' may now be configured to imply '-p', using\n'log.diffMerges-m-imply-p', issue warning and suggest to instead use\n\n --diff-merges=on,hide\n\nthat does not depend on user configuration.\n\nSigned-off-by: Sergey Organov \n---\n diff-merges.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex bb2797ff8cc5..1f4e43e16940 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -10,6 +10,7 @@ static diff_merges_setup_func_t set_to_default = set_separate;\n static int suppress_m_parsing;\n static int hide = 0;\n static int m_imply_p = 0;\n+static int got_m = 0;\n \n static void suppress(struct rev_info *revs)\n {\n@@ -179,6 +180,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t\tset_to_default(revs);\n \t\tset_hide(revs);\n \t\trevs->merges_imply_patch = m_imply_p;\n+\t\tgot_m = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\tset_combined(revs);\n \t\trevs->merges_imply_patch = 1;\n@@ -239,5 +241,7 @@ void diff_merges_setup_revs(struct rev_info *revs)\n \tif (revs->merges_imply_patch || revs->merges_need_diff) {\n \t\tif (!revs->diffopt.output_format)\n \t\t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\n-\t}\n+\t} else if (got_m)\n+\t\twarning(_(\"legacy use of lone '-m' detected: please use '--diff-merges=on,hide' instead, as '-m' may imply '-p'\"));\n+\n }\n-- \n2.25.1\n\n"},{"id":"469221","messageId":"20221217132955.108542-6-sorganov@gmail.com","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"[PATCH v1 5/5] diff-merges: improve --diff-merges documentation","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-17T13:29:55Z","receivedAt":"2022-12-17T13:30:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"As we've now added a flag to --diff-merges, and so there are now flags\nand formats, document them as such.\n\nAnother improvement is that now there is no extremely long line\ncontaing all the --diff-merges options.\n\nSigned-off-by: Sergey Organov \n---\n Documentation/diff-options.txt | 114 ++++++++++++++++++---------------\n 1 file changed, 62 insertions(+), 52 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 977f9135b0d6..a3fbdb85a8b8 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -34,76 +34,86 @@ endif::git-diff[]\n endif::git-format-patch[]\n \n ifdef::git-log[]\n---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::\n+-m::\n+\tShow diffs for merge commits in the default format.\n+\tShortcut for '--diff-merges=on,hide' unless\n+\t`log.diffMerges-m-imply-p` configuration is active, in which\n+\tcase it's a shortcut for '--diff-merges=on -p'.\n+\n+-c::\n+\tShortcut for '--diff-merges=combined -p'.\n+\n+--cc::\n+\tShortcut for '--diff-merges=dense-combined -p'.\n+\n+--remerge-diff::\n+\tShortcut for '--diff-merges=remerge -p'.\n+\n --no-diff-merges::\n-\tSpecify diff format to be used for merge commits. Default is\n+\tSynonym for '--diff-merges=off'.\n+\n+--diff-merges=(|)[,(|),...]::\n+\tSpecify diff format and flags to be used for merge commits. Default is\n \t{diff-merges-default} unless `--first-parent` is in use, in which case\n-\t`first-parent` is the default. Comma-separated list of\n-\tsupported values is accepted as well.\n+\t`first-parent` is the default.\n +\n---diff-merges=(off|none):::\n---no-diff-merges:::\n+The last format specified has precedence, whereas flags are\n+cumulative. Comma-separated list is handy to provide flag(s) along\n+with format, e.g.: `--diff-merges=first-parent,hide` is handy form of\n+`--diff-merges=first-parent --diff-merges=hide`.\n++\n+The following formats are supported:\n++\n+--\n+off, none::\n \tDisable output of diffs for merge commits. Useful to override\n \timplied value.\n +\n---diff-merges=on:::\n---diff-merges=m:::\n--m:::\n-\tThese options make diff output for merge commits to be shown in\n-\tthe default format. The default format could be changed using\n+on, m::\n+\tMake diff output for merge commits to be shown in the default\n+\tformat. The default format could be changed using\n \t`log.diffMerges` configuration parameter, which default value\n \tis `separate`.\n +\n-\t`-m` is a shortcut for '--diff-merges=on,hide'.\n-\tIn addition it implies `-p` when `log.diffMerges-m-imply-p` is\n-\tactive.\n+first-parent, 1::\n+\tShow full diff with respect to first parent. This is the same\n+\tformat as `--patch` produces for non-merge commits.\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+separate::\n+\tShow full diff with respect to each of parents.\n+\tSeparate log entry and diff is generated for each parent.\n +\n---diff-merges=separate:::\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.\n-+\n---diff-merges=remerge:::\n---diff-merges=r:::\n---remerge-diff:::\n-\tWith this option, two-parent merge commits are remerged to\n-\tcreate a temporary tree object -- potentially containing files\n-\twith conflict markers and such. A diff is then shown between\n-\tthat temporary tree and the actual merge commit.\n+remerge, r::\n+\tRemerge two-parent merge commits to create a temporary tree\n+\tobject--potentially containing files with conflict markers\n+\tand such. A diff is then shown between that temporary tree\n+\tand the actual merge commit.\n +\n The output emitted when this option is used is subject to change, and\n so is its interaction with other options (unless explicitly\n documented).\n +\n---diff-merges=combined:::\n---diff-merges=c:::\n--c:::\n-\tWith this option, diff output for a merge commit shows the\n-\tdifferences from each of the parents to the merge result\n-\tsimultaneously instead of showing pairwise diff between a\n-\tparent and the result one at a time. Furthermore, it lists\n-\tonly files which were modified from all parents. `-c` implies\n-\t`-p`.\n+combined, c::\n+\tShow differences from each of the parents to the merge\n+\tresult simultaneously instead of showing pairwise diff between\n+\ta parent and the result one at a time. Furthermore, it lists\n+\tonly files which were modified from all parents.\n +\n---diff-merges=dense-combined:::\n---diff-merges=cc:::\n---cc:::\n-\tWith this option the output produced by\n-\t`--diff-merges=combined` is further compressed by omitting\n-\tuninteresting hunks whose contents in the parents have only\n-\ttwo variants and the merge result picks one of them without\n-\tmodification. `--cc` implies `-p`.\n+dense-combined, cc::\n+\tFurther compress output produced by `--diff-merges=combined`\n+\tby omitting uninteresting hunks whose contents in the parents\n+\thave only two variants and the merge result picks one of them\n+\twithout modification.\n+--\n +\n---diff-merges=hide:::\n---diff-merges=no-hide:::\n-\tHide (do not hide) the diff for merge commits unless `-p` options is given\n-\tas well. The default `no-hide` could be changed using `log.diffMerges`\n-\tconfiguration parameter.\n+The following flags are supported:\n++\n+--\n+[no-]hide::\n+\tHide diff for merge commits unless `-p` options is given as\n+\twell. The default `no-hide` could be changed using\n+\t`log.diffMerges` configuration parameter.\n+--\n \n --combined-all-paths::\n \tThis flag causes combined diffs (used for merge commits) to\n-- \n2.25.1\n\n"},{"id":"469242","messageId":"xmqqbko1xv86.fsf@gitster.g","threadId":"58858","inReplyTo":"20221217132955.108542-1-sorganov@gmail.com","subject":"Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-18T03:10:33Z","receivedAt":"2022-12-18T03:12:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> The last attempt to fix '-m' option for \"git log\" to imply '-p', to\n> make it consistent with similar options (-c/--cc), was called by the\n> request on the mailing list, here:\n\nIn retrospect, this old attempt may probably shouldn't have been\ndone, as there wasn't really a compelling need to change the\nbehaviour of \"-m\". The \"combined diff\" options were \"if specified,\ngive output\" from day one, unlike \"-m\" which was \"modify the\nbehaviour of '-p' if given\" for a long time. Changing any\nestablished behaviour risks breaking the exising users and the\nupside must outweigh the risk. There wasn't overwhelming upside\nback then to risk, and of course it backfired, ...\n\n> However, the suggested (and accepted at first) patch series:\n>\n> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t\n>\n> appeared to have two problems:\n>\n> * --diff-merges options are incomplete and have no way to provide\n> exact existing '-m' semantics.\n>\n> * implying '-p' by '-m' by default broke some legacy uses of\n> \"git log --firt-parent -m\".\n\n... like so. Without learning from the experience, we may repeat\ndoing the same thing over and over and expecting different outcome,\nbut it would not give us a very good end-user experience if it\nbreaks them every time we try doing that.\n\n> Due to this, the last patch of those patch series has been later\n> reverted:\n>\n> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/\n>\n> effectively leaving the issue unresolved.\n\nFairly accurate description.\n\nThese patches do look like a good approach to solve the first point\namong the \"two problems\" in the previous round. Thanks for working\non it.\n\nIIRC, the previous round (why is this round marked as v1, by the\nway?) was reviewed by some folks, so lets wait to hear from them\nhow this round does better.\n\nUnfortunately, I do not think of any \"solution\" that would avoid\nbreaking folks, if its end goal is to flip the default, either by\nhardcoding or with a configuration variable. IOW, the other one\namong the \"two problems\" in the previous round sounds unsolvable.\nWe should question if it was really an \"issue\" worth \"resolving\",\nthough.\n"},{"id":"469287","messageId":"87cz8ffp7m.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"xmqqbko1xv86.fsf@gitster.g","subject":"Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-19T14:22:21Z","receivedAt":"2022-12-19T14:22:34Z","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>> The last attempt to fix '-m' option for \"git log\" to imply '-p', to\n>> make it consistent with similar options (-c/--cc), was called by the\n>> request on the mailing list, here:\n>\n> In retrospect, this old attempt may probably shouldn't have been\n> done, as there wasn't really a compelling need to change the\n> behaviour of \"-m\". The \"combined diff\" options were \"if specified,\n> give output\" from day one, unlike \"-m\" which was \"modify the\n> behaviour of '-p' if given\" for a long time.\n\nNo, I think your memory played trick on you, at least for --cc:\n\nhttps://lore.kernel.org/git/1440110591-12941-3-git-send-email-gitster@pobox.com/\n\nPlease also see the history of -m, -c, --cc that I've tried to reproduce\nfrom the repo and mailing list, here:\n\nhttps://lore.kernel.org/git/87ilijt2c3.fsf@osv.gnss.ru/\n\nThanks,\n-- Sergey Organov\n"},{"id":"469288","messageId":"87bknzfow4.fsf@osv.gnss.ru","threadId":"58858","inReplyTo":"xmqqbko1xv86.fsf@gitster.g","subject":"Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2022-12-19T14:29:15Z","receivedAt":"2022-12-19T14:29:22Z","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> > * implying '-p' by '-m' by default broke some legacy uses of\n> > \"git log --firt-parent -m\".\n> \n> ... like so. Without learning from the experience, we may repeat\n> doing the same thing over and over and expecting different outcome,\n> but it would not give us a very good end-user experience if it\n> breaks them every time we try doing that.\n\nThe experience tells that this form was likely the only useful form of\nusing -m, and then it has been obsoleted for more than two years\nalready, since --first-parent started to imply -m:\n\nhttps://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/\n\n[...]\n\n> IIRC, the previous round (why is this round marked as v1, by the\n> way?)\n\nWell, everybody knows indexing always starts at 0, right? ;-)\n\nNow, \"git format-patch\" has the --reroll-count=N, and as that was the\nfirst re-roll, I've used \"1\" for that. Is this my mistake?\n\n> was reviewed by some folks, so lets wait to hear from them\n> how this round does better.\n\nYes, correct.\n\n>\n> Unfortunately, I do not think of any \"solution\" that would avoid\n> breaking folks, if its end goal is to flip the default, either by\n> hardcoding or with a configuration variable. IOW, the other one\n> among the \"two problems\" in the previous round sounds unsolvable.\n> We should question if it was really an \"issue\" worth \"resolving\",\n> though.\n\nDunno, but similar issue with --cc apparently was worth resolving when\nyou've changed its behavior back in 2015:\n\nhttps://lore.kernel.org/git/1440110591-12941-3-git-send-email-gitster@pobox.com/\n\nThe '-m' not behaving as people expect is an issue that I'm trying to\nsolve.\n\nThanks,\n-- Sergey Organov\n"}]}