{"thread":{"id":"53994","subject":"[PATCH 2/3] doc/git-log: describe --diff-merges=off","startedAt":"2020-08-05T22:08:56Z","lastAt":"2020-08-17T20:36:57Z","messageCount":17,"participants":["Sergey Organov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"402988","messageId":"20200805220832.3800-3-sorganov@gmail.com","threadId":"53994","inReplyTo":"20200805220832.3800-1-sorganov@gmail.com","subject":"[PATCH 2/3] doc/git-log: describe --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T22:08:31Z","receivedAt":"2020-08-05T22:08:56Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Signed-off-by: Sergey Organov <sorganov@gmail.com>\n---\n Documentation/git-log.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 9ccba65469d7..f3727c786453 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`).\n \trename or copy detection have been requested).\n \n -m::\n---diff-merges::\n \tThis flag makes the merge commits show the full diff like\n \tregular commits; for each merge parent, a separate log entry\n \tand diff is generated. An exception is that only diff against\n@@ -153,6 +152,11 @@ combined-diff option or with `--no-diff-merges`).\n \tin that case, the output represents the changes the merge\n \tbrought _into_ the then-current branch.\n \n+--diff-merges=off::\n+--no-diff-merges::\n+\tDisable output of diffs for merge commits (default). Useful to\n+\toverride `-m`, `-c`, or `--cc`.\n+\n :git-log: 1\n include::diff-options.txt[]\n \n-- \n2.20.1\n\n"},{"id":"402989","messageId":"20200805220832.3800-2-sorganov@gmail.com","threadId":"53994","inReplyTo":"20200805220832.3800-1-sorganov@gmail.com","subject":"[PATCH 1/3] revision: change \"--diff-merges\" option to require parameter","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T22:08:30Z","receivedAt":"2020-08-05T22:08:57Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"--diff-merges=off is the only accepted form for now, a synonym for\n--no-diff-merges.\n\nThis patch is a preparation for adding more values, as well as supporting\n--diff-merges=<parent>, where <parent> is single parent number to output diff\nagainst.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n revision.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 669bc856694f..417659cfcb10 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2323,8 +2323,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diff = 1;\n \t\trevs->diffopt.flags.recursive = 1;\n \t\trevs->diffopt.flags.tree_in_recursive = 1;\n-\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--diff-merges\")) {\n+\t} else if (!strcmp(arg, \"-m\")) {\n \t\trevs->ignore_merges = 0;\n+\t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n+\t\tif (!strcmp(optarg, \"off\")) {\n+\t\t\trevs->ignore_merges = 1;\n+\t\t} else {\n+\t\t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n+\t\t}\n+\t\treturn argcount;\n \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n \t\trevs->ignore_merges = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n-- \n2.20.1\n\n"},{"id":"402990","messageId":"20200805220832.3800-1-sorganov@gmail.com","threadId":"53994","inReplyTo":null,"subject":"[PATCH 0/3] Change \"--diff-merges\" to require parameter","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T22:08:29Z","receivedAt":"2020-08-05T22:08:58Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"These patches present a minimal set of changes to turn --diff-merges\nto require parameter. For now we only introduce --diff-merges=off as\nsynonym for --no-diff-merges.\n\nThese are on top of jk/log-fp-implies-m \"making --first-parent imply -m\"\nby Jeff King, that introduced --[no-]diff-merges as a boolean option to\nprovide ability to override implied -m by --no-diff-merges.\n\nSergey Organov (3):\n  revision: change \"--diff-merges\" option to require parameter\n  doc/git-log: describe --diff-merges=off\n  t/t4013: add test for --diff-merges=off\n\n Documentation/git-log.txt                     |  6 +-\n revision.c                                    |  9 ++-\n t/t4013-diff-various.sh                       |  1 +\n ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++\n 4 files changed, 92 insertions(+), 2 deletions(-)\n create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n\n-- \n2.20.1\n\n"},{"id":"402991","messageId":"20200805220832.3800-4-sorganov@gmail.com","threadId":"53994","inReplyTo":"20200805220832.3800-1-sorganov@gmail.com","subject":"[PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T22:08:32Z","receivedAt":"2020-08-05T22:08:59Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Signed-off-by: Sergey Organov <sorganov@gmail.com>\n---\n t/t4013-diff-various.sh                       |  1 +\n ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++\n 2 files changed, 79 insertions(+)\n create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 40e222c94520..86fb11cecc61 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master\n # improved by Timo's patch\n log --root --cc --patch-with-stat --summary master\n log --no-diff-merges -p --first-parent master\n+log --diff-merges=off -p --first-parent master\n log -p --first-parent master\n log -m -p --first-parent master\n log -m -p master\ndiff --git a/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master b/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\nnew file mode 100644\nindex 000000000000..c878f13c9519\n--- /dev/null\n+++ b/t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n@@ -0,0 +1,78 @@\n+$ git log --diff-merges=off -p --first-parent master\n+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64\n+Merge: 9a6d494 c7a2ab9\n+Author: A U Thor <author@example.com>\n+Date:   Mon Jun 26 00:04:00 2006 +0000\n+\n+    Merge branch 'side' into master\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor <author@example.com>\n+Date:   Mon Jun 26 00:02:00 2006 +0000\n+\n+    Third\n+\n+diff --git a/dir/sub b/dir/sub\n+index 8422d40..cead32e 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -2,3 +2,5 @@ A\n+ B\n+ C\n+ D\n++E\n++F\n+diff --git a/file1 b/file1\n+new file mode 100644\n+index 0000000..b1e6722\n+--- /dev/null\n++++ b/file1\n+@@ -0,0 +1,3 @@\n++A\n++B\n++C\n+\n+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44\n+Author: A U Thor <author@example.com>\n+Date:   Mon Jun 26 00:01:00 2006 +0000\n+\n+    Second\n+    \n+    This is the second commit.\n+\n+diff --git a/dir/sub b/dir/sub\n+index 35d242b..8422d40 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -1,2 +1,4 @@\n+ A\n+ B\n++C\n++D\n+diff --git a/file0 b/file0\n+index 01e79c3..b414108 100644\n+--- a/file0\n++++ b/file0\n+@@ -1,3 +1,6 @@\n+ 1\n+ 2\n+ 3\n++4\n++5\n++6\n+diff --git a/file2 b/file2\n+deleted file mode 100644\n+index 01e79c3..0000000\n+--- a/file2\n++++ /dev/null\n+@@ -1,3 +0,0 @@\n+-1\n+-2\n+-3\n+\n+commit 444ac553ac7612cc88969031b02b3767fb8a353a\n+Author: A U Thor <author@example.com>\n+Date:   Mon Jun 26 00:00:00 2006 +0000\n+\n+    Initial\n+$\n-- \n2.20.1\n\n"},{"id":"403000","messageId":"xmqq3650n2rc.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"20200805220832.3800-4-sorganov@gmail.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T22:31:35Z","receivedAt":"2020-08-05T22:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  t/t4013-diff-various.sh                       |  1 +\n>  ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++\n>  2 files changed, 79 insertions(+)\n>  create mode 100644 t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n>\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 40e222c94520..86fb11cecc61 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master\n>  # improved by Timo's patch\n>  log --root --cc --patch-with-stat --summary master\n>  log --no-diff-merges -p --first-parent master\n> +log --diff-merges=off -p --first-parent master\n\nI think we want to also test it in a different order, e.g.\n\n    log --first-parent --diff-merges=off -p master\n\nother than that, these three patches all look good to me.\n\nThanks.\n"},{"id":"403001","messageId":"87lfisk915.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqq3650n2rc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T22:44:22Z","receivedAt":"2020-08-05T22:44:28Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>>  t/t4013-diff-various.sh                       |  1 +\n>>  ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++\n>>  2 files changed, 79 insertions(+)\n>>  create mode 100644\n>> t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n>>\n>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>> index 40e222c94520..86fb11cecc61 100755\n>> --- a/t/t4013-diff-various.sh\n>> +++ b/t/t4013-diff-various.sh\n>> @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master\n>>  # improved by Timo's patch\n>>  log --root --cc --patch-with-stat --summary master\n>>  log --no-diff-merges -p --first-parent master\n>> +log --diff-merges=off -p --first-parent master\n>\n> I think we want to also test it in a different order, e.g.\n>\n>     log --first-parent --diff-merges=off -p master\n\nDunno, why original Jeff series didn't need \n\n     log --first-parent --no-diff-merges -p master\n\nthen?\n\n> other than that, these three patches all look good to me.\n\nThanks,\n-- Sergey\n"},{"id":"403008","messageId":"xmqqy2msllz0.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"87lfisk915.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T23:19:31Z","receivedAt":"2020-08-05T23:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n>>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>>> ---\n>>>  t/t4013-diff-various.sh                       |  1 +\n>>>  ...--diff-merges=off_-p_--first-parent_master | 78 +++++++++++++++++++\n>>>  2 files changed, 79 insertions(+)\n>>>  create mode 100644\n>>> t/t4013/diff.log_--diff-merges=off_-p_--first-parent_master\n>>>\n>>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>>> index 40e222c94520..86fb11cecc61 100755\n>>> --- a/t/t4013-diff-various.sh\n>>> +++ b/t/t4013-diff-various.sh\n>>> @@ -298,6 +298,7 @@ log --root -c --patch-with-stat --summary master\n>>>  # improved by Timo's patch\n>>>  log --root --cc --patch-with-stat --summary master\n>>>  log --no-diff-merges -p --first-parent master\n>>> +log --diff-merges=off -p --first-parent master\n>>\n>> I think we want to also test it in a different order, e.g.\n>>\n>>     log --first-parent --diff-merges=off -p master\n>\n> Dunno, why original Jeff series didn't need \n>\n>      log --first-parent --no-diff-merges -p master\n>\n> then?\n\nWho said it didn't need it?\n"},{"id":"403009","messageId":"xmqqtuxglksy.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"xmqqy2msllz0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T23:44:45Z","receivedAt":"2020-08-05T23:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Dunno, why original Jeff series didn't need \n>>\n>>      log --first-parent --no-diff-merges -p master\n>>\n>> then?\n>\n> Who said it didn't need it?\n\nTo elaborate a bit more, we are all humans, and even reviewers\nshould be given the second chance to suggest improvements to things\nthat can use them, which s/he previously missed.  If we keep saying\n\"you say this is wrong, but the other guy did the same wrong thing\nlast week\", we can never make the world better.\n\n"},{"id":"403030","messageId":"87ft90uq8w.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqqtuxglksy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-06T08:34:39Z","receivedAt":"2020-08-06T11:05:25Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> Dunno, why original Jeff series didn't need \n>>>\n>>>      log --first-parent --no-diff-merges -p master\n>>>\n>>> then?\n>>\n>> Who said it didn't need it?\n>\n> To elaborate a bit more, we are all humans, and even reviewers\n> should be given the second chance to suggest improvements to things\n> that can use them, which s/he previously missed.  If we keep saying\n> \"you say this is wrong, but the other guy did the same wrong thing\n> last week\", we can never make the world better.\n\nI asked because I thought you see some essential difference between two\ntests, as you didn't suggest to add similar permutation test to the\noriginal. I think this reply resolves my doubt.\n\nI still think this additional test not needed unless we are going to\ntest all the permutations, and there are already 6 of them even for this\nsimple test, so we'd need to add 10 more tests to the 2 originals. If\nsomebody is willing to implement machinery to test all the option\npermutations automatically... But that's out of scope of these patches.\n\nTo be honest, if we don't test all the permutations anyway, I'd rather\ntest most \"human\" variant only, that to me is:\n\n  log --first-parent -p --no-diff-merges master\n\nbecause --first-parent tells how to travel in history, and the rest\ntells how to represent results, starting from generic \"show me diffs\"\nand followed by \"oh, but don't bother me with merge diffs, please\".\n\nAnd now we are back to my original suggestion to invert the order of\nparameters in these tests, but approached from another direction ;)\n\nOverall, I can obviously add such a test to the series if you insist,\nbut to me it still looks pointless. What doesn't look pointless is\nreplacing the original with the version you suggested, for the reasons\nthat I've already explained in preliminary discussions of these patches.\n\nFinally, if you still insist on additional test(s), please also tell if\nI should add similar permutation to the original --no-diff-merges test.\n\nThanks,\n-- Sergey\n"},{"id":"403054","messageId":"xmqqmu37lmym.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"87ft90uq8w.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T17:10:25Z","receivedAt":"2020-08-06T17:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> I asked because I thought you see some essential difference between two\n> tests, as you didn't suggest to add similar permutation test to the\n> original. I think this reply resolves my doubt.\n\nYeah, I didn't explain this very well.\n\nThe thing is that the way \"--first-parent\" interacts with other\noptions that countermand \"--diff-merges\" (i.e. \"--no-diff-merges\"\nand \"--diff-merges=off\") needs to be highlighted with extra clarity,\nsimply because \"--first-parent\" is different from a simple\n\"combination\" of \"follow the first parent commit chain while\ntraversing\" and \"--diff-merges\" [*1*]\n\nIt merely \"implies\" (or \"defaults\", which is the usual word we use)\n\"--diff-merges\", so anything the user says explicitly on the command\nline countermands it, i.e.\n\n    git log --first-parent --no-diff-merges -p\n\ndoes not trigger \"--diff-merges\" because the user explicitly says\nthat diffs for patches are unwanted after saying \"--first-parent\"\n(i.e. without \"--no-diff-merges\" later, we would have done the\n\"diff-merges\" implied by \"--first-parent\").\n\nAnd\n\n    git log --no-diff-merges --first-parent -p\n\ndoes not trigger \"--diff-merges\" because the user explicitly said\nthat diffs for patches are unwanted by the time \"--first-parent\" is\nseen (i.e. without \"--no-diff-merges\" upfront, \"--first-parent\"\nwould have weighed in to enable \"--diff-merges\" behavior, but\nbecause the user already said \"no\", it wouldn't).\n\nThis is quite different from options that are orthogonal to each\nother.  We do not need permutations of \"log --root --cc\" vs \"log\n--cc --root\" tested.  We know from the code that they cannot\npossibly interact with each other.  We could still test the\npermutations, and it may give us more \"complete\" coverage, but it\nmay be of very low value.\n\nAs to permutations of additive options, we do want to make sure they\nare truly additive (as opposed to being the \"last one wins\") and as\na side effect of ensuring the additive nature, we would end up\ntesting the effect of the order of options given, e.g. \"git commit\n-m paragraph1 -m paragraph2\".\n\nAs to permutations of options that directly overrides or opposes\neach other, we may want to ensure e.g. that \"git log -p -U2 -U4\"\nuses four context lines and \"git log -p -U4 -U2\" uses two or that\n\"git format-patch --attach --no-attach\" does not use attachment and\n\"git format-patch --no-attach --attach\" does (i.e. \"last one wins\").\n\nBut again, especially for commands that use parse-options API\ncorrectly to implement their command line options, it is hard to get\nthese two cases wrong (e.g. you need to deliberately write a\ncallback to make \"log -p -U2 -U4\" additive to use six context\nlines), so it may be of lower value to throughly test permutations.\nIt would not be as low as testing permutations of unrelated options,\nthough.\n\n\n[Footnote]\n\n*1* It makes it worse that the \"-m\" option itself changes behavior\n(whether it is given explicitly or implied) when used together with\n\"--first-parent\".  Unlike \"git log -m -p\" without \"--first-parent\",\nyou cannot see the difference the mainline brought in to the side\nbranch when the \"--first-parent\" option is in use.\n"},{"id":"403096","messageId":"875z9v7b0k.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqqmu37lmym.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-06T20:52:11Z","receivedAt":"2020-08-06T20:54:27Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> I asked because I thought you see some essential difference between two\n>> tests, as you didn't suggest to add similar permutation test to the\n>> original. I think this reply resolves my doubt.\n>\n> Yeah, I didn't explain this very well.\n>\n\n> The thing is that the way \"--first-parent\" interacts with other\n> options that countermand \"--diff-merges\" (i.e. \"--no-diff-merges\"\n> and \"--diff-merges=off\") needs to be highlighted with extra clarity,\n> simply because \"--first-parent\" is different from a simple\n> \"combination\" of \"follow the first parent commit chain while\n> traversing\" and \"--diff-merges\" [*1*]\n\n[...]\n\nThanks a lot for thorough explanations, Junio! I appreciate it and will\nre-read them more carefully, in the morning.\n\nFor now, can we leave these patches as-is and then add additional tests\non top, please? I'm afraid that adding them now will force me to rewrite\ncurrent descriptions, as the result won't be minimal anymore, and I\nfeel uneasy about writing commit messages for these permuted tests\nanyway.\n\nThanks,\n-- Sergey\n"},{"id":"403758","messageId":"874kp4uoic.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqqmu37lmym.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T23:11:45Z","receivedAt":"2020-08-07T23:11:45Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> I asked because I thought you see some essential difference between two\n>> tests, as you didn't suggest to add similar permutation test to the\n>> original. I think this reply resolves my doubt.\n>\n> Yeah, I didn't explain this very well.\n\nI think you actually did in a previous thread where I've suggested to\nchange the order in the existing test to relax it slightly, but more\nexplanations never hurt.\n\n>\n> The thing is that the way \"--first-parent\" interacts with other\n> options that countermand \"--diff-merges\" (i.e. \"--no-diff-merges\"\n> and \"--diff-merges=off\") needs to be highlighted with extra clarity,\n> simply because \"--first-parent\" is different from a simple\n> \"combination\" of \"follow the first parent commit chain while\n> traversing\" and \"--diff-merges\" [*1*]\n>\n> [ *1* It makes it worse that the \"-m\" option itself changes behavior\n> (whether it is given explicitly or implied) when used together with\n> \"--first-parent\".  Unlike \"git log -m -p\" without \"--first-parent\",\n> you cannot see the difference the mainline brought in to the side\n> branch when the \"--first-parent\" option is in use. ]\n\nThe more you explain it, the more I'm reinforced in my view that the\nwhole thing is unnecessary complicated, and that we'd better fix it than\ntry to cast it in stone.\n\n> It merely \"implies\" (or \"defaults\", which is the usual word we use)\n> \"--diff-merges\", so anything the user says explicitly on the command\n> line countermands it, i.e.\n\nThis sounds exactly as if \"implied\" option were put at the beginning of\nthe options list, no?\n\nBy the way, could you please give an example in documentation where\n\"defaults\" is used to mean \"implies\", or is it informal and just for\ncommunication of intentions? I failed to find it myself, see below.\n\n>\n>     git log --first-parent --no-diff-merges -p\n>\n> does not trigger \"--diff-merges\" because the user explicitly says\n> that diffs for patches are unwanted after saying \"--first-parent\"\n> (i.e. without \"--no-diff-merges\" later, we would have done the\n> \"diff-merges\" implied by \"--first-parent\").\n>\n> And\n>\n>     git log --no-diff-merges --first-parent -p\n>\n> does not trigger \"--diff-merges\"\n\nSo, do these both effectively resolve to:\n\n      git log -m --no-diff-merges --first-parent --patch \n\nwhere first -m is the one implied by --first-parent, and the rest are\nreal options relieved from their side-effects, so that reals are\nmutually independent and thus immune to specific relative positions?\n\n> because the user explicitly said that diffs for patches are unwanted\n> by the time \"--first-parent\" is seen (i.e. without \"--no-diff-merges\"\n> upfront, \"--first-parent\" would have weighed in to enable\n> \"--diff-merges\" behavior, but because the user already said \"no\", it\n> wouldn't).\n\nThis is too complicated for my taste. I believe useful outcome could be\nachieved without such inter-dependencies, only by using options that\n/actually/ imply other option(s) by simple substitution, instead of\n\"defaulting\", whatever it actually means, that would allow to\ntechnically describe options without resorting to guessing user\nintentions, and without need to explain behavior by outcome of a set of\nexamples.\n\nThose particular options aside, there is yet another existing kind of\noption that we didn't take into account yet:\n\n      --patch-with-raw\n           Synonym for -p --raw.\n\nSynonym! Now, \"synonym\" means something very definite: that I can safely\nsubstitute one for another with no change of semantics, right? So, here\nit is, the \"implies\" that I mean -- true substitution, and also with no\nother side-effects! Synonym, perfect! For myself I call such option a\n\"convenience option\", an option that does nothing else but brings in\n(implies) specific set of other options. OK, let's remember we already\nhave that.\n\n[The only problem with it is that even longest form \"--patch --raw\" is\nshorter than the option itself, so there doesn't seem to be a point of\nhaving this synonym. Looks like historical option converted to\n\"convenience option\" some time ago.]\n\nDigging further, in an attempt to actually find where \"defaults\" term is\nused, I stomped over acutal use of \"imply\", rather than \"default\", and\nthen to yet another kind of option, \"shorthand\"! What a nice name for\nwhat I called \"convenience option\" above, I thought! Let's check:\n\n--oneline::\n\tThis is a shorthand for \"--pretty=oneline --abbrev-commit\"\n\tused together.\n\nNice, \"shorthand\" sounds exactly like \"shorter synonym\", so it must be\ntrue substitution!\n\nBut then, we have this:\n\n--no-abbrev-commit::\n\tShow the full 40-byte hexadecimal commit object name. This negates\n\t`--abbrev-commit` and those options which imply it such as\n\t\"--oneline\". It also overrides the `log.abbrevCommit` variable.\n\nSo, --oneline \"implies\", according to this piece of git documentation,\n--abbrev-commit, and simultaneously --oneline is \"shorthand\"==\"synonym\",\nthat to me means it substitutes --abbrev-commit.\n\nThe problem is that, as direct consequence of the above, \"implies\" must\nmean simple substitution, that contradicts the definition of \"implies\"\nbeing something called \"defaults\" that you gave at the beginning of your\nexplanations.\n\nSo, finally, I'm entirely lost in all this, again. There doesn't seem to\nbe strict definition that could be relied upon, and that is not a good\nsign.\n\n[Unrelated, but mention not to forget. According to description above,\nwhen --oneline implies --abbrev-commit, the --no-abbrev-commit negates\nentire --oneline rather than implied --abbrev-commit! Is this a defect\nof the description, or an intended behavior, I wonder?]\n\n>\n> This is quite different from options that are orthogonal to each\n> other.  We do not need permutations of \"log --root --cc\" vs \"log\n> --cc --root\" tested.  We know from the code that they cannot\n> possibly interact with each other.  We could still test the\n> permutations, and it may give us more \"complete\" coverage, but it\n> may be of very low value.\n\nThese I like very much!\n\nIdeally, all the different options would be orthogonal to each other,\nmaking things much simpler to grasp, test, and describe, overall. I\nbelieve this should be one of the explicit rules or guidelines for\ndesign for new options, as well as for fixing old options whenever it's\npossible without breaking things.\n\n> As to permutations of additive options, we do want to make sure they\n> are truly additive (as opposed to being the \"last one wins\") and as\n> a side effect of ensuring the additive nature, we would end up\n> testing the effect of the order of options given, e.g. \"git commit\n> -m paragraph1 -m paragraph2\".\n\nThese are to be rare exceptions, I think, with explicit tests indeed.\n\n> As to permutations of options that directly overrides or opposes\n> each other, we may want to ensure e.g. that \"git log -p -U2 -U4\"\n> uses four context lines and \"git log -p -U4 -U2\" uses two or that\n> \"git format-patch --attach --no-attach\" does not use attachment and\n> \"git format-patch --no-attach --attach\" does (i.e. \"last one wins\").\n\nYeah, the same option with different values should override itself in\ngeneral, except additives that you've mentioned above.\n\n>\n> But again, especially for commands that use parse-options API\n> correctly to implement their command line options, it is hard to get\n> these two cases wrong (e.g. you need to deliberately write a\n> callback to make \"log -p -U2 -U4\" additive to use six context\n> lines), so it may be of lower value to throughly test permutations.\n> It would not be as low as testing permutations of unrelated options,\n> though.\n\nThis is very good! The only problem remaining is to avoid creating new\ninter-dependencies and to get rid of existing wherever possible.\n\nBTW, the revision.c does not use the parse-options API, right? Is it\npossible to change that gradually, or should it be done all at once\ninstead? I mean, if I implement new option for revision.c, should I\nsomehow rather use the parse-options API instead of imitating existing\ncode in revision.c?\n\nThanks,\n-- Sergey\n"},{"id":"403427","messageId":"xmqq8sek917j.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"20200805220832.3800-3-sorganov@gmail.com","subject":"Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T00:06:56Z","receivedAt":"2020-08-12T00:07:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n>  Documentation/git-log.txt | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n> index 9ccba65469d7..f3727c786453 100644\n> --- a/Documentation/git-log.txt\n> +++ b/Documentation/git-log.txt\n> @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`).\n>  \trename or copy detection have been requested).\n>  \n>  -m::\n> ---diff-merges::\n\nShouldn't this \"--diff-merges\" be removed from here?  As [1/3]\nupdated it like so:\n\n-\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--diff-merges\")) {\n+\t} else if (!strcmp(arg, \"-m\")) {\n \t\trevs->ignore_merges = 0;\n+\t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n+\t\tif (!strcmp(optarg, \"off\")) {\n+\t\t\trevs->ignore_merges = 1;\n+\t\t} else {\n+\t\t\tdie(_(\"unknown value for --diff-merges: %s\"), optarg);\n+\t\t}\n+\t\treturn argcount;\n\n\"git log --diff-merges\" would get either an \"option --diff-merges\nrequires a value\" error from diff.c::parse_long_opt(), or an\n\"unknown value for --diff-merges: <opt>\" error from the above code.\n\nOther than that, I think 1&2/3 looks good, and we've covered the\ntests with 3/3 already, so we are in a reasonably good shape.\n\nThanks.\n\n\n>  \tThis flag makes the merge commits show the full diff like\n>  \tregular commits; for each merge parent, a separate log entry\n>  \tand diff is generated. An exception is that only diff against\n> @@ -153,6 +152,11 @@ combined-diff option or with `--no-diff-merges`).\n>  \tin that case, the output represents the changes the merge\n>  \tbrought _into_ the then-current branch.\n>  \n> +--diff-merges=off::\n> +--no-diff-merges::\n> +\tDisable output of diffs for merge commits (default). Useful to\n> +\toverride `-m`, `-c`, or `--cc`.\n> +\n>  :git-log: 1\n>  include::diff-options.txt[]\n"},{"id":"403430","messageId":"xmqqzh707kq1.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"xmqq8sek917j.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T00:48:22Z","receivedAt":"2020-08-12T00:48:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>>  Documentation/git-log.txt | 6 +++++-\n>>  1 file changed, 5 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n>> index 9ccba65469d7..f3727c786453 100644\n>> --- a/Documentation/git-log.txt\n>> +++ b/Documentation/git-log.txt\n>> @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`).\n>>  \trename or copy detection have been requested).\n>>  \n>>  -m::\n>> ---diff-merges::\n>\n> Shouldn't this \"--diff-merges\" be removed from here?\n\nSorry, my eyes.  Yes, you are removing it from here.\n\nAll is well, then.\n"},{"id":"403435","messageId":"87imdoe16z.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqqzh707kq1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] doc/git-log: describe --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-12T08:08:20Z","receivedAt":"2020-08-12T08:08:25Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Sergey Organov <sorganov@gmail.com> writes:\n>>\n>>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>>> ---\n>>>  Documentation/git-log.txt | 6 +++++-\n>>>  1 file changed, 5 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\n>>> index 9ccba65469d7..f3727c786453 100644\n>>> --- a/Documentation/git-log.txt\n>>> +++ b/Documentation/git-log.txt\n>>> @@ -145,7 +145,6 @@ combined-diff option or with `--no-diff-merges`).\n>>>  \trename or copy detection have been requested).\n>>>  \n>>>  -m::\n>>> ---diff-merges::\n>>\n>> Shouldn't this \"--diff-merges\" be removed from here?\n>\n> Sorry, my eyes.  Yes, you are removing it from here.\n\nDon't mention it, -- these \"---\" caught me as well when I first looked\nat the diff!\n\nThanks,\n-- Sergey\n"},{"id":"403833","messageId":"xmqqr1s52nv5.fsf@gitster.c.googlers.com","threadId":"53994","inReplyTo":"874kp4uoic.fsf@osv.gnss.ru","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-17T17:17:34Z","receivedAt":"2020-08-17T17:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> So, do these both effectively resolve to:\n>\n>       git log -m --no-diff-merges --first-parent --patch \n>\n> where first -m is the one implied by --first-parent, ...\n\nEh, why does --first-parent even affect the choice of -m/no-m when\n--no-diff-merges is explicitly given?\n\n> This is too complicated for my taste.\n\nNo one is forcing you to rely on the implication by --first-parent.\nYou can always give an explicit --[no-]diff-merges when you use the\n\"--first-parent\" option.\n\nThanks.\n"},{"id":"403883","messageId":"87wo1xko0y.fsf@osv.gnss.ru","threadId":"53994","inReplyTo":"xmqqr1s52nv5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t/t4013: add test for --diff-merges=off","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-17T20:36:45Z","receivedAt":"2020-08-17T20:36:57Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> So, do these both effectively resolve to:\n>>\n>>       git log -m --no-diff-merges --first-parent --patch \n>>\n>> where first -m is the one implied by --first-parent, ...\n>\n> Eh, why does --first-parent even affect the choice of -m/no-m when\n> --no-diff-merges is explicitly given?\n\nWell, let's try your question on 3 abstract options:\n\n\"Why does --X even affects the choice of --Y/no-Y when --Z is explicitly\ngiven?\"\n\nAnd what is the assumed answer?\n\n\"--X should only affect the choice of --Y/no-Y when no --Z is explicitly\ngiven\"\n\nright? Or, in other terms:\n\n\"--X implies --Y unless --Z is explicitly given (as opposed to implied?)\"\n\nDo you see the problem here? I do see this as a problem. Simple \"--X\nimplies --Y\" should be sufficient to achieve any of required behaviors.\nNo need for such complications.\n\nI just try to figure if some more or less strict model is hidden behind\nall this, and it seems there is none.\n\nThanks,\n-- Sergey\n"}]}