{"thread":{"id":"53938","subject":"[PATCH 0/3] making --first-parent imply -m","startedAt":"2020-07-28T16:36:27Z","lastAt":"2020-08-07T23:07:21Z","messageCount":86,"participants":["Jeff King","Junio C Hamano","Taylor Blau","Chris Torek","Sergey Organov"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"402211","messageId":"20200728163617.GA2649887@coredump.intra.peff.net","threadId":"53938","inReplyTo":null,"subject":"[PATCH 0/3] making --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:36:17Z","receivedAt":"2020-07-28T16:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For some projects, it's useful to walk the first parent history, looking\nat each merge commit as a normal commit introducing all of the changes\nno its side branch. E.g.:\n\n git log --first-parent -m -Sfoo -p\n\nmight show you the topic or pull request that introduced code \"foo\". But\nI quite often forget to add \"-m\", and get confused that it doesn't\nreturn any results.\n\nThis series just makes --first-parent imply -m. That doesn't change any\noutput by itself, but does mean that diff options like \"-p\", \"-S\", etc,\nbehave sensibly.\n\n [1/3]: log: drop \"--cc implies -m\" logic\n [2/3]: revision: add \"--ignore-merges\" option to counteract \"-m\"\n [3/3]: log: enable \"-m\" automatically with \"--first-parent\"\n\n Documentation/rev-list-options.txt | 1 +\n builtin/log.c | 7 +-\n revision.c | 10 ++-\n revision.h | 2 +-\n t/t4013-diff-various.sh | 1 +\n ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n t/t4013/diff.log_-p_--first-parent_master | 22 ++++++\n 7 files changed, 113 insertions(+), 8 deletions(-)\n create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n\n-Peff\n"},{"id":"402212","messageId":"20200728163637.GA2650252@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"[PATCH 1/3] log: drop \"--cc implies -m\" logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:36:37Z","receivedAt":"2020-07-28T16:36:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This was added by 82dee4160c (log: show merge commit when --cc is given,\n2015-08-20), which explains why we need it. But that commit failed to\nnotice that setup_revisions() already does the same thing, since\ncd2bdc5309 (Common option parsing for \"git log --diff\" and friends,\n2006-04-14).\n\nSigned-off-by: Jeff King \n---\nJust a cleanup that would be worth taking regardless of the rest of the\nseries.\n\n builtin/log.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d104d5c688..281d2ae8eb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -731,10 +731,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n \t/* Turn --cc/-c into -p --cc/-c when -p was not given */\n \tif (!rev->diffopt.output_format && rev->combine_merges)\n \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n-\n-\t/* Turn -m on when --cc/-c was given */\n-\tif (rev->combine_merges)\n-\t\trev->ignore_merges = 0;\n }\n \n int cmd_log(int argc, const char **argv, const char *prefix)\n-- \n2.28.0.rc2.475.g53c7e1c7f4\n\n"},{"id":"402215","messageId":"20200728163853.GB2650252@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"[PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:38:53Z","receivedAt":"2020-07-28T16:38:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-m\" option sets revs->ignore_merges to \"0\", but there's no way to\nundo it. This probably isn't something anybody overly cares about, since\n\"1\" is already the default, but it will serve as an escape hatch when we\nflip the default for ignore_merges to \"0\" in more situations.\n\nWe'll also add a few extra niceties:\n\n - initialize the value to \"-1\" to indicate \"not set\", and then resolve\n it to the normal 0/1 bool in setup_revisions(). This lets any tweak\n functions, as well as setup_revisions() itself, avoid clobbering the\n user's preference (which until now they couldn't actually express).\n\n - since we now have --ignore-merges, let's add the matching\n --no-ignore-merges, which is just a synonym for \"-m\". In fact, it's\n simpler to just document --no-ignore-merges alongside \"-m\", and\n leave it implied that its opposite countermands it.\n\nThe new test shows that this behaves just the same as the current\nbehavior without \"-m\".\n\nSigned-off-by: Jeff King \n---\nI pulled the option name from the rev_info field name. It might be too\nbroad (we are not ignoring merges during the traversal, only for the\ndiff). It could be \"--no-diff-merges\" or something, but that would\ninvolve flipping the sense of the boolean (but that would just be in the\ncode, not user-visible, so not that big a deal).\n\n Documentation/rev-list-options.txt | 1 +\n builtin/log.c | 4 +-\n revision.c | 10 ++-\n revision.h | 2 +-\n t/t4013-diff-various.sh | 1 +\n ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n 6 files changed, 90 insertions(+), 6 deletions(-)\n create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex b01b2b6773..fbd8fa0381 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n \trename or copy detection have been requested).\n \n -m::\n+--no-ignore-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\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 281d2ae8eb..39b3d773a9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -599,8 +599,8 @@ static int show_tree_object(const struct object_id *oid,\n static void show_setup_revisions_tweak(struct rev_info *rev,\n \t\t\t\t struct setup_revision_opt *opt)\n {\n-\tif (rev->ignore_merges) {\n-\t\t/* There was no \"-m\" on the command line */\n+\tif (rev->ignore_merges < 0) {\n+\t\t/* There was no \"-m\" variant on the command line */\n \t\trev->ignore_merges = 0;\n \t\tif (!rev->first_parent_only && !rev->combine_merges) {\n \t\t\t/* No \"--first-parent\", \"-c\", or \"--cc\" */\ndiff --git a/revision.c b/revision.c\nindex 6aa7f4f567..a36c4eaf26 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1795,7 +1795,7 @@ void repo_init_revisions(struct repository *r,\n \n \trevs->repo = r;\n \trevs->abbrev = DEFAULT_ABBREV;\n-\trevs->ignore_merges = 1;\n+\trevs->ignore_merges = -1;\n \trevs->simplify_history = 1;\n \trevs->pruning.repo = r;\n \trevs->pruning.flags.recursive = 1;\n@@ -2323,8 +2323,10 @@ 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\")) {\n+\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--no-ignore-merges\")) {\n \t\trevs->ignore_merges = 0;\n+\t} else if (!strcmp(arg, \"--ignore-merges\")) {\n+\t\trevs->ignore_merges = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 0;\n@@ -2834,8 +2836,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tcopy_pathspec(&revs->diffopt.pathspec,\n \t\t\t\t &revs->prune_data);\n \t}\n-\tif (revs->combine_merges)\n+\tif (revs->combine_merges && revs->ignore_merges < 0)\n \t\trevs->ignore_merges = 0;\n+\tif (revs->ignore_merges < 0)\n+\t\trevs->ignore_merges = 1;\n \tif (revs->combined_all_paths && !revs->combine_merges)\n \t\tdie(\"--combined-all-paths makes no sense without -c or --cc\");\n \ndiff --git a/revision.h b/revision.h\nindex f412ae85eb..5258024743 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -190,11 +190,11 @@ struct rev_info {\n \t\t\tshow_root_diff:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n-\t\t\tignore_merges:1,\n \t\t\tcombine_merges:1,\n \t\t\tcombined_all_paths:1,\n \t\t\tdense_combined_merges:1,\n \t\t\talways_show_header:1;\n+\tint ignore_merges:2;\n \n \t/* Format info */\n \tint\t\tshow_notes;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 43267d6024..8f9181316f 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master\n 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 --ignore-merges -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_--ignore-merges_-p_--first-parent_master b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\nnew file mode 100644\nindex 0000000000..fa0cdc8a23\n--- /dev/null\n+++ b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n@@ -0,0 +1,78 @@\n+$ git log --ignore-merges -p --first-parent 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' into master\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \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 \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 \n+Date: Mon Jun 26 00:00:00 2006 +0000\n+\n+ Initial\n+$\n-- \n2.28.0.rc2.475.g53c7e1c7f4\n\n"},{"id":"402214","messageId":"20200728163900.GC2650252@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"[PATCH 3/3] log: enable \"-m\" automatically with \"--first-parent\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:39:00Z","receivedAt":"2020-07-28T16:39:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using \"--first-parent\" to consider history as a single line of\ncommits, git-log still defaults to treating merges specially, even\nthough they could be considered as single commits in the linearized\nhistory (that just introduce all of the changes from the second and\nhigher parents).\n\nLet's instead have \"--first-parent\" imply \"-m\", which makes something\nlike:\n\n git log --first-parent -p\n\ndo what you'd expect. Likewise:\n\n git log --first-parent -Sfoo\n\nwill find \"foo\" in merge commits.\n\nNo new test is needed; we'll tweak the output of the existing\n\"--first-parent -p\" test, which now matches the \"-m --first-parent -p\"\ntest. The unchanged existing test for \"--ignore-merges\" confirms that\nthe user can get the old behavior if they want.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 3 +++\n t/t4013/diff.log_-p_--first-parent_master | 22 ++++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39b3d773a9..83b147c23a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n \t/* Turn --cc/-c into -p --cc/-c when -p was not given */\n \tif (!rev->diffopt.output_format && rev->combine_merges)\n \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n+\n+\tif (rev->first_parent_only && rev->ignore_merges < 0)\n+\t\trev->ignore_merges = 0;\n }\n \n int cmd_log(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t4013/diff.log_-p_--first-parent_master b/t/t4013/diff.log_-p_--first-parent_master\nindex c6a5876d80..fe044399f0 100644\n--- a/t/t4013/diff.log_-p_--first-parent_master\n+++ b/t/t4013/diff.log_-p_--first-parent_master\n@@ -6,6 +6,28 @@ Date: Mon Jun 26 00:04:00 2006 +0000\n \n Merge branch 'side' into master\n \n+diff --git a/dir/sub b/dir/sub\n+index cead32e..992913c 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -4,3 +4,5 @@ C\n+ D\n+ E\n+ F\n++1\n++2\n+diff --git a/file0 b/file0\n+index b414108..10a8a9f 100644\n+--- a/file0\n++++ b/file0\n+@@ -4,3 +4,6 @@\n+ 4\n+ 5\n+ 6\n++A\n++B\n++C\n+\n commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n Author: A U Thor \n Date: Mon Jun 26 00:02:00 2006 +0000\n-- \n2.28.0.rc2.475.g53c7e1c7f4\n"},{"id":"402219","messageId":"20200728164727.GA2650574@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] making --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:47:27Z","receivedAt":"2020-07-28T16:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:\n\n> For some projects, it's useful to walk the first parent history, looking\n> at each merge commit as a normal commit introducing all of the changes\n> no its side branch. E.g.:\n> \n> git log --first-parent -m -Sfoo -p\n\nOne thing I should have mentioned, since the subject may be misleading:\nthis is only for git-log, not for plumbing like rev-list (which anyway\nwould need the option in diff-tree). The commits themselves are clear\nthat this is the case, but the cover letter was not. :)\n\n-Peff\n"},{"id":"402221","messageId":"xmqqeeovegbv.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] making --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-28T16:48:20Z","receivedAt":"2020-07-28T16:48:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For some projects, it's useful to walk the first parent history, looking\n> at each merge commit as a normal commit introducing all of the changes\n> no its side branch. E.g.:\n>\n> git log --first-parent -m -Sfoo -p\n>\n> might show you the topic or pull request that introduced code \"foo\". But\n> I quite often forget to add \"-m\", and get confused that it doesn't\n> return any results.\n\nYes. I agree that --first-parent should imply -m when combined with\ndiff options like -p, --raw, etc. I am not sure if -m should kick\nin without any diff options, though. Doesn't it have side effects?\n\nThanks.\n\n"},{"id":"402223","messageId":"20200728165337.GB2650574@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqqeeovegbv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/3] making --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T16:53:37Z","receivedAt":"2020-07-28T16:53:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 09:48:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > For some projects, it's useful to walk the first parent history, looking\n> > at each merge commit as a normal commit introducing all of the changes\n> > no its side branch. E.g.:\n> >\n> > git log --first-parent -m -Sfoo -p\n> >\n> > might show you the topic or pull request that introduced code \"foo\". But\n> > I quite often forget to add \"-m\", and get confused that it doesn't\n> > return any results.\n> \n> Yes. I agree that --first-parent should imply -m when combined with\n> diff options like -p, --raw, etc. I am not sure if -m should kick\n> in without any diff options, though. Doesn't it have side effects?\n\nI couldn't find any. It impacts log_tree_diff() if we see a commit with\ntwo parents, but --first-parent will already override that.\n\nI poked around in the code and couldn't find any other cases that would\nbe impacted, nor do any tests fail. Definitely if you can find a case\nwhere it matters outside of the diff, I'd be interested to see it. :)\n\n-Peff\n"},{"id":"402222","messageId":"20200728165518.GI87373@syl.lan","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] making --first-parent imply -m","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-28T16:55:18Z","receivedAt":"2020-07-28T16:55:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 28, 2020 at 12:36:17PM -0400, Jeff King wrote:\n> For some projects, it's useful to walk the first parent history, looking\n> at each merge commit as a normal commit introducing all of the changes\n> no its side branch. E.g.:\n>\n> git log --first-parent -m -Sfoo -p\n>\n> might show you the topic or pull request that introduced code \"foo\". But\n> I quite often forget to add \"-m\", and get confused that it doesn't\n> return any results.\n>\n> This series just makes --first-parent imply -m. That doesn't change any\n> output by itself, but does mean that diff options like \"-p\", \"-S\", etc,\n> behave sensibly.\n>\n> [1/3]: log: drop \"--cc implies -m\" logic\n> [2/3]: revision: add \"--ignore-merges\" option to counteract \"-m\"\n> [3/3]: log: enable \"-m\" automatically with \"--first-parent\"\n>\n> Documentation/rev-list-options.txt | 1 +\n> builtin/log.c | 7 +-\n> revision.c | 10 ++-\n> revision.h | 2 +-\n> t/t4013-diff-various.sh | 1 +\n> ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n> t/t4013/diff.log_-p_--first-parent_master | 22 ++++++\n> 7 files changed, 113 insertions(+), 8 deletions(-)\n> create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n>\n> -Peff\n\nVery handy to have. The first patch looks good and could be taken\nindependently. The other two patches look fine to me, too, and it's\nbehavior that at least I would want.\n\nSo, the whole series can have my:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"402225","messageId":"CAPx1Gvf7d5kdPqYVvruStZuXTySBFZEWMx_1O2MtWJatUM3j+w@mail.gmail.com","threadId":"53938","inReplyTo":"20200728163853.GB2650252@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-07-28T17:52:14Z","receivedAt":"2020-07-28T17:52:28Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"First, not that anyone should particularly care: I heartily\napprove. :-)\n\nOn Tue, Jul 28, 2020 at 9:40 AM Jeff King wrote:\n> I pulled the option name from the rev_info field name. It might be too\n> broad (we are not ignoring merges during the traversal, only for the\n> diff). It could be \"--no-diff-merges\" or something, but that would\n> involve flipping the sense of the boolean (but that would just be in the\n> code, not user-visible, so not that big a deal).\n\nPerhaps a bit bikesheddy, but I would suggest some clarifying\nnotes in the documentation. The fact that `git log` *follows*\nmerges by default is pretty obvious, but the fact that it doesn't\n*diff them at all* by default appears to be surprising to most Git\nnewcomers. (It was to me, so many years ago, and I see this all\nthe time on stackoverflow.)\n\n Note that --ignore-merges / --no-ignore-merges affect\n only diff generation, not commit traversal. Note\n also that by default, \"git log -p\" does not generate\n diffs for merges except when using --first-parent.\n See also the -c and --cc options and note that the\n default for \"git show\" is \"--cc\".\n\n(The \"except when using --first-parent\" is of course new here.)\n\nThis probably needs a bit of tweaking, but the big idea is\nto communicate and emphasize three things:\n\n 1. \"git log -p\" and \"git show\" behave differently by default.\n It's a surprise, so we should call it out separately.\n\n 2. The default for \"git log -p\" is no diff at all for merge\n commits, so see -c / --cc when you're looking at -m.\n\n 3. The default for \"git show\" is \"--cc\". (This note doesn't\n really belong here; perhaps it should be separately\n listed under the -c and --cc options?)\n\nChris\n"},{"id":"402248","messageId":"xmqqv9i7cq1k.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200728163853.GB2650252@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-28T21:01:27Z","receivedAt":"2020-07-28T21:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The \"-m\" option sets revs->ignore_merges to \"0\", but there's no way to\n> undo it. This probably isn't something anybody overly cares about, since\n> \"1\" is already the default, but it will serve as an escape hatch when we\n> flip the default for ignore_merges to \"0\" in more situations.\n>\n> We'll also add a few extra niceties:\n>\n> - initialize the value to \"-1\" to indicate \"not set\", and then resolve\n> it to the normal 0/1 bool in setup_revisions(). This lets any tweak\n> functions, as well as setup_revisions() itself, avoid clobbering the\n> user's preference (which until now they couldn't actually express).\n>\n> - since we now have --ignore-merges, let's add the matching\n> --no-ignore-merges, which is just a synonym for \"-m\". In fact, it's\n> simpler to just document --no-ignore-merges alongside \"-m\", and\n> leave it implied that its opposite countermands it.\n>\n> The new test shows that this behaves just the same as the current\n> behavior without \"-m\".\n>\n> Signed-off-by: Jeff King \n> ---\n> I pulled the option name from the rev_info field name. It might be too\n> broad (we are not ignoring merges during the traversal, only for the\n> diff). It could be \"--no-diff-merges\" or something, but that would\n> involve flipping the sense of the boolean (but that would just be in the\n> code, not user-visible, so not that big a deal).\n>\n> Documentation/rev-list-options.txt | 1 +\n> builtin/log.c | 4 +-\n> revision.c | 10 ++-\n> revision.h | 2 +-\n> t/t4013-diff-various.sh | 1 +\n> ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n> 6 files changed, 90 insertions(+), 6 deletions(-)\n> create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index b01b2b6773..fbd8fa0381 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n> \trename or copy detection have been requested).\n> \n> -m::\n> +--no-ignore-merges::\n\nThis invites a natural \"does --ignore-merges exist, and if so what\ndoes it do?\" Why not to have \"--[no-]ignore-merges\" as a separate\nentry immediately after the existing \"-m\" instead?\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\nThat is,\n\n\t--[no-]ignore-merges::\n\n\t\t`--no-ignore-merges` is a synonym to `-m`.\n\t\t`--ignore-merges` countermands an earlier `-m` that\n\t\tis either explicitly or implicitly given.\n\nor something along the line, perhaps?\n\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 43267d6024..8f9181316f 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master\n> 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 --ignore-merges -p --first-parent master\n\nThis explicitly says \"I do not want to see diff for merges\" ...\n\n> log -p --first-parent master\n> log -m -p --first-parent master\n> log -m -p master\n> diff --git a/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n> new file mode 100644\n> index 0000000000..fa0cdc8a23\n> --- /dev/null\n> +++ b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master\n> @@ -0,0 +1,78 @@\n> +$ git log --ignore-merges -p --first-parent 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' into master\n\n... and that is honored here? Good.\n\n"},{"id":"402344","messageId":"20200729182206.GA2339835@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqqv9i7cq1k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T18:22:06Z","receivedAt":"2020-07-29T18:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 02:01:27PM -0700, Junio C Hamano wrote:\n\n> > -m::\n> > +--no-ignore-merges::\n> \n> This invites a natural \"does --ignore-merges exist, and if so what\n> does it do?\" Why not to have \"--[no-]ignore-merges\" as a separate\n> entry immediately after the existing \"-m\" instead?\n\nI was hoping it would all be implied and I could dodge those questions.\nBut it seems not. :)\n\nAfter thinking on it more, I flipped it to:\n\n -m::\n --diff-merges::\n [existing text...]\n\nand then I don't think we need to have another block for\n--no-diff-merges.\n\nI'll likewise add a statement that \"-m\" is implied by \"--first-parent\"\nand can be counteracted with the \"--no\" form, which I think should spell\nout all the implications of the series.\n\n-Peff\n"},{"id":"402346","messageId":"20200729183049.GA2340161@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729182206.GA2339835@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T18:30:49Z","receivedAt":"2020-07-29T18:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 29, 2020 at 02:22:07PM -0400, Jeff King wrote:\n\n> After thinking on it more, I flipped it to:\n> \n> -m::\n> --diff-merges::\n> [existing text...]\n> \n> and then I don't think we need to have another block for\n> --no-diff-merges.\n> \n> I'll likewise add a statement that \"-m\" is implied by \"--first-parent\"\n> and can be counteracted with the \"--no\" form, which I think should spell\n> out all the implications of the series.\n\nHmm, I take that back. I tried adding this:\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 0785a0cfe9..41c859e63f 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1154,7 +1154,9 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n \tand diff is generated. An exception is that only diff against\n \tthe first parent is shown when `--first-parent` option is given;\n \tin that case, the output represents the changes the merge\n-\tbrought _into_ the then-current branch.\n+\tbrought _into_ the then-current branch. Note that\n+\t`--first-parent` implies `-m` if no combined-diff option is\n+\tenabled; you can use `--no-diff-merges` to override that.\n \n -r::\n \tShow recursive diffs.\n\nbut then I'm left wondering: why would you ever want to override it? I\nadded the option as an escape hatch in case anybody really needed the\nold behavior. But I have trouble imagining a scenario where that is the\ncase, and I wonder if it is even worth advertising.\n\n-Peff\n"},{"id":"402347","messageId":"20200729184239.GB2339835@coredump.intra.peff.net","threadId":"53938","inReplyTo":"CAPx1Gvf7d5kdPqYVvruStZuXTySBFZEWMx_1O2MtWJatUM3j+w@mail.gmail.com","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T18:42:39Z","receivedAt":"2020-07-29T18:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 10:52:14AM -0700, Chris Torek wrote:\n\n> On Tue, Jul 28, 2020 at 9:40 AM Jeff King wrote:\n> > I pulled the option name from the rev_info field name. It might be too\n> > broad (we are not ignoring merges during the traversal, only for the\n> > diff). It could be \"--no-diff-merges\" or something, but that would\n> > involve flipping the sense of the boolean (but that would just be in the\n> > code, not user-visible, so not that big a deal).\n> \n> Perhaps a bit bikesheddy, but I would suggest some clarifying\n> notes in the documentation. The fact that `git log` *follows*\n> merges by default is pretty obvious, but the fact that it doesn't\n> *diff them at all* by default appears to be surprising to most Git\n> newcomers. (It was to me, so many years ago, and I see this all\n> the time on stackoverflow.)\n\nYeah, I agree this is a point of confusion. My hope is that the code\nchange makes some of the confusion go away without people having to read\nthe documentation.\n\nBut I think \"git log -p\" (or \"git log -Sfoo\") still remains as a point\nof confusion. Possibly we should be using \"--cc\" by default there, too.\nIt's obviously more expensive, but I wonder how much that really matters\nin practice. I'd rather not tie it in to this series, though, which I\nthink is more unambiguously helpful. :)\n\n> Note that --ignore-merges / --no-ignore-merges affect\n> only diff generation, not commit traversal.\n\nYeah, that would definitely reduce confusion, but I'm switching it to\n--diff-merges (and flipping the boolean nature), which I think is even\nbetter.\n\n> Note\n> also that by default, \"git log -p\" does not generate\n> diffs for merges except when using --first-parent.\n> See also the -c and --cc options and note that the\n> default for \"git show\" is \"--cc\".\n\nI think this would be useful to have in the manpage, but not under this\noption. It looks like we can expand on this under the \"Diff Formatting\"\nsection. That's still kind of buried, but is the first place that makes\nany sense to me. I'll see if I can do a patch on top of my series.\n\n-Peff\n"},{"id":"402348","messageId":"xmqqv9i69mqi.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200729183049.GA2340161@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: add \"--ignore-merges\" option to counteract \"-m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-29T18:53:25Z","receivedAt":"2020-07-29T18:53:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 29, 2020 at 02:22:07PM -0400, Jeff King wrote:\n>\n>> After thinking on it more, I flipped it to:\n>> \n>> -m::\n>> --diff-merges::\n>> [existing text...]\n>> \n>> and then I don't think we need to have another block for\n>> --no-diff-merges.\n>> \n>> I'll likewise add a statement that \"-m\" is implied by \"--first-parent\"\n>> and can be counteracted with the \"--no\" form, which I think should spell\n>> out all the implications of the series.\n>\n> Hmm, I take that back. I tried adding this:\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 0785a0cfe9..41c859e63f 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -1154,7 +1154,9 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n> \tand diff is generated. An exception is that only diff against\n> \tthe first parent is shown when `--first-parent` option is given;\n> \tin that case, the output represents the changes the merge\n> -\tbrought _into_ the then-current branch.\n> +\tbrought _into_ the then-current branch. Note that\n> +\t`--first-parent` implies `-m` if no combined-diff option is\n> +\tenabled; you can use `--no-diff-merges` to override that.\n> \n> -r::\n> \tShow recursive diffs.\n>\n> but then I'm left wondering: why would you ever want to override it? I\n> added the option as an escape hatch in case anybody really needed the\n> old behavior.\n\nOld behaviour meaning \"git log --first-parent -p master..next\" would\nmostly list commits without patches unless there is a single-parent\ncommit thrown in the first-parent chain? I would imagine it would\nbe one way to locate and view reverts, cherry-picks and \"oops I\nbotched the merge previously\" fix-ups on 'next'.\n\nI could add \"--no-merges\" to that command line but then the output\nloses all the clues on when topics are merged in what order and show\nonly those single-parent commits, so it is not exactly the same\nthing. It loses a lot, and the current behaviour strikes a very\ngood balance for the use case.\n\nSo, if we make \"-m\" the default in some cases (like \"--first-parent\"),\nwe do need a way to toggle it off.\n\nThanks.\n\n"},{"id":"402357","messageId":"20200729201002.GA2989059@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200728163617.GA2649887@coredump.intra.peff.net","subject":"[PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:10:02Z","receivedAt":"2020-07-29T20:10:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:\n\n> This series just makes --first-parent imply -m. That doesn't change any\n> output by itself, but does mean that diff options like \"-p\", \"-S\", etc,\n> behave sensibly.\n\nHere's a re-roll taking into account the discussion so far:\n\n - the escape hatch option name is flipped to \"--no-diff-merges\" (with\n \"--diff-merges\" matching \"-m\")\n\n - the documentation now discusses the change as well as the existing\n handling of merges; this involved a few extra cleanups. Try\n \"doc-diff\" for a better view of the actual rendered changes.\n\n I do think longer term we'd be better off to stop having this maze\n of ifdef'd inclusions, and just have gitdiff(7) which covers all of\n the possibilities in human-readable text (so yes, you might see a\n mention of diff-files while you're looking for info on git-log, but\n that would also broaden your mind about how the different commands\n work). But that's clearly outside the scope of this series. I think\n what's here is a strict improvement.\n\nPatches:\n\n [1/7]: log: drop \"--cc implies -m\" logic\n [2/7]: revision: add \"--no-diff-merges\" option to counteract \"-m\"\n [3/7]: log: enable \"-m\" automatically with \"--first-parent\"\n [4/7]: doc/git-log: move \"Diff Formatting\" from rev-list-options\n [5/7]: doc/git-log: drop \"-r\" diff option\n [6/7]: doc/git-log: move \"-t\" into diff-options list\n [7/7]: doc/git-log: clarify handling of merge commit diffs\n\n Documentation/diff-options.txt | 5 ++\n Documentation/git-log.txt | 43 +++++++++-\n Documentation/rev-list-options.txt | 45 -----------\n builtin/log.c | 7 +-\n revision.c | 10 ++-\n revision.h | 2 +-\n t/t4013-diff-various.sh | 1 +\n ..._--no-diff-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n t/t4013/diff.log_-p_--first-parent_master | 22 ++++++\n 9 files changed, 158 insertions(+), 55 deletions(-)\n create mode 100644 t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master\n\nRange diff from v1:\n\n1: 518deab41f = 1: 4277d82c0c log: drop \"--cc implies -m\" logic\n2: 836553f54e ! 2: 78750f9054 revision: add \"--ignore-merges\" option to counteract \"-m\"\n @@ Metadata\n Author: Jeff King \n \n ## Commit message ##\n - revision: add \"--ignore-merges\" option to counteract \"-m\"\n + revision: add \"--no-diff-merges\" option to counteract \"-m\"\n \n The \"-m\" option sets revs->ignore_merges to \"0\", but there's no way to\n undo it. This probably isn't something anybody overly cares about, since\n @@ Commit message\n functions, as well as setup_revisions() itself, avoid clobbering the\n user's preference (which until now they couldn't actually express).\n \n - - since we now have --ignore-merges, let's add the matching\n - --no-ignore-merges, which is just a synonym for \"-m\". In fact, it's\n - simpler to just document --no-ignore-merges alongside \"-m\", and\n - leave it implied that its opposite countermands it.\n + - since we now have --no-diff-merges, let's add the matching\n + --diff-merges, which is just a synonym for \"-m\". Then we don't even\n + need to document --no-diff-merges separately; it countermands the\n + long form of \"-m\" in the usual way.\n \n The new test shows that this behaves just the same as the current\n behavior without \"-m\".\n @@ Documentation/rev-list-options.txt: options may be given. See linkgit:git-diff-f\n \trename or copy detection have been requested).\n \n -m::\n -+--no-ignore-merges::\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 @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, cons\n \t\trevs->diffopt.flags.recursive = 1;\n \t\trevs->diffopt.flags.tree_in_recursive = 1;\n -\t} else if (!strcmp(arg, \"-m\")) {\n -+\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--no-ignore-merges\")) {\n ++\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--diff-merges\")) {\n \t\trevs->ignore_merges = 0;\n -+\t} else if (!strcmp(arg, \"--ignore-merges\")) {\n ++\t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n +\t\trevs->ignore_merges = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\trevs->diff = 1;\n @@ t/t4013-diff-various.sh: log --root --patch-with-stat --summary master\n 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 --ignore-merges -p --first-parent master\n ++log --no-diff-merges -p --first-parent master\n log -p --first-parent master\n log -m -p --first-parent master\n log -m -p master\n \n - ## t/t4013/diff.log_--ignore-merges_-p_--first-parent_master (new) ##\n + ## t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master (new) ##\n @@\n -+$ git log --ignore-merges -p --first-parent master\n ++$ git log --no-diff-merges -p --first-parent master\n +commit 59d314ad6f356dd08601a4cd5e530381da3e3c64\n +Merge: 9a6d494 c7a2ab9\n +Author: A U Thor \n3: 3381fefeeb ! 3: 512b230003 log: enable \"-m\" automatically with \"--first-parent\"\n @@ Commit message\n \n No new test is needed; we'll tweak the output of the existing\n \"--first-parent -p\" test, which now matches the \"-m --first-parent -p\"\n - test. The unchanged existing test for \"--ignore-merges\" confirms that\n + test. The unchanged existing test for \"--no-diff-merges\" confirms that\n the user can get the old behavior if they want.\n \n ## builtin/log.c ##\n-: ---------- > 4: 2310dd62a6 doc/git-log: move \"Diff Formatting\" from rev-list-options\n-: ---------- > 5: 7a9a6b2d94 doc/git-log: drop \"-r\" diff option\n-: ---------- > 6: e369e0ac50 doc/git-log: move \"-t\" into diff-options list\n-: ---------- > 7: c1e769448c doc/git-log: clarify handling of merge commit diffs\n"},{"id":"402358","messageId":"20200729201017.GA2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 1/7] log: drop \"--cc implies -m\" logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:10:17Z","receivedAt":"2020-07-29T20:10:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This was added by 82dee4160c (log: show merge commit when --cc is given,\n2015-08-20), which explains why we need it. But that commit failed to\nnotice that setup_revisions() already does the same thing, since\ncd2bdc5309 (Common option parsing for \"git log --diff\" and friends,\n2006-04-14).\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d104d5c688..281d2ae8eb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -731,10 +731,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n \t/* Turn --cc/-c into -p --cc/-c when -p was not given */\n \tif (!rev->diffopt.output_format && rev->combine_merges)\n \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n-\n-\t/* Turn -m on when --cc/-c was given */\n-\tif (rev->combine_merges)\n-\t\trev->ignore_merges = 0;\n }\n \n int cmd_log(int argc, const char **argv, const char *prefix)\n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402359","messageId":"20200729201020.GB2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 2/7] revision: add \"--no-diff-merges\" option to counteract \"-m\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:10:20Z","receivedAt":"2020-07-29T20:10:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-m\" option sets revs->ignore_merges to \"0\", but there's no way to\nundo it. This probably isn't something anybody overly cares about, since\n\"1\" is already the default, but it will serve as an escape hatch when we\nflip the default for ignore_merges to \"0\" in more situations.\n\nWe'll also add a few extra niceties:\n\n - initialize the value to \"-1\" to indicate \"not set\", and then resolve\n it to the normal 0/1 bool in setup_revisions(). This lets any tweak\n functions, as well as setup_revisions() itself, avoid clobbering the\n user's preference (which until now they couldn't actually express).\n\n - since we now have --no-diff-merges, let's add the matching\n --diff-merges, which is just a synonym for \"-m\". Then we don't even\n need to document --no-diff-merges separately; it countermands the\n long form of \"-m\" in the usual way.\n\nThe new test shows that this behaves just the same as the current\nbehavior without \"-m\".\n\nSigned-off-by: Jeff King \n---\n Documentation/rev-list-options.txt | 1 +\n builtin/log.c | 4 +-\n revision.c | 10 ++-\n revision.h | 2 +-\n t/t4013-diff-various.sh | 1 +\n ..._--no-diff-merges_-p_--first-parent_master | 78 +++++++++++++++++++\n 6 files changed, 90 insertions(+), 6 deletions(-)\n create mode 100644 t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex b01b2b6773..0785a0cfe9 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.\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\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 281d2ae8eb..39b3d773a9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -599,8 +599,8 @@ static int show_tree_object(const struct object_id *oid,\n static void show_setup_revisions_tweak(struct rev_info *rev,\n \t\t\t\t struct setup_revision_opt *opt)\n {\n-\tif (rev->ignore_merges) {\n-\t\t/* There was no \"-m\" on the command line */\n+\tif (rev->ignore_merges < 0) {\n+\t\t/* There was no \"-m\" variant on the command line */\n \t\trev->ignore_merges = 0;\n \t\tif (!rev->first_parent_only && !rev->combine_merges) {\n \t\t\t/* No \"--first-parent\", \"-c\", or \"--cc\" */\ndiff --git a/revision.c b/revision.c\nindex 6aa7f4f567..669bc85669 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1795,7 +1795,7 @@ void repo_init_revisions(struct repository *r,\n \n \trevs->repo = r;\n \trevs->abbrev = DEFAULT_ABBREV;\n-\trevs->ignore_merges = 1;\n+\trevs->ignore_merges = -1;\n \trevs->simplify_history = 1;\n \trevs->pruning.repo = r;\n \trevs->pruning.flags.recursive = 1;\n@@ -2323,8 +2323,10 @@ 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\")) {\n+\t} else if (!strcmp(arg, \"-m\") || !strcmp(arg, \"--diff-merges\")) {\n \t\trevs->ignore_merges = 0;\n+\t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n+\t\trevs->ignore_merges = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 0;\n@@ -2834,8 +2836,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tcopy_pathspec(&revs->diffopt.pathspec,\n \t\t\t\t &revs->prune_data);\n \t}\n-\tif (revs->combine_merges)\n+\tif (revs->combine_merges && revs->ignore_merges < 0)\n \t\trevs->ignore_merges = 0;\n+\tif (revs->ignore_merges < 0)\n+\t\trevs->ignore_merges = 1;\n \tif (revs->combined_all_paths && !revs->combine_merges)\n \t\tdie(\"--combined-all-paths makes no sense without -c or --cc\");\n \ndiff --git a/revision.h b/revision.h\nindex f412ae85eb..5258024743 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -190,11 +190,11 @@ struct rev_info {\n \t\t\tshow_root_diff:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n-\t\t\tignore_merges:1,\n \t\t\tcombine_merges:1,\n \t\t\tcombined_all_paths:1,\n \t\t\tdense_combined_merges:1,\n \t\t\talways_show_header:1;\n+\tint ignore_merges:2;\n \n \t/* Format info */\n \tint\t\tshow_notes;\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 43267d6024..40e222c945 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master\n 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 -p --first-parent master\n log -m -p --first-parent master\n log -m -p master\ndiff --git a/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master b/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master\nnew file mode 100644\nindex 0000000000..94bf1850b2\n--- /dev/null\n+++ b/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master\n@@ -0,0 +1,78 @@\n+$ git log --no-diff-merges -p --first-parent 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' into master\n+\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \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 \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 \n+Date: Mon Jun 26 00:00:00 2006 +0000\n+\n+ Initial\n+$\n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402360","messageId":"20200729201028.GC2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 3/7] log: enable \"-m\" automatically with \"--first-parent\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:10:28Z","receivedAt":"2020-07-29T20:10:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using \"--first-parent\" to consider history as a single line of\ncommits, git-log still defaults to treating merges specially, even\nthough they could be considered as single commits in the linearized\nhistory (that just introduce all of the changes from the second and\nhigher parents).\n\nLet's instead have \"--first-parent\" imply \"-m\", which makes something\nlike:\n\n git log --first-parent -p\n\ndo what you'd expect. Likewise:\n\n git log --first-parent -Sfoo\n\nwill find \"foo\" in merge commits.\n\nNo new test is needed; we'll tweak the output of the existing\n\"--first-parent -p\" test, which now matches the \"-m --first-parent -p\"\ntest. The unchanged existing test for \"--no-diff-merges\" confirms that\nthe user can get the old behavior if they want.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 3 +++\n t/t4013/diff.log_-p_--first-parent_master | 22 ++++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39b3d773a9..83b147c23a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n \t/* Turn --cc/-c into -p --cc/-c when -p was not given */\n \tif (!rev->diffopt.output_format && rev->combine_merges)\n \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n+\n+\tif (rev->first_parent_only && rev->ignore_merges < 0)\n+\t\trev->ignore_merges = 0;\n }\n \n int cmd_log(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t4013/diff.log_-p_--first-parent_master b/t/t4013/diff.log_-p_--first-parent_master\nindex c6a5876d80..fe044399f0 100644\n--- a/t/t4013/diff.log_-p_--first-parent_master\n+++ b/t/t4013/diff.log_-p_--first-parent_master\n@@ -6,6 +6,28 @@ Date: Mon Jun 26 00:04:00 2006 +0000\n \n Merge branch 'side' into master\n \n+diff --git a/dir/sub b/dir/sub\n+index cead32e..992913c 100644\n+--- a/dir/sub\n++++ b/dir/sub\n+@@ -4,3 +4,5 @@ C\n+ D\n+ E\n+ F\n++1\n++2\n+diff --git a/file0 b/file0\n+index b414108..10a8a9f 100644\n+--- a/file0\n++++ b/file0\n+@@ -4,3 +4,6 @@\n+ 4\n+ 5\n+ 6\n++A\n++B\n++C\n+\n commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n Author: A U Thor \n Date: Mon Jun 26 00:02:00 2006 +0000\n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402361","messageId":"20200729201116.GD2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 4/7] doc/git-log: move \"Diff Formatting\" from rev-list-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:11:16Z","receivedAt":"2020-07-29T20:11:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our rev-list-options.txt include has a \"Diff Formatting\" section, but it\nis ifndef'd out for all manpages except git-log. And a few bits of the\ntext are rather out of date.\n\nWe say \"some of these options are specific to git-rev-list\". That's\nobviously silly since we (even before this patch) show the content only\nfor git-log. But moreover, it's not true; each of the listed options is\nmeaningful for other diff commands.\n\nWe also say \"...however other diff options may be given. See git-diff-files\nfor more options.\" But there's no need to do so; git-log already has a\n\"Common Diff Options\" section which includes diff-options.txt.\n\nSo let's move these options over to git-log and put them with the other\ndiff options, giving a single \"diff\" section for the git-log\ndocumentation. We'll call it \"Diff Formatting\" but use the all-caps\ntop-level header to match its sibling sections. And we'll rewrite the\nsection intro to remove the useless bits and give a more generic\noverview of the section which can be later extended.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-log.txt | 42 +++++++++++++++++++++++++--\n Documentation/rev-list-options.txt | 46 ------------------------------\n 2 files changed, 40 insertions(+), 48 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 20e6d21a74..fb3998d8e0 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -111,8 +111,46 @@ include::rev-list-options.txt[]\n \n include::pretty-formats.txt[]\n \n-COMMON DIFF OPTIONS\n--------------------\n+DIFF FORMATTING\n+---------------\n+\n+By default, `git log` does not generate any diff output. The options\n+below can be used to show the changes made by each commit.\n+\n+-c::\n+\tWith this option, diff output for a merge commit\n+\tshows the differences from each of the parents to the merge result\n+\tsimultaneously instead of showing pairwise diff between a parent\n+\tand the result one at a time. Furthermore, it lists only files\n+\twhich were modified from all parents.\n+\n+--cc::\n+\tThis flag implies the `-c` option and further compresses the\n+\tpatch output by omitting uninteresting hunks whose contents in\n+\tthe parents have only two variants and the merge result picks\n+\tone of them without modification.\n+\n+--combined-all-paths::\n+\tThis flag causes combined diffs (used for merge commits) to\n+\tlist the name of the file from all parents. It thus only has\n+\teffect when -c or --cc are specified, and is likely only\n+\tuseful if filename changes are detected (i.e. when either\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+\tthe first parent is shown when `--first-parent` option is given;\n+\tin that case, the output represents the changes the merge\n+\tbrought _into_ the then-current branch.\n+\n+-r::\n+\tShow recursive diffs.\n+\n+-t::\n+\tShow the tree objects in the diff output. This implies `-r`.\n \n :git-log: 1\n include::diff-options.txt[]\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 0785a0cfe9..398178d72a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -1117,49 +1117,3 @@ ifdef::git-rev-list[]\n \tby a tab.\n endif::git-rev-list[]\n endif::git-shortlog[]\n-\n-ifndef::git-shortlog[]\n-ifndef::git-rev-list[]\n-Diff Formatting\n-~~~~~~~~~~~~~~~\n-\n-Listed below are options that control the formatting of diff output.\n-Some of them are specific to linkgit:git-rev-list[1], however other diff\n-options may be given. See linkgit:git-diff-files[1] for more options.\n-\n--c::\n-\tWith this option, diff output for a merge commit\n-\tshows the differences from each of the parents to the merge result\n-\tsimultaneously instead of showing pairwise diff between a parent\n-\tand the result one at a time. Furthermore, it lists only files\n-\twhich were modified from all parents.\n-\n---cc::\n-\tThis flag implies the `-c` option and further compresses the\n-\tpatch output by omitting uninteresting hunks whose contents in\n-\tthe parents have only two variants and the merge result picks\n-\tone of them without modification.\n-\n---combined-all-paths::\n-\tThis flag causes combined diffs (used for merge commits) to\n-\tlist the name of the file from all parents. It thus only has\n-\teffect when -c or --cc are specified, and is likely only\n-\tuseful if filename changes are detected (i.e. when either\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-\tthe first parent is shown when `--first-parent` option is given;\n-\tin that case, the output represents the changes the merge\n-\tbrought _into_ the then-current branch.\n-\n--r::\n-\tShow recursive diffs.\n-\n--t::\n-\tShow the tree objects in the diff output. This implies `-r`.\n-endif::git-rev-list[]\n-endif::git-shortlog[]\n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402362","messageId":"20200729201127.GE2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 5/7] doc/git-log: drop \"-r\" diff option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:11:27Z","receivedAt":"2020-07-29T20:11:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This has been the default since 170c04383b (Porcelain level \"log\" family\nshould recurse when diffing., 2007-08-27). There's not even a way to\nturn it off, so you'd never even want \"-r\" to override that.\n\nIt's not the default for plumbing like diff-tree, of course, but the\noption is documented separately there.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-log.txt | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex fb3998d8e0..2cbe636b2b 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -146,11 +146,8 @@ below can be used to show the changes made by each commit.\n \tin that case, the output represents the changes the merge\n \tbrought _into_ the then-current branch.\n \n--r::\n-\tShow recursive diffs.\n-\n -t::\n-\tShow the tree objects in the diff output. This implies `-r`.\n+\tShow the tree objects in the diff output.\n \n :git-log: 1\n include::diff-options.txt[]\n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402363","messageId":"20200729201210.GF2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 6/7] doc/git-log: move \"-t\" into diff-options list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:12:10Z","receivedAt":"2020-07-29T20:12:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"-t\" option is infrequently used; it doesn't deserve a spot near the\ntop of the options list. Let's push it down into the diff-options\ninclude, near the definition of --raw.\n\nWe'll protect it with a git-log ifdef, since it doesn't make any sense\nfor non-tree diff commands. Note that this means it also shows up in\ngit-show, but that's a good thing; it applies equally well there.\n\nSigned-off-by: Jeff King \n---\nI'm open to suggestions of a better spot to put it. The diff options\nlist is a rather unwieldy mess.\n\n Documentation/diff-options.txt | 5 +++++\n Documentation/git-log.txt | 3 ---\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 7987d72b02..b7af973d9c 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -73,6 +73,11 @@ ifndef::git-format-patch[]\n \tSynonym for `-p --raw`.\n endif::git-format-patch[]\n \n+ifdef::git-log[]\n+-t::\n+\tShow the tree objects in the diff output.\n+endif::git-log[]\n+\n --indent-heuristic::\n \tEnable the heuristic that shifts diff hunk boundaries to make patches\n \teasier to read. This is the default.\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 2cbe636b2b..0a4c99e5f8 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -146,9 +146,6 @@ below can be used to show the changes made by each commit.\n \tin that case, the output represents the changes the merge\n \tbrought _into_ the then-current branch.\n \n--t::\n-\tShow the tree objects in the diff output.\n-\n :git-log: 1\n include::diff-options.txt[]\n \n-- \n2.28.0.465.gd2839157e3\n\n"},{"id":"402364","messageId":"20200729201220.GG2989342@coredump.intra.peff.net","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"[PATCH v2 7/7] doc/git-log: clarify handling of merge commit diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T20:12:20Z","receivedAt":"2020-07-29T20:12:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It can be surprising that git-log doesn't show any diff for merge\ncommits by default. Arguably \"--cc\" would be a reasonable default, but\nit's very expensive (which is why we turn it on for \"git show\" but not\nfor \"git log\"). Let's at least document the current behavior, including\nthe recent \"--first-parent implies -m\" case\n\nSigned-off-by: Jeff King \n---\n Documentation/git-log.txt | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 0a4c99e5f8..9ccba65469 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -117,6 +117,13 @@ DIFF FORMATTING\n By default, `git log` does not generate any diff output. The options\n below can be used to show the changes made by each commit.\n \n+Note that unless one of `-c`, `--cc`, or `-m` is given, merge commits\n+will never show a diff, even if a diff format like `--patch` is\n+selected, nor will they match search options like `-S`. The exception is\n+when `--first-parent` is in use, in which merges are treated like normal\n+single-parent commits (this can be overridden by providing a\n+combined-diff option or with `--no-diff-merges`).\n+\n -c::\n \tWith this option, diff output for a merge commit\n \tshows the differences from each of the parents to the merge result\n-- \n2.28.0.465.gd2839157e3\n"},{"id":"402367","messageId":"xmqqeeou9ici.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200729201028.GC2989342@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/7] log: enable \"-m\" automatically with \"--first-parent\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-29T20:28:13Z","receivedAt":"2020-07-29T20:28:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 39b3d773a9..83b147c23a 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,\n> \t/* Turn --cc/-c into -p --cc/-c when -p was not given */\n> \tif (!rev->diffopt.output_format && rev->combine_merges)\n> \t\trev->diffopt.output_format = DIFF_FORMAT_PATCH;\n> +\n> +\tif (rev->first_parent_only && rev->ignore_merges < 0)\n> +\t\trev->ignore_merges = 0;\n\nNice; thanks to the previous \"initialize to -1 and override only\nwhen not touched\", this becomes quite straight-forward.\n"},{"id":"402373","messageId":"xmqq5za69h0k.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200729201116.GD2989342@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] doc/git-log: move \"Diff Formatting\" from rev-list-options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-29T20:56:59Z","receivedAt":"2020-07-29T20:57:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Our rev-list-options.txt include has a \"Diff Formatting\" section, but it\n> is ifndef'd out for all manpages except git-log. And a few bits of the\n> text are rather out of date.\n>\n> We say \"some of these options are specific to git-rev-list\". That's\n> obviously silly since we (even before this patch) show the content only\n> for git-log. But moreover, it's not true; each of the listed options is\n> meaningful for other diff commands.\n>\n> We also say \"...however other diff options may be given. See git-diff-files\n> for more options.\" But there's no need to do so; git-log already has a\n> \"Common Diff Options\" section which includes diff-options.txt.\n>\n> So let's move these options over to git-log and put them with the other\n> diff options, giving a single \"diff\" section for the git-log\n> documentation. We'll call it \"Diff Formatting\" but use the all-caps\n> top-level header to match its sibling sections. And we'll rewrite the\n> section intro to remove the useless bits and give a more generic\n> overview of the section which can be later extended.\n\nMakes sense. I first was afraid of regressing \"git show\"\ndocumentation because the conditional inclusion was\n\n > -\n > -ifndef::git-shortlog[]\n > -ifndef::git-rev-list[]\n > -Diff Formatting\n > -~~~~~~~~~~~~~~~\n > -\n\nbut it seems that Documentation/git-show.txt does not even include\nthis section being moved in the first place.\n\nWe might move these to a new file and include it from both git-log.txt\nand git-show.txt but that can be left outside the topioc.\n"},{"id":"402376","messageId":"20200729210217.GB2992025@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqq5za69h0k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 4/7] doc/git-log: move \"Diff Formatting\" from rev-list-options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T21:02:17Z","receivedAt":"2020-07-29T21:02:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 29, 2020 at 01:56:59PM -0700, Junio C Hamano wrote:\n\n> > So let's move these options over to git-log and put them with the other\n> > diff options, giving a single \"diff\" section for the git-log\n> > documentation. We'll call it \"Diff Formatting\" but use the all-caps\n> > top-level header to match its sibling sections. And we'll rewrite the\n> > section intro to remove the useless bits and give a more generic\n> > overview of the section which can be later extended.\n> \n> Makes sense. I first was afraid of regressing \"git show\"\n> documentation because the conditional inclusion was\n> \n> > -\n> > -ifndef::git-shortlog[]\n> > -ifndef::git-rev-list[]\n> > -Diff Formatting\n> > -~~~~~~~~~~~~~~~\n> > -\n> \n> but it seems that Documentation/git-show.txt does not even include\n> this section being moved in the first place.\n> \n> We might move these to a new file and include it from both git-log.txt\n> and git-show.txt but that can be left outside the topioc.\n\nYeah. As I said earlier, the maze of includes is a bit of a mess. I did\nwonder whether git-show might need some mention of merge-handling, too,\nbut it explicitly mentions that \"--cc\" is the default, and it contains\nthe whole \"combined diff\" section by including diff-generate-patch.txt.\n\nSo I think it's OK as-is, though I wouldn't object if anybody wanted to\nlook at reorganizing all of the diff documentation. :)\n\n-Peff\n"},{"id":"402377","messageId":"CAPx1GvesMkZfgVDNQcHHOWHmVTOrEnpbJB5WOqZwG14m2RCEiA@mail.gmail.com","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-07-29T21:03:08Z","receivedAt":"2020-07-29T21:03:23Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Jul 29, 2020 at 1:10 PM Jeff King wrote:\n> Here's a re-roll taking into account the discussion so far:\n>\n> - the escape hatch option name is flipped to \"--no-diff-merges\" (with\n> \"--diff-merges\" matching \"-m\")\n>\n> - the documentation now discusses the change as well as the existing\n> handling of merges; this involved a few extra cleanups. Try\n> \"doc-diff\" for a better view of the actual rendered changes.\n>\n> I do think longer term we'd be better off to stop having this maze\n> of ifdef'd inclusions, and just have gitdiff(7) which covers all of\n> the possibilities in human-readable text (so yes, you might see a\n> mention of diff-files while you're looking for info on git-log, but\n> that would also broaden your mind about how the different commands\n> work). But that's clearly outside the scope of this series. I think\n> what's here is a strict improvement.\n\nI agree. The new series looks good.\n\nChris\n"},{"id":"402378","messageId":"xmqqzh7i81tk.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200729201210.GF2989342@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/7] doc/git-log: move \"-t\" into diff-options list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-29T21:10:31Z","receivedAt":"2020-07-29T21:10:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The \"-t\" option is infrequently used; it doesn't deserve a spot near the\n> top of the options list. Let's push it down into the diff-options\n> include, near the definition of --raw.\n\nTrue. It does not have any effect unless --raw is in use (i.e. it\ndoes have an effect to \"git show --patch-with-raw\" or anything that\n(also) shows \"--raw\" output), and it makes sense to have \"-t\" near\n\"--raw\".\n"},{"id":"402385","messageId":"871rku3soc.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200729201002.GA2989059@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-07-29T21:41:39Z","receivedAt":"2020-07-29T21:41:45Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:\n>\n>> This series just makes --first-parent imply -m. That doesn't change any\n>> output by itself, but does mean that diff options like \"-p\", \"-S\", etc,\n>> behave sensibly.\n\nThis is definitely step in the right direction, thanks a lot for working\non it!\n\n>\n> Here's a re-roll taking into account the discussion so far:\n>\n> - the escape hatch option name is flipped to \"--no-diff-merges\" (with\n> \"--diff-merges\" matching \"-m\")\n\nRather than being just a synonym for -m, is there a chance for\n\"--diff-merges\" implementation to be turned to output diff to the first\nparent only, no matter if --first-parent is active or not?\n\nAlternatively, may it have a parameter such as \"-m parent-number\" of\n\"git cherry-pick\" being set to \"1\" by default?\n\nThis -m output of diffs to all the parents is in fact primary source of\nconfusion for me, even over all these mind-blowing inter-dependencies\nbetween --first-parent, --cc, -c, -m, -p and what not. Who ever needs\nthese (potentially huge) diffs against other parents, anyway?\n\nIntroduction of this new option is a great opportunity for improvement\nthat would be a pity to miss.\n\nThanks,\n-- Sergey\n"},{"id":"402386","messageId":"20200729215558.GA2994526@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqqzh7i81tk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 6/7] doc/git-log: move \"-t\" into diff-options list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T21:55:58Z","receivedAt":"2020-07-29T21:56:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 29, 2020 at 02:10:31PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The \"-t\" option is infrequently used; it doesn't deserve a spot near the\n> > top of the options list. Let's push it down into the diff-options\n> > include, near the definition of --raw.\n> \n> True. It does not have any effect unless --raw is in use (i.e. it\n> does have an effect to \"git show --patch-with-raw\" or anything that\n> (also) shows \"--raw\" output), and it makes sense to have \"-t\" near\n> \"--raw\".\n\nIt affects --name-status, etc, too. But yeah, I would think the primary\nuse is with --raw.\n\n-Peff\n"},{"id":"402607","messageId":"20200731230858.GA1461090@coredump.intra.peff.net","threadId":"53938","inReplyTo":"871rku3soc.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T23:08:58Z","receivedAt":"2020-07-31T23:09:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote:\n\n> > Here's a re-roll taking into account the discussion so far:\n> >\n> > - the escape hatch option name is flipped to \"--no-diff-merges\" (with\n> > \"--diff-merges\" matching \"-m\")\n> \n> Rather than being just a synonym for -m, is there a chance for\n> \"--diff-merges\" implementation to be turned to output diff to the first\n> parent only, no matter if --first-parent is active or not?\n>\n> Alternatively, may it have a parameter such as \"-m parent-number\" of\n> \"git cherry-pick\" being set to \"1\" by default?\n\nYes, I agree that would be a useful feature, but I don't think it needs\nto be part of this series. It could be implemented as --diff-merges=1 to\nshow only the one against the first parent, or as its own option. But we\ncan add that on top.\n\n> This -m output of diffs to all the parents is in fact primary source of\n> confusion for me, even over all these mind-blowing inter-dependencies\n> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs\n> these (potentially huge) diffs against other parents, anyway?\n\nI've used \"-m\" second-parent diffs occasionally for hunting down\nmismerges, etc, but I agree that most of the time you just want to see\nthe diff against the first parent.\n\n> Introduction of this new option is a great opportunity for improvement\n> that would be a pity to miss.\n\nAdding an optional value to the flag is something we can do later. We\nwould miss the opportunity for \"--diff-merges\" to default to\n\"--diff-merges=1\", but I'm not sure I'd want to do that anyway. Having\nit be consistent with \"-m\" seems less confusing to me, and it is already\ntoo late to change that.\n\nIf we want an option that defaults to \"1\", we can give it a new name.\nThe only thing that is lost now is that --diff-merges would already be\ntaken. :) But I think I'd probably call such an option \"--diff-parents\"\nor something like that anyway.\n\n-Peff\n"},{"id":"402644","messageId":"87mu3drynx.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200731230858.GA1461090@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-02T12:59:46Z","receivedAt":"2020-08-02T13:00:01Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote:\n>\n>> > Here's a re-roll taking into account the discussion so far:\n>> >\n>> > - the escape hatch option name is flipped to \"--no-diff-merges\" (with\n>> > \"--diff-merges\" matching \"-m\")\n>> \n>> Rather than being just a synonym for -m, is there a chance for\n>> \"--diff-merges\" implementation to be turned to output diff to the first\n>> parent only, no matter if --first-parent is active or not?\n>>\n>> Alternatively, may it have a parameter such as \"-m parent-number\" of\n>> \"git cherry-pick\" being set to \"1\" by default?\n>\n> Yes, I agree that would be a useful feature, but I don't think it needs\n> to be part of this series. It could be implemented as --diff-merges=1 to\n> show only the one against the first parent, or as its own option. But we\n> can add that on top.\n>\n>> This -m output of diffs to all the parents is in fact primary source of\n>> confusion for me, even over all these mind-blowing inter-dependencies\n>> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs\n>> these (potentially huge) diffs against other parents, anyway?\n>\n> I've used \"-m\" second-parent diffs occasionally for hunting down\n> mismerges, etc, but I agree that most of the time you just want to see\n> the diff against the first parent.\n>\n>> Introduction of this new option is a great opportunity for improvement\n>> that would be a pity to miss.\n>\n> Adding an optional value to the flag is something we can do later. We\n> would miss the opportunity for \"--diff-merges\" to default to\n> \"--diff-merges=1\", but I'm not sure I'd want to do that anyway. Having\n> it be consistent with \"-m\" seems less confusing to me, and it is already\n> too late to change that.\n>\n> If we want an option that defaults to \"1\", we can give it a new name.\n> The only thing that is lost now is that --diff-merges would already be\n> taken. :) But I think I'd probably call such an option \"--diff-parents\"\n> or something like that anyway.\n\nYeah, I see your point, thanks for considering!\n\nWhat I in fact have in mind is something like:\n\n --diff-merges[=|c|cc|all]\n\nto have a single point of definition of the needed format of merges\nrepresentation.\n\nThen comes the question of the default. \"all\" is what'd make it behave\nas \"-m\" behaves now. If it's too late for --diff-merges to have\ndifferent default, could the default possibly be a configuration\noption rather than yet another command-line option?\n\n-- Sergey\n"},{"id":"402664","messageId":"xmqqsgd5rlwi.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87mu3drynx.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-02T17:35:25Z","receivedAt":"2020-08-02T17:35:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> What I in fact have in mind is something like:\n>\n> --diff-merges[=|c|cc|all]\n>\n> to have a single point of definition of the needed format of merges\n> representation.\n\nA command line option that takes _optional_ argument is evil; it\nhurts teachability (e.g. \"why does this option always require\n\"--opt=val\" and refuses '--opt val'?\"). Making the optional value\nconfigurable is even more so (e.g. \"teacher said to use '--opt', but\nit does not behave the way she taught---ah, I have this config\").\n\n> Then comes the question of the default. \"all\" is what'd make it behave\n> as \"-m\" behaves now. If it's too late for --diff-merges to have\n> different default, could the default possibly be a configuration\n> option rather than yet another command-line option?\n\nIntroduce --diff-parent=(none||c|cc|all) that is\ndifferent from --diff-merges, and -m and --[no-]diff-merges can be\ndefined in terms of that, at which point we can gradually deprecate\nthem if we wanted to, no?\n\n"},{"id":"402687","messageId":"87o8nrybnb.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqsgd5rlwi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T15:47:20Z","receivedAt":"2020-08-03T15:47:24Z","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>> What I in fact have in mind is something like:\n>>\n>> --diff-merges[=|c|cc|all]\n>>\n>> to have a single point of definition of the needed format of merges\n>> representation.\n>\n> A command line option that takes _optional_ argument is evil; it\n> hurts teachability (e.g. \"why does this option always require\n> \"--opt=val\" and refuses '--opt val'?\").\n\nYeah, I sympathize.\n\n> Making the optional value configurable is even more so (e.g. \"teacher\n> said to use '--opt', but it does not behave the way she taught---ah, I\n> have this config\").\n\nOK, let's drop the suggestion then.\n\n>\n>> Then comes the question of the default. \"all\" is what'd make it behave\n>> as \"-m\" behaves now. If it's too late for --diff-merges to have\n>> different default, could the default possibly be a configuration\n>> option rather than yet another command-line option?\n>\n> Introduce --diff-parent=(none||c|cc|all) that is\n> different from --diff-merges, and -m and --[no-]diff-merges can be\n> defined in terms of that, at which point we can gradually deprecate\n> them if we wanted to, no?\n\nSounds great, I only hoped we can do it right now, with this new\n--diff-merges option, maybe as a pre-requisite to the patches in\nquestion, but Jeff said it's too late, dunno why.\n\nThanks,\n-- Sergey\n"},{"id":"402697","messageId":"xmqqd047wuuh.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87o8nrybnb.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-03T16:35:34Z","receivedAt":"2020-08-03T16:35:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Sounds great, I only hoped we can do it right now, with this new\n> --diff-merges option, maybe as a pre-requisite to the patches in\n> question, but Jeff said it's too late, dunno why.\n\nA follow-up patch or two to remove the \"--diff-merges\" option and\nadd the \"--diff-parents=(none||c|cc|all)\" option on top of\nthe jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a\npossibility.\n\nBut is it worth the delay? I dunno.\n"},{"id":"402699","messageId":"87pn87smw3.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqd047wuuh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T16:41:00Z","receivedAt":"2020-08-03T16:41:04Z","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>> Sounds great, I only hoped we can do it right now, with this new\n>> --diff-merges option, maybe as a pre-requisite to the patches in\n>> question, but Jeff said it's too late, dunno why.\n>\n> A follow-up patch or two to remove the \"--diff-merges\" option and\n> add the \"--diff-parents=(none||c|cc|all)\" option on top of\n> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a\n> possibility.\n>\n> But is it worth the delay? I dunno.\n\nI don't think it's worth the delay, provided yet another new\n--diff-parents is to be implemented rather that using --diff-merges for\nthat.\n\n-- Sergey\n"},{"id":"402703","messageId":"xmqqy2mvve65.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87pn87smw3.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-03T17:21:06Z","receivedAt":"2020-08-03T17:21:15Z","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>>> Sounds great, I only hoped we can do it right now, with this new\n>>> --diff-merges option, maybe as a pre-requisite to the patches in\n>>> question, but Jeff said it's too late, dunno why.\n>>\n>> A follow-up patch or two to remove the \"--diff-merges\" option and\n>> add the \"--diff-parents=(none||c|cc|all)\" option on top of\n>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a\n>> possibility.\n>>\n>> But is it worth the delay? I dunno.\n>\n> I don't think it's worth the delay, provided yet another new\n> --diff-parents is to be implemented rather that using --diff-merges for\n> that.\n\nI was responding to your \"it's too late, dunno why\", as you seemed\nnot to want to waste an option \"--diff-merges\" that will become\nunused when \"--diff-parents\" come and also wanted it to happen right\nnow. If you no longer want to see it happen right now, that's OK by\nme. \n\n\n\n"},{"id":"402713","messageId":"20200803180824.GA2711830@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87o8nrybnb.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-03T18:08:24Z","receivedAt":"2020-08-03T18:08:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote:\n\n> > A command line option that takes _optional_ argument is evil; it\n> > hurts teachability (e.g. \"why does this option always require\n> > \"--opt=val\" and refuses '--opt val'?\").\n> \n> Yeah, I sympathize.\n\nSorry, the optional argument was my suggestion. I don't think they're\nthat evil, but I agree they require extra knowledge for the user. I\ndon't mind avoiding them when possible (and it's definitely possible\nhere).\n\n> > Introduce --diff-parent=(none||c|cc|all) that is\n> > different from --diff-merges, and -m and --[no-]diff-merges can be\n> > defined in terms of that, at which point we can gradually deprecate\n> > them if we wanted to, no?\n> \n> Sounds great, I only hoped we can do it right now, with this new\n> --diff-merges option, maybe as a pre-requisite to the patches in\n> question, but Jeff said it's too late, dunno why.\n\nIt's too late for \"-m\" to change semantics (we could do a long\ndeprecation, but I don't see much point in doing so). But --diff-merges\nis definitely still changeable until we release v2.29. My resistance was\nmostly that I didn't want to complicate my series by adding new\nelements. But we could do something on top.\n\n-Peff\n"},{"id":"402746","messageId":"871rknsdo4.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200803180824.GA2711830@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T20:00:11Z","receivedAt":"2020-08-03T20:00:16Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote:\n>\n>> > A command line option that takes _optional_ argument is evil; it\n>> > hurts teachability (e.g. \"why does this option always require\n>> > \"--opt=val\" and refuses '--opt val'?\").\n>> \n>> Yeah, I sympathize.\n>\n> Sorry, the optional argument was my suggestion. I don't think they're\n> that evil, but I agree they require extra knowledge for the user. I\n> don't mind avoiding them when possible (and it's definitely possible\n> here).\n>\n>> > Introduce --diff-parent=(none||c|cc|all) that is\n>> > different from --diff-merges, and -m and --[no-]diff-merges can be\n>> > defined in terms of that, at which point we can gradually deprecate\n>> > them if we wanted to, no?\n>> \n>> Sounds great, I only hoped we can do it right now, with this new\n>> --diff-merges option, maybe as a pre-requisite to the patches in\n>> question, but Jeff said it's too late, dunno why.\n>\n> It's too late for \"-m\" to change semantics (we could do a long\n> deprecation, but I don't see much point in doing so).\n\nI thought not of changing semantics of -m. Suppose we introduce\n\n --diff-merges=(none||c|cc|all)\n\nbefore your patch(es). Then your patch would read: \"making --first-parent\nimply --diff-merges=1\" and it'd miss that --[no-]diff-merges part, no?\n\n> But --diff-merges is definitely still changeable until we release\n> v2.29. My resistance was mostly that I didn't want to complicate my\n> series by adding new elements. But we could do something on top.\n\nCan't we do yours on top instead? I'd expect it'd then be even simpler.\n\nThanks,\n-- Sergey\n"},{"id":"402749","messageId":"87y2mvqxy6.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqy2mvve65.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T20:25:05Z","receivedAt":"2020-08-03T20:25:10Z","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>> Junio C Hamano writes:\n>>\n>>> Sergey Organov writes:\n>>>\n>>>> Sounds great, I only hoped we can do it right now, with this new\n>>>> --diff-merges option, maybe as a pre-requisite to the patches in\n>>>> question, but Jeff said it's too late, dunno why.\n>>>\n>>> A follow-up patch or two to remove the \"--diff-merges\" option and\n>>> add the \"--diff-parents=(none||c|cc|all)\" option on top of\n>>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a\n>>> possibility.\n>>>\n>>> But is it worth the delay? I dunno.\n>>\n>> I don't think it's worth the delay, provided yet another new\n>> --diff-parents is to be implemented rather that using --diff-merges for\n>> that.\n>\n> I was responding to your \"it's too late, dunno why\", as you seemed\n> not to want to waste an option \"--diff-merges\" that will become\n> unused when \"--diff-parents\" come and also wanted it to happen right\n> now. If you no longer want to see it happen right now, that's OK by\n> me.\n\nEh, no, as I see it, I suggested to have\n\n --diff-merges=(none||c|cc|all)\n\nright now rather than introduce yet another option (--diff-parents)\nlater, as well as to make --first-parent imply --diff-merges=1 rather\nthan \"-m\" (the latter in turn being synonym for --diff-merges=all), and\nI thought that's what was rejected by Jeff on the ground that it's too\nlate, but as he clarified in his recent response it was not.\n\nI mean, why introduce --[no-]diff-merges in the first place, if we do\nagree --xxx=(none|...) is where we'd like to end up? I thought the\nanswer was \"it's too late\", but in fact it was an answer to changing\nsemantics of -m that I don't think I suggest.\n\nAs a side-note, my secret hope is for pure \"git log -p\" to give me diff\nagainst first parent for all the commits, no matter how many parents\nthey happen to have. This desire still sounds like a job for\nconfiguration option though, rather than, or in addition to,\n--diff-merges or --diff-parents? We well can later introduce a\nconfig to assume --diff-merges= when no explicit\n--diff-merges= is specified, right?\n\nThanks,\n-- Sergey\n"},{"id":"402753","messageId":"20200803205532.GC2715275@coredump.intra.peff.net","threadId":"53938","inReplyTo":"871rknsdo4.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-03T20:55:32Z","receivedAt":"2020-08-03T20:55:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote:\n\n> > It's too late for \"-m\" to change semantics (we could do a long\n> > deprecation, but I don't see much point in doing so).\n> \n> I thought not of changing semantics of -m. Suppose we introduce\n> \n> --diff-merges=(none||c|cc|all)\n> \n> before your patch(es). Then your patch would read: \"making --first-parent\n> imply --diff-merges=1\" and it'd miss that --[no-]diff-merges part, no?\n\nSure, that would be OK with me. You'd have --diff-merges=none to get\nthe current behavior, and probably make --no-diff-merges an alias for\nthat.\n\n> > But --diff-merges is definitely still changeable until we release\n> > v2.29. My resistance was mostly that I didn't want to complicate my\n> > series by adding new elements. But we could do something on top.\n> \n> Can't we do yours on top instead? I'd expect it'd then be even simpler.\n\nMine is in 'next', so there is no rebuilding it on top of anything else\nwithout a revert. But I don't see any particular reason to do that\nversus just changing the behavior on top. What's in 'next' is generally\nnot rewound, but the behaviors are not cemented with respect to\nbackwards compatibility.\n\n-Peff\n"},{"id":"402754","messageId":"20200803205815.GD2715275@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87y2mvqxy6.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-03T20:58:15Z","receivedAt":"2020-08-03T20:58:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote:\n\n> I mean, why introduce --[no-]diff-merges in the first place, if we do\n> agree --xxx=(none|...) is where we'd like to end up? I thought the\n> answer was \"it's too late\", but in fact it was an answer to changing\n> semantics of -m that I don't think I suggest.\n\nSpelling out the between the lines of my answer a bit more, it was\nreally: I am happy enough with the topic as-is and do not want to rework\nit again. But if _you_ want to do so, I have no problem with it. :)\n\n> As a side-note, my secret hope is for pure \"git log -p\" to give me diff\n> against first parent for all the commits, no matter how many parents\n> they happen to have. This desire still sounds like a job for\n> configuration option though, rather than, or in addition to,\n> --diff-merges or --diff-parents? We well can later introduce a\n> config to assume --diff-merges= when no explicit\n> --diff-merges= is specified, right?\n\nYes, I think a config there would be reasonable as long as:\n\n - we have the command-line options to counteract it when necessary\n (i.e., --diff-parents or your advanced --diff-merges exists, too)\n\n - it only impacts porcelain \"git log\", and not plumbing like\n diff-tree.\n\n-Peff\n"},{"id":"402758","messageId":"87pn87qvl8.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200803205815.GD2715275@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T21:16:03Z","receivedAt":"2020-08-03T21:16:08Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote:\n>\n>> I mean, why introduce --[no-]diff-merges in the first place, if we do\n>> agree --xxx=(none|...) is where we'd like to end up? I thought the\n>> answer was \"it's too late\", but in fact it was an answer to changing\n>> semantics of -m that I don't think I suggest.\n>\n> Spelling out the between the lines of my answer a bit more, it was\n> really: I am happy enough with the topic as-is and do not want to rework\n> it again. But if _you_ want to do so, I have no problem with it. :)\n\nOK, I see!\n\n>\n>> As a side-note, my secret hope is for pure \"git log -p\" to give me diff\n>> against first parent for all the commits, no matter how many parents\n>> they happen to have. This desire still sounds like a job for\n>> configuration option though, rather than, or in addition to,\n>> --diff-merges or --diff-parents? We well can later introduce a\n>> config to assume --diff-merges= when no explicit\n>> --diff-merges= is specified, right?\n>\n> Yes, I think a config there would be reasonable as long as:\n>\n> - we have the command-line options to counteract it when necessary\n> (i.e., --diff-parents or your advanced --diff-merges exists, too)\n>\n> - it only impacts porcelain \"git log\", and not plumbing like\n> diff-tree.\n\nYeah, these two are totally agreed upon.\n\nThanks,\n-- Sergey\n"},{"id":"402759","messageId":"87lfivqvgf.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200803205532.GC2715275@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-03T21:18:56Z","receivedAt":"2020-08-03T21:19:02Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote:\n>\n>> > It's too late for \"-m\" to change semantics (we could do a long\n>> > deprecation, but I don't see much point in doing so).\n>> \n>> I thought not of changing semantics of -m. Suppose we introduce\n>> \n>> --diff-merges=(none||c|cc|all)\n>> \n>> before your patch(es). Then your patch would read: \"making --first-parent\n>> imply --diff-merges=1\" and it'd miss that --[no-]diff-merges part, no?\n>\n> Sure, that would be OK with me. You'd have --diff-merges=none to get\n> the current behavior, and probably make --no-diff-merges an alias for\n> that.\n\nYes, keeping --no-diff-merges as an alias might make sense, especially\nif it's on top of yours.\n\n>\n>> > But --diff-merges is definitely still changeable until we release\n>> > v2.29. My resistance was mostly that I didn't want to complicate my\n>> > series by adding new elements. But we could do something on top.\n>> \n>> Can't we do yours on top instead? I'd expect it'd then be even simpler.\n>\n> Mine is in 'next', so there is no rebuilding it on top of anything else\n> without a revert. But I don't see any particular reason to do that\n> versus just changing the behavior on top. What's in 'next' is generally\n> not rewound, but the behaviors are not cemented with respect to\n> backwards compatibility.\n\nAh, now I see, thanks!\n\n-- Sergey\n"},{"id":"402826","messageId":"874kpi47xj.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200803180824.GA2711830@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T17:50:16Z","receivedAt":"2020-08-04T17:50:37Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> It's too late for \"-m\" to change semantics (we could do a long\n> deprecation, but I don't see much point in doing so). But --diff-merges\n> is definitely still changeable until we release v2.29. My resistance was\n> mostly that I didn't want to complicate my series by adding new\n> elements. But we could do something on top.\n\nAttached is rather minimal incompatible change to --diff-merges that'd\nallow extensions in the future, to get out of urge for the discussed\nchanges. I'm going to follow-up with actual improvements and I'm aware\nit lacks documentation changes.\n\nWhat do you think, is it OK to have something like this before v2.29?\nAnd, by the way, what's approximate timeline to v2.29?\n\nAs for me, I'm not sure 'combined-all-paths' should be included and if\nit should, is it a good enough name. In addition, do we need more\ndescriptive (additional) names for \"c\" and \"cc\" modes?\n\n-- Sergey\n\n\n\nFrom b75e410797d4576e266d056ece16acf07e4b0116 Mon Sep 17 00:00:00 2001\nFrom: Sergey Organov \nDate: Tue, 4 Aug 2020 16:48:27 +0300\nSubject: [PATCH RFC] revision: change \"--diff-merges\" option to require\n parameter\n\nMake --diff-merges require parameter having one of the following values:\n\n off, all, c, cc, combined-all-paths\n\nthat are equivalents of passing the following separate options, respectively:\n\n --no-diff-merges, -m, -c, -cc, --combined-all-paths\n\nthat, except --no-diff-merges, could be deprecated later.\n\nThis gives us single option that entirely defines how merge commits are to be\nrepresented.\n\nThis patch is a preparation for supporting --diff-merges=, where\n is parent number to provide diff against, that adds new essential\nfunctionality and therefore is a separate change.\n\nSigned-off-by: Sergey Organov \n---\n revision.c | 23 ++++++++++++++++++++++-\n 1 file changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 669bc856694f..dcdff59bc36a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n \t\trevs->ignore_merges = 0;\n+\t\tif (!strcmp(optarg, \"off\")) {\n+\t\t\trevs->ignore_merges = 1;\n+\t\t} else if (!strcmp(optarg, \"all\")) {\n+\t\t\trevs->diff = 0;\n+\t\t} else if (!strcmp(optarg, \"c\")) {\n+\t\t\trevs->diff = 1;\n+\t\t\trevs->dense_combined_merges = 0;\n+\t\t\trevs->combine_merges = 1;\n+\t\t} else if (!strcmp(optarg, \"cc\")) {\n+\t\t\trevs->diff = 1;\n+\t\t\trevs->dense_combined_merges = 1;\n+\t\t\trevs->combine_merges = 1;\n+\t\t} else if (!strcmp(optarg, \"combined-all-paths\")) {\n+\t\t\trevs->diff = 1;\n+\t\t\trevs->combined_all_paths = 1;\n+\t\t} else {\n+\t\t\tdie(\"--diff-merges: unknown value '%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, \"-m\")) {\n+\t\trevs->ignore_merges = 0;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 0;\n-- \n2.20.1\n\n"},{"id":"402832","messageId":"xmqqbljqrydm.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"874kpi47xj.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T19:42:45Z","receivedAt":"2020-08-04T19:42:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Jeff King writes:\n>\n>> It's too late for \"-m\" to change semantics (we could do a long\n>> deprecation, but I don't see much point in doing so). But --diff-merges\n>> is definitely still changeable until we release v2.29. My resistance was\n>> mostly that I didn't want to complicate my series by adding new\n>> elements. But we could do something on top.\n>\n> Attached is rather minimal incompatible change to --diff-merges that'd\n> allow extensions in the future, to get out of urge for the discussed\n> changes. I'm going to follow-up with actual improvements and I'm aware\n> it lacks documentation changes.\n\nThe overall direction is probably OK, but I wouldn't call it minimal.\n\n> What do you think, is it OK to have something like this before v2.29?\n> And, by the way, what's approximate timeline to v2.29?\n\ntinyurl.com/gitCal\n\n> As for me, I'm not sure 'combined-all-paths' should be included and if\n> it should, is it a good enough name.\n\nAs a user, I do not think I can guess, from the option name, what\nthat option is trying to do.\n\nAs a minimum patch, I think it is OK to have just 'all' and 'none'\n(not even c or cc, let alone the one with ultra-long name whose\neffect is mystery) before we let the result graduate to 'master'.\nOthers can be added on top, as the primary focus of Peff's series is\nto make sure \"-m\" can be countermanded, for which being able to say\n\"no\" is sufficient, and the primary reason why we are further\nfutzing with the series with this addition is to leave the door open\nfor later additions of different \"modes\" in which how\n\"--diff-merges\" option can operate (iow, Peff's was merely on/off,\nbut you are making sure others such as can be added over\ntime).\n\nThanks.\n"},{"id":"402833","messageId":"20200804195830.GA2014743@coredump.intra.peff.net","threadId":"53938","inReplyTo":"874kpi47xj.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T19:58:30Z","receivedAt":"2020-08-04T19:58:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:\n\n> Attached is rather minimal incompatible change to --diff-merges that'd\n> allow extensions in the future, to get out of urge for the discussed\n> changes. I'm going to follow-up with actual improvements and I'm aware\n> it lacks documentation changes.\n\nThanks, I like the direction here. Definitely it would need\ndocumentation, but also tests (probably in t4013 alongside the ones my\nseries added; in fact you'd probably need to adjust my tests for the\nnon-optional argument).\n\n> diff --git a/revision.c b/revision.c\n> index 669bc856694f..dcdff59bc36a 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2323,10 +2323,31 @@ 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 ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n> \t\trevs->ignore_merges = 0;\n> +\t\tif (!strcmp(optarg, \"off\")) {\n> +\t\t\trevs->ignore_merges = 1;\n> +\t\t} else if (!strcmp(optarg, \"all\")) {\n> +\t\t\trevs->diff = 0;\n\nShould this be revs->ignore_merges = 0?\n\n> +\t\t} else if (!strcmp(optarg, \"c\")) {\n> +\t\t\trevs->diff = 1;\n> +\t\t\trevs->dense_combined_merges = 0;\n> +\t\t\trevs->combine_merges = 1;\n> +\t\t} else if (!strcmp(optarg, \"cc\")) {\n> +\t\t\trevs->diff = 1;\n> +\t\t\trevs->dense_combined_merges = 1;\n> +\t\t\trevs->combine_merges = 1;\n> +\t\t} else if (!strcmp(optarg, \"combined-all-paths\")) {\n> +\t\t\trevs->diff = 1;\n> +\t\t\trevs->combined_all_paths = 1;\n\nI think Junio's suggestion to push these out to a separate patch is a\ngood one.\n\nIt's unfortunate that we have to duplicate all of the various options\nthat get set (from the \"--cc\", etc, blocks). But I think the boilerplate\nfor pushing it into a helper would make it even harder to read.\n\n> +\t\t} else {\n> +\t\t\tdie(\"--diff-merges: unknown value '%s'.\", optarg);\n> +\t\t}\n\nA few nits:\n\n - we usually don't have a period at the end of our error messages\n\n - this should probably be marked for translation, i.e.,\n die(_(\"translated message\"), optarg)\n\n - I think other similar messages are more like:\n\n unknown value for --diff-merges: %s\n\n> +\t\treturn argcount;\n> \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n> \t\trevs->ignore_merges = 1;\n\nI thought at first that the --no- form would be handled by\nparse_long_opt() via the parseopt code, but it is not using parseopt at\nall. :) So it is correct to keep this.\n\n-Peff\n"},{"id":"402834","messageId":"20200804200018.GB2014743@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqqbljqrydm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T20:00:18Z","receivedAt":"2020-08-04T20:00:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:\n\n> As a minimum patch, I think it is OK to have just 'all' and 'none'\n> (not even c or cc, let alone the one with ultra-long name whose\n> effect is mystery) before we let the result graduate to 'master'.\n> Others can be added on top, as the primary focus of Peff's series is\n> to make sure \"-m\" can be countermanded, for which being able to say\n> \"no\" is sufficient, and the primary reason why we are further\n> futzing with the series with this addition is to leave the door open\n> for later additions of different \"modes\" in which how\n> \"--diff-merges\" option can operate (iow, Peff's was merely on/off,\n> but you are making sure others such as can be added over\n> time).\n\nI like that suggestion very much. It solves the \"optional arguments are\nevil\" problem without having to worry about the other bits.\n\n-Peff\n"},{"id":"402839","messageId":"877due1688.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804200018.GB2014743@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T20:55:19Z","receivedAt":"2020-08-04T20:55:27Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:\n>\n>> As a minimum patch, I think it is OK to have just 'all' and 'none'\n>> (not even c or cc, let alone the one with ultra-long name whose\n>> effect is mystery) before we let the result graduate to 'master'.\n>> Others can be added on top, as the primary focus of Peff's series is\n>> to make sure \"-m\" can be countermanded, for which being able to say\n>> \"no\" is sufficient, and the primary reason why we are further\n>> futzing with the series with this addition is to leave the door open\n>> for later additions of different \"modes\" in which how\n>> \"--diff-merges\" option can operate (iow, Peff's was merely on/off,\n>> but you are making sure others such as can be added over\n>> time).\n>\n> I like that suggestion very much. It solves the \"optional arguments are\n> evil\" problem without having to worry about the other bits.\n\nSo do I, will do in a few minutes.\n\nI only don't like --diff-merges=none (even though it sounds great for\n--diff-parents=none) and used --diff-merges=off instead. It's not a\nstrong feeling though, and I'm fine with whatever we decide.\n\nThanks,\n-- Sergey\n"},{"id":"402840","messageId":"873652166e.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804195830.GA2014743@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T20:56:25Z","receivedAt":"2020-08-04T20:56:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:\n>\n>> Attached is rather minimal incompatible change to --diff-merges that'd\n>> allow extensions in the future, to get out of urge for the discussed\n>> changes. I'm going to follow-up with actual improvements and I'm aware\n>> it lacks documentation changes.\n>\n> Thanks, I like the direction here. Definitely it would need\n> documentation, but also tests (probably in t4013 alongside the ones my\n> series added; in fact you'd probably need to adjust my tests for the\n> non-optional argument).\n>\n>> diff --git a/revision.c b/revision.c\n>> index 669bc856694f..dcdff59bc36a 100644\n>> --- a/revision.c\n>> +++ b/revision.c\n>> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct\n>> 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 ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n>> \t\trevs->ignore_merges = 0;\n>> +\t\tif (!strcmp(optarg, \"off\")) {\n>> +\t\t\trevs->ignore_merges = 1;\n>> +\t\t} else if (!strcmp(optarg, \"all\")) {\n>> +\t\t\trevs->diff = 0;\n>\n> Should this be revs->ignore_merges = 0?\n\nIt's 4 lines above, as it's in fact common for all the cases but the\nfirst one.\n\n-- Sergey\n"},{"id":"402842","messageId":"87wo2eyuo0.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804200018.GB2014743@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T21:21:03Z","receivedAt":"2020-08-04T21:21:11Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:\n>\n>> As a minimum patch, I think it is OK to have just 'all' and 'none'\n>> (not even c or cc, let alone the one with ultra-long name whose\n>> effect is mystery) before we let the result graduate to 'master'.\n>> Others can be added on top, as the primary focus of Peff's series is\n>> to make sure \"-m\" can be countermanded, for which being able to say\n>> \"no\" is sufficient, and the primary reason why we are further\n>> futzing with the series with this addition is to leave the door open\n>> for later additions of different \"modes\" in which how\n>> \"--diff-merges\" option can operate (iow, Peff's was merely on/off,\n>> but you are making sure others such as can be added over\n>> time).\n>\n> I like that suggestion very much. It solves the \"optional arguments are\n> evil\" problem without having to worry about the other bits.\n\nHere is the patch reduced to absolute minimum, both functionally and\ntextually. I removed even 'all', as it has its own subtleties that need\nfurther discussion, so the patch only introduces --diff-merges=off.\n\nIf it looks OK, I'll do documentation and tests parts.\n\nThanks,\n-- Sergey\n\n\n\nFrom 34cba8c49e2c40fbc228b9904491633939792b6d Mon Sep 17 00:00:00 2001\nFrom: Sergey Organov \nDate: Tue, 4 Aug 2020 16:48:27 +0300\nSubject: [PATCH RFC v2] revision: change \"--diff-merges\" option to require\n parameter\n\n--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=, where is single parent number to output diff\nagainst.\n\nSigned-off-by: Sergey Organov \n---\n revision.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 669bc856694f..88a0bfdb4a4c 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(\"--diff-merges: unknown value '%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":"402843","messageId":"20200804212201.GA2020725@coredump.intra.peff.net","threadId":"53938","inReplyTo":"877due1688.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T21:22:01Z","receivedAt":"2020-08-04T21:22:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:\n\n> I only don't like --diff-merges=none (even though it sounds great for\n> --diff-parents=none) and used --diff-merges=off instead. It's not a\n> strong feeling though, and I'm fine with whatever we decide.\n\nI think that is fine. I took \"none\" to be \"diff against none of the\nparents\", which is the opposite of \"all\". But \"off\" conveys that, too.\n\n-Peff\n"},{"id":"402845","messageId":"20200804212518.GB2020725@coredump.intra.peff.net","threadId":"53938","inReplyTo":"873652166e.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T21:25:18Z","receivedAt":"2020-08-04T21:25:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:\n\n> >> diff --git a/revision.c b/revision.c\n> >> index 669bc856694f..dcdff59bc36a 100644\n> >> --- a/revision.c\n> >> +++ b/revision.c\n> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct\n> >> 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 ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n> >> \t\trevs->ignore_merges = 0;\n> >> +\t\tif (!strcmp(optarg, \"off\")) {\n> >> +\t\t\trevs->ignore_merges = 1;\n> >> +\t\t} else if (!strcmp(optarg, \"all\")) {\n> >> +\t\t\trevs->diff = 0;\n> >\n> > Should this be revs->ignore_merges = 0?\n> \n> It's 4 lines above, as it's in fact common for all the cases but the\n> first one.\n\nAh, I missed that. That raises more questions, though. ;)\n\nFor \"-m\" we do not need to set revs->diff; why do we need to do so\nhere?\n\nFor \"--cc\", we do not need to set revs->ignore_merges. Why do we need to\ndo so here? We do need it set eventually, but I think setup_revisions()\nlater handles that, and wants ignore_merges untouched to decide whether\nthe user asked for it or not.\n\n-Peff\n"},{"id":"402846","messageId":"20200804212651.GC2020725@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87wo2eyuo0.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T21:26:51Z","receivedAt":"2020-08-04T21:26:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2020 at 12:21:03AM +0300, Sergey Organov wrote:\n\n> Here is the patch reduced to absolute minimum, both functionally and\n> textually. I removed even 'all', as it has its own subtleties that need\n> further discussion, so the patch only introduces --diff-merges=off.\n\nYeah, I agree this is the minimum (though I suspect the documentation\nmay be easier with \"all\" or similar to explain \"-m\" in terms of\n--diff-merges).\n\n> If it looks OK, I'll do documentation and tests parts.\n\nMy nits about the die() message remain, but other than that it looks OK\nto me.\n\n-Peff\n"},{"id":"402847","messageId":"xmqq7duerti9.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"873652166e.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T21:27:58Z","receivedAt":"2020-08-04T21:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Jeff King writes:\n>\n>>> +\t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n>>> \t\trevs->ignore_merges = 0;\n>>> +\t\tif (!strcmp(optarg, \"off\")) {\n>>> +\t\t\trevs->ignore_merges = 1;\n>>> +\t\t} else if (!strcmp(optarg, \"all\")) {\n>>> +\t\t\trevs->diff = 0;\n>>\n>> Should this be revs->ignore_merges = 0?\n>\n> It's 4 lines above, as it's in fact common for all the cases but the\n> first one.\n\nI may be mistaken, but I thought Peff was asking about turning\nrevs->diff off. I somehow thought that the equivalence planned for\nthe short term is:\n\n (new) (peff's) (master)\n\tdiff-merges=none == --no-diff-merges == ! -m\n\tdiff-merges=all == --diff-merges == -m\n\nand future extension would add equivalents to --cc and -c, in\naddition to , which is not something we can do with the\ncurrent UI and machinery.\n\nSo, shouldn't the body of that else if clause for \"all\" be a no-op?\ni.e.\n\n\t\t} else if (!strcmp(optarg, \"all\")) {\n\t\t\t; /* nothing */\n\t\t}\n\n\n"},{"id":"402849","messageId":"87y2muxf5m.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804212518.GB2020725@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T21:41:25Z","receivedAt":"2020-08-04T21:41:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:\n>\n>> >> diff --git a/revision.c b/revision.c\n>> >> index 669bc856694f..dcdff59bc36a 100644\n>> >> --- a/revision.c\n>> >> +++ b/revision.c\n>> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct\n>> >> 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 ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n>> >> \t\trevs->ignore_merges = 0;\n>> >> +\t\tif (!strcmp(optarg, \"off\")) {\n>> >> +\t\t\trevs->ignore_merges = 1;\n>> >> +\t\t} else if (!strcmp(optarg, \"all\")) {\n>> >> +\t\t\trevs->diff = 0;\n>> >\n>> > Should this be revs->ignore_merges = 0?\n>> \n>> It's 4 lines above, as it's in fact common for all the cases but the\n>> first one.\n>\n> Ah, I missed that. That raises more questions, though. ;)\n>\n> For \"-m\" we do not need to set revs->diff; why do we need to do so\n> here?\n\nGood question!\n\nI believe --diff-merges=all should reset revs->diff back to 0 to\nentirely undo all the effects of '-c' and '--cc', provided those\nappeared before on the command-line, that '-m' fails to do.\n\nAdmittedly, I didn't yet check what is the outcome of, say,\n\"git log -c -m\". Is it = \"-c\", =\"-m\", or what?\n\nAlso, this makes 'all' not the entire synonym for '-m', and that's why I\nremoved 'all' from the second version of the patch ;-)\n\n>\n> For \"--cc\", we do not need to set revs->ignore_merges. Why do we need to\n> do so here? We do need it set eventually, but I think setup_revisions()\n> later handles that, and wants ignore_merges untouched to decide whether\n> the user asked for it or not.\n\nOK, I'll take further look at this for further changes, -- thanks for\ntelling!\n\nMy general approach though is that every of mutually exclusive options\nshould better explicitly set all the involved variables appropriately.\n\nThanks,\n-- Sergey\n"},{"id":"402850","messageId":"87tuxixezy.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqq7duerti9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T21:44:49Z","receivedAt":"2020-08-04T21:44:54Z","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>> Jeff King writes:\n>>\n>>>> +\t} else if ((argcount = parse_long_opt(\"diff-merges\", argv, &optarg))) {\n>>>> \t\trevs->ignore_merges = 0;\n>>>> +\t\tif (!strcmp(optarg, \"off\")) {\n>>>> +\t\t\trevs->ignore_merges = 1;\n>>>> +\t\t} else if (!strcmp(optarg, \"all\")) {\n>>>> +\t\t\trevs->diff = 0;\n>>>\n>>> Should this be revs->ignore_merges = 0?\n>>\n>> It's 4 lines above, as it's in fact common for all the cases but the\n>> first one.\n>\n> I may be mistaken, but I thought Peff was asking about turning\n> revs->diff off.\n\nNo, but this one was in his follow-up that I already answered a few\nminutes ago.\n\n> I somehow thought that the equivalence planned for\n> the short term is:\n>\n> (new) (peff's) (master)\n> \tdiff-merges=none == --no-diff-merges == ! -m\n> \tdiff-merges=all == --diff-merges == -m\n\nThe second one is somewhat problematic, so I excluded it for now (see\naforementioned answer for more discussion).\n\nThanks,\n-- Sergey\n"},{"id":"402852","messageId":"xmqq3652rs84.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200804212201.GA2020725@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T21:55:39Z","receivedAt":"2020-08-04T21:55:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:\n>\n>> I only don't like --diff-merges=none (even though it sounds great for\n>> --diff-parents=none) and used --diff-merges=off instead. It's not a\n>> strong feeling though, and I'm fine with whatever we decide.\n>\n> I think that is fine. I took \"none\" to be \"diff against none of the\n> parents\", which is the opposite of \"all\". But \"off\" conveys that, too.\n\nFor now, \"off\" is OK, but then we'll regret when \"all\" comes,\nbecause \"off\" would not exactly sit opposite to \"all\". On the other\nhand, \"none\" would: Compare with all parents? Compare with none of\nthe parents?\n\nThanks.\n\n"},{"id":"402853","messageId":"87imdyxedw.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804195830.GA2014743@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T21:58:03Z","receivedAt":"2020-08-04T21:58:07Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:\n\n[...]\n\n>\n>> +\t\t} else {\n>> +\t\t\tdie(\"--diff-merges: unknown value '%s'.\", optarg);\n>> +\t\t}\n>\n> A few nits:\n\nI missed this the first time, sorry!\n\n>\n> - we usually don't have a period at the end of our error messages\n\nOops, I got the dot from\n\n die(\"--unpacked= no longer supported.\");\n\nin the same file. Will fix.\n\n> - this should probably be marked for translation, i.e.,\n> die(_(\"translated message\"), optarg)\n\nOK, will do.\n\n>\n> - I think other similar messages are more like:\n>\n> unknown value for --diff-merges: %s\n\nThanks, I'll change wording to this one.\n\n-- Sergey\n"},{"id":"402861","messageId":"878seuxdz8.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqq3652rs84.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T22:06:51Z","receivedAt":"2020-08-04T22:06:56Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:\n>>\n>>> I only don't like --diff-merges=none (even though it sounds great for\n>>> --diff-parents=none) and used --diff-merges=off instead. It's not a\n>>> strong feeling though, and I'm fine with whatever we decide.\n>>\n>> I think that is fine. I took \"none\" to be \"diff against none of the\n>> parents\", which is the opposite of \"all\". But \"off\" conveys that, too.\n>\n> For now, \"off\" is OK, but then we'll regret when \"all\" comes,\n> because \"off\" would not exactly sit opposite to \"all\".\n\nIMHO, \"off\" does not need to be opposite for \"all\", as it suppresses\ndiff output altogether. I read --diff-merge=off as \"turn /off/ diff\noutput for merge commits\".\n\nBesides, \"all\", that I don't like either, is among \"c\" and \"cc\", all 3\nbeing different versions of diffs against all the parents, no?\n\n> On the other hand, \"none\" would: Compare with all parents? Compare\n> with none of the parents?\n\nI think \"none\" would have been appropriate for --diff-parents indeed,\nbut not for --diff-merges.\n\nThanks,\n-- Sergey\n"},{"id":"402862","messageId":"20200804220740.GA2022650@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87y2muxf5m.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T22:07:40Z","receivedAt":"2020-08-04T22:07:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:\n\n> >> It's 4 lines above, as it's in fact common for all the cases but the\n> >> first one.\n> >\n> > Ah, I missed that. That raises more questions, though. ;)\n> >\n> > For \"-m\" we do not need to set revs->diff; why do we need to do so\n> > here?\n> \n> Good question!\n> \n> I believe --diff-merges=all should reset revs->diff back to 0 to\n> entirely undo all the effects of '-c' and '--cc', provided those\n> appeared before on the command-line, that '-m' fails to do.\n\nMaking it counteract \"--cc\" makes sense, but revs->diff is used for much\nmore than that. So \"--cc\" sets revs->diff to 1, but so do many other\nunrelated options (e.g., \"--full-diff\" for one).\n\nI think to do it right you'd need to have this part of the code just set\na single enum variable, like:\n\n ...\n else if (!strcmp(arg, \"--cc\")) {\n revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;\n } else if (!strcmp(arg, \"-m\")) {\n revs->diff_merges = DIFF_MERGES_ALL_PARENTS;\n }\n ...etc...\n\nand then later resolve that into the set of flags we need:\n\n switch (revs->diff_merges) {\n case DIFF_MERGES_DENSE_COMBINED:\n\trevs->diff = 1;\n\trevs->dense_combined_merges = 1;\n\trevs->combine_merges = 1;\n\trevs->ignore_merges = 0;\n\tbreak;\n case DIFF_MERGES_ALL_PARENTS:\n\trevs->ignore_merges = 0;\n\tbreak;\n ...etc...\n }\n\nit may even be that we can get rid of the separate combine_merges and\ndense_combined_merges flag and just have callers look at\nrev.diff_merges, which would simplify the code even further. But that\ncleanup could also come later on top.\n\n> Admittedly, I didn't yet check what is the outcome of, say,\n> \"git log -c -m\". Is it = \"-c\", =\"-m\", or what?\n\nHaving looked at the code, I'm pretty sure it behaves like \"-c\". IMHO\nthat is a bug and the two should be mutually exclusive (i.e., override\neach other). Changing that would not be strictly backwards, but I think\nit may be OK under the notion that the current behavior is nonsense.\n\n-Peff\n"},{"id":"402863","messageId":"20200804220833.GB2022650@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87imdyxedw.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T22:08:33Z","receivedAt":"2020-08-04T22:08:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2020 at 12:58:03AM +0300, Sergey Organov wrote:\n\n> > - we usually don't have a period at the end of our error messages\n> \n> Oops, I got the dot from\n> \n> die(\"--unpacked= no longer supported.\");\n> \n> in the same file. Will fix.\n\nYeah, there are a few that have snuck in. I wouldn't be opposed to a\npatch to fix that one. :)\n\n-Peff\n"},{"id":"402864","messageId":"20200804221440.GC2022650@coredump.intra.peff.net","threadId":"53938","inReplyTo":"878seuxdz8.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T22:14:40Z","receivedAt":"2020-08-04T22:14:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote:\n\n> > For now, \"off\" is OK, but then we'll regret when \"all\" comes,\n> > because \"off\" would not exactly sit opposite to \"all\".\n> \n> IMHO, \"off\" does not need to be opposite for \"all\", as it suppresses\n> diff output altogether. I read --diff-merge=off as \"turn /off/ diff\n> output for merge commits\".\n> \n> Besides, \"all\", that I don't like either, is among \"c\" and \"cc\", all 3\n> being different versions of diffs against all the parents, no?\n\nI think \"all-parents\" is much more descriptive than \"all\" (which might\nmake you think \"all merges\", but it has nothing to do with that). It\nwould make more sense if we later add the building to say \"diff against\nparent 1\" or \"diff against parents 1 and 3\".\n\nYou might also consider whether \"combined\" is actually mutually\nexclusive with parent selection. We have focused on which parents you'd\nwant to \"-m\" against. But in the most general case, you could ask for a\ncombined-diff between parents 1 and 3 of an octopus merge.\n\nThat's just coming from the angle of \"what is the most general and\northogonal set of features\". I think the vast majority of what anyone\nwould want to do would be covered by doing a diff against only a single\nparent, and then it would almost always be the first parent. And\ncertainly you'd need to add a bunch of code to the combined diff\nmachinery to make it support arbitrary sets of parents. So this probably\nisn't that interesting a direction to go, at least for now. I'm just\nraising the issue now because we'll be locked into the semantics of this\noption, which may not be able to express the full set of what's possible\n(so we'd be stuck adding another option later).\n\n-Peff\n"},{"id":"402865","messageId":"87y2muvz0g.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804220740.GA2022650@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T22:15:27Z","receivedAt":"2020-08-04T22:15:34Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:\n>\n>> >> It's 4 lines above, as it's in fact common for all the cases but the\n>> >> first one.\n>> >\n>> > Ah, I missed that. That raises more questions, though. ;)\n>> >\n>> > For \"-m\" we do not need to set revs->diff; why do we need to do so\n>> > here?\n>> \n>> Good question!\n>> \n>> I believe --diff-merges=all should reset revs->diff back to 0 to\n>> entirely undo all the effects of '-c' and '--cc', provided those\n>> appeared before on the command-line, that '-m' fails to do.\n>\n> Making it counteract \"--cc\" makes sense, but revs->diff is used for much\n> more than that. So \"--cc\" sets revs->diff to 1, but so do many other\n> unrelated options (e.g., \"--full-diff\" for one).\n>\n> I think to do it right you'd need to have this part of the code just set\n> a single enum variable, like:\n>\n> ...\n> else if (!strcmp(arg, \"--cc\")) {\n> revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;\n> } else if (!strcmp(arg, \"-m\")) {\n> revs->diff_merges = DIFF_MERGES_ALL_PARENTS;\n> }\n> ...etc...\n>\n> and then later resolve that into the set of flags we need:\n>\n> switch (revs->diff_merges) {\n> case DIFF_MERGES_DENSE_COMBINED:\n> \trevs->diff = 1;\n> \trevs->dense_combined_merges = 1;\n> \trevs->combine_merges = 1;\n> \trevs->ignore_merges = 0;\n> \tbreak;\n> case DIFF_MERGES_ALL_PARENTS:\n> \trevs->ignore_merges = 0;\n> \tbreak;\n> ...etc...\n> }\n>\n> it may even be that we can get rid of the separate combine_merges and\n> dense_combined_merges flag and just have callers look at\n> rev.diff_merges, which would simplify the code even further. But that\n> cleanup could also come later on top.\n\nSounds like a plan! I like it.\n\nThanks,\n-- Sergey\n"},{"id":"402867","messageId":"87sgd2vyi1.fsf_-_@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804220833.GB2022650@coredump.intra.peff.net","subject":"[PATCH] revision: fix die() message for \"--unpacked=\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T22:26:30Z","receivedAt":"2020-08-04T22:26:35Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"\nGet rid of the trailing dot and mark for translation.\n\nSigned-off-by: Sergey Organov \n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 669bc856694f..d08cb5c0e9cd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2315,7 +2315,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--unpacked\")) {\n \t\trevs->unpacked = 1;\n \t} else if (starts_with(arg, \"--unpacked=\")) {\n-\t\tdie(\"--unpacked= no longer supported.\");\n+\t\tdie(_(\"--unpacked= no longer supported\"));\n \t} else if (!strcmp(arg, \"-r\")) {\n \t\trevs->diff = 1;\n \t\trevs->diffopt.flags.recursive = 1;\n-- \n2.20.1\n"},{"id":"402869","messageId":"xmqqpn86qb6a.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200804221440.GC2022650@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-04T22:49:17Z","receivedAt":"2020-08-04T22:49:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You might also consider whether \"combined\" is actually mutually\n> exclusive with parent selection. We have focused on which parents you'd\n> want to \"-m\" against. But in the most general case, you could ask for a\n> combined-diff between parents 1 and 3 of an octopus merge.\n\nYeah, we want to specify a possibly empty set of integers\n(1..) to combine the diff; if it is empty set, we won't\nsee any diff. If it is full set, we'd get the current c/cc behavior.\nAnything in between we cannot currently express. Fun ;-)\n\n> That's just coming from the angle of \"what is the most general and\n> orthogonal set of features\". I think the vast majority of what anyone\n> would want to do would be covered by doing a diff against only a single\n> parent, and then it would almost always be the first parent. And\n> certainly you'd need to add a bunch of code to the combined diff\n> machinery to make it support arbitrary sets of parents. So this probably\n> isn't that interesting a direction to go, at least for now. \n\nYeah, it is mostly for fun--I do not see an immediate practical use\ncase, either.\n\n> I'm just\n> raising the issue now because we'll be locked into the semantics of this\n> option, which may not be able to express the full set of what's possible\n> (so we'd be stuck adding another option later).\n\nYeah, but a good thing is that we won't have to worry about this\nuntil much later, as long as we would just be introducing \"diff\nagainst no parents\" and nothing else (or together with \"diff against\nall parents\", which would make it easier to explain \"-m\").\n\n"},{"id":"402870","messageId":"87d046vx8e.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804221440.GC2022650@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-04T22:53:53Z","receivedAt":"2020-08-04T22:54:00Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote:\n>\n>> > For now, \"off\" is OK, but then we'll regret when \"all\" comes,\n>> > because \"off\" would not exactly sit opposite to \"all\".\n>> \n>> IMHO, \"off\" does not need to be opposite for \"all\", as it suppresses\n>> diff output altogether. I read --diff-merge=off as \"turn /off/ diff\n>> output for merge commits\".\n>> \n>> Besides, \"all\", that I don't like either, is among \"c\" and \"cc\", all 3\n>> being different versions of diffs against all the parents, no?\n>\n> I think \"all-parents\" is much more descriptive than \"all\" (which might\n> make you think \"all merges\", but it has nothing to do with that). It\n> would make more sense if we later add the building to say \"diff against\n> parent 1\" or \"diff against parents 1 and 3\".\n>\n> You might also consider whether \"combined\" is actually mutually\n> exclusive with parent selection. We have focused on which parents you'd\n> want to \"-m\" against. But in the most general case, you could ask for a\n> combined-diff between parents 1 and 3 of an octopus merge.\n>\n> That's just coming from the angle of \"what is the most general and\n> orthogonal set of features\". I think the vast majority of what anyone\n> would want to do would be covered by doing a diff against only a single\n> parent, and then it would almost always be the first parent. And\n> certainly you'd need to add a bunch of code to the combined diff\n> machinery to make it support arbitrary sets of parents. So this probably\n> isn't that interesting a direction to go, at least for now. I'm just\n> raising the issue now because we'll be locked into the semantics of this\n> option, which may not be able to express the full set of what's possible\n> (so we'd be stuck adding another option later).\n\nMakes sense, and I got an idea.\n\n--diff-merges= will still give diff against one specific parent.\n\nIn case of combined/separate diffs, it will produce diffs against all\nthe parents that, if happens to be needed, could later be refined by a\nnew --diff-parents option that defaults to 'all'.\n\nThen, for example,\n\n--diff-merges=1\n\nwould finally be just a short-cut for\n\n--diff-merges=separate --diff-parents=1\n\nwhile\n\n--diff-merges=separate --diff-parents=all\n\nwould be the same as\n\n--diff-merges=separate (what we called \"all\" so far)\n\nand then we may have\n\n--diff-merges=condensed-combined --diff-parents=1-3,8\n\nfor the bravest ;-)\n\nThis would lead us, currently to:\n\n--diff-merges=(off,separate,combined,condensed-combined,)\n\nand leave us ability to implement advanced parents selection, in an\nunlikely case it's needed, in a separate new option.\n\nThanks,\n-- Sergey\n"},{"id":"402873","messageId":"xmqqa6zaq7qp.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87sgd2vyi1.fsf_-_@osv.gnss.ru","subject":"Re: [PATCH] revision: fix die() message for \"--unpacked=\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T00:03:26Z","receivedAt":"2020-08-05T00:03:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Get rid of the trailing dot and mark for translation.\n>\n> Signed-off-by: Sergey Organov \n> ---\n> revision.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 669bc856694f..d08cb5c0e9cd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2315,7 +2315,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t} else if (!strcmp(arg, \"--unpacked\")) {\n> \t\trevs->unpacked = 1;\n> \t} else if (starts_with(arg, \"--unpacked=\")) {\n> -\t\tdie(\"--unpacked= no longer supported.\");\n> +\t\tdie(_(\"--unpacked= no longer supported\"));\n> \t} else if (!strcmp(arg, \"-r\")) {\n> \t\trevs->diff = 1;\n> \t\trevs->diffopt.flags.recursive = 1;\n\nMakes sense. Will queue. thanks.\n"},{"id":"402910","messageId":"xmqqeeoloz74.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87k0ydp0hc.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T16:05:35Z","receivedAt":"2020-08-05T16:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Jeff King writes:\n> ...\n> In this case your original test:\n>\n> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master\n>\n> would fail, as implied --diff-merges=1 then wins.\n\nIMHO, I think this is an absolutely wrong thing to do. At least to\nme (and I suspect it would be to many users), what \"--first-parent\nimplies 'when showing a diff, compare only with the first parent'\"\nmeans is that it should do so unless told to do otherwise.\n\n git log --no-diff-merges -p --first-parent\n\nexplicitly tells the command that the user does not want to see\npatches for merge commits. I do not see any reason why\n\"--first-parent\", which merely *implies* a specific diff generation\npreference for merges, countermand it. IOW the implication is\nconditional.\n\nIt is like saying\n\n git log --first-parent\n\nshould show patches because it *implies* comparing only with the\nfirst parent, but you can see why it is wrong. It is because that\nimplication is conditional---it kicks in only when the command is\ntold to compare with any parent (i.e. \"-p\"). \n\nI.e. the implication is \"compare only with the first parent if told\nto compare, and if not told what to compare with explicitly\".\n"},{"id":"402935","messageId":"87d045nfju.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqeeoloz74.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T17:55:17Z","receivedAt":"2020-08-05T17:58:46Z","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>> Jeff King writes:\n>> ...\n>> In this case your original test:\n>>\n>> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master\n>>\n>> would fail, as implied --diff-merges=1 then wins.\n>\n> IMHO, I think this is an absolutely wrong thing to do. At least to\n> me (and I suspect it would be to many users), what \"--first-parent\n> implies 'when showing a diff, compare only with the first parent'\"\n> means is that it should do so unless told to do otherwise.\n>\n> git log --no-diff-merges -p --first-parent\n>\n> explicitly tells the command that the user does not want to see\n> patches for merge commits. I do not see any reason why\n> \"--first-parent\", which merely *implies* a specific diff generation\n> preference for merges, countermand it. IOW the implication is\n> conditional.\n>\n> It is like saying\n>\n> git log --first-parent\n>\n> should show patches because it *implies* comparing only with the\n> first parent, but you can see why it is wrong. It is because that\n> implication is conditional---it kicks in only when the command is\n> told to compare with any parent (i.e. \"-p\"). \n>\n> I.e. the implication is \"compare only with the first parent if told\n> to compare, and if not told what to compare with explicitly\".\n\nI believe my approach is more straightforward and is free from\ninterpretations.\n\nTo make my point let's get back to the subject (for a moment :-)).\n\nTo me \"--first-parent implies -m\" is simple and unambiguous:\n\n(git log [*] --first-parent [*]) == (git log [*] --first-parent -m [*])\n\nNo further explanations are needed.\n\nThe consequence is that an option that is supposed to override -m must\nfollow -m, and thus --first-parent, not precede it, period.\n\nYes, we can invent the rule that implied options don't participate in\noverriding of explicit options, or that explicit option always overrides\nall the implicit, or some such, but I see absolutely no reason to\ncomplicate the model.\n\nThanks,\n-- Sergey\n"},{"id":"402940","messageId":"xmqqsgd0opzd.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87d045nfju.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T19:24:38Z","receivedAt":"2020-08-05T19:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> Yes, we can invent the rule that implied options don't ...\n\n\"invent\"? It is nothing new, isn't it? IIRC, Peff's \"first-parent\nimplies 'm' but can be countermanded with --no-diff-merges\" defines\n\"implication\" exactly that way. I do not think that is a recent\ninvention but it is just following the patterns set by other options\nthat has conditional implications.\n\nIOW,\n\n\t$ git log --no-diff-merges --first-parent -p next\n\t$ git log --first-parent -p --no-diff-merges next\n\nshould both mean the same thing. The user said no patch is wanted\nfor merge commits with --no-diff-merges and --first-parent does not\naffect it.\n\n\n"},{"id":"402944","messageId":"87k0ydp0hc.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200804195830.GA2014743@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T15:37:51Z","receivedAt":"2020-08-05T19:49:09Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:\n>\n>> Attached is rather minimal incompatible change to --diff-merges that'd\n>> allow extensions in the future, to get out of urge for the discussed\n>> changes. I'm going to follow-up with actual improvements and I'm aware\n>> it lacks documentation changes.\n>\n> Thanks, I like the direction here. Definitely it would need\n> documentation, but also tests (probably in t4013 alongside the ones my\n> series added; in fact you'd probably need to adjust my tests for the\n> non-optional argument).\n\nI turned to tests, and found that I have a doubt about the test\nyou've added:\n\ngit log --no-diff-merges -p --first-parent master\n\nIn modified tests, I'd like to move --no-diff-merges to the end, for the\ntest to be less restrictive:\n\ngit log -p --first-parent --no-diff-merges master\n\nIt should change nothing for now, but it will allow us in the future to\nget rid of mutual dependencies between in -m and --first-parent in favor\nof --first-parent to imply --diff-merges=1. We then will need to\noverride the latter by subsequent --no-diff-merges:\n\ngit log -p --first-parent [--diff-merges=1: implied] --no-diff-merges master\n\nIn this case your original test:\n\ngit log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master\n\nwould fail, as implied --diff-merges=1 then wins.\n\nThen I'm going to add a copy:\n\ngit log -p --first-parent --diff-merges=off master\n\nto check that this form works as well.\n\nWhat do you think?\n\nThanks,\n-- Sergey\n"},{"id":"402958","messageId":"877ducltxd.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqsgd0opzd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-05T20:27:42Z","receivedAt":"2020-08-05T20:27:49Z","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>> Yes, we can invent the rule that implied options don't ...\n>\n> \"invent\"? It is nothing new, isn't it? IIRC, Peff's \"first-parent\n> implies 'm' but can be countermanded with --no-diff-merges\" defines\n> \"implication\" exactly that way. I do not think that is a recent\n> invention but it is just following the patterns set by other options\n> that has conditional implications.\n>\n> IOW,\n>\n> \t$ git log --no-diff-merges --first-parent -p next\n> \t$ git log --first-parent -p --no-diff-merges next\n>\n> should both mean the same thing. The user said no patch is wanted\n> for merge commits with --no-diff-merges and --first-parent does not\n> affect it.\n\nI disagree, but I drop the issue for now for the goal of making sensible\nprogress with --diff-merges. I'll do the patches without this\nmodifications of the tests.\n\nThanks,\n-- Sergey.\n"},{"id":"403123","messageId":"20200807082643.GA34012@coredump.intra.peff.net","threadId":"53938","inReplyTo":"xmqqpn86qb6a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T08:26:43Z","receivedAt":"2020-08-07T08:26:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote:\n\n> > I'm just\n> > raising the issue now because we'll be locked into the semantics of this\n> > option, which may not be able to express the full set of what's possible\n> > (so we'd be stuck adding another option later).\n> \n> Yeah, but a good thing is that we won't have to worry about this\n> until much later, as long as we would just be introducing \"diff\n> against no parents\" and nothing else (or together with \"diff against\n> all parents\", which would make it easier to explain \"-m\").\n\nAgreed. My only question is whether the possibility of later having\nthose other options might influence how we name the two options we add\nnow. I think it's clear to all of us in this thread how those two easy\noptions should behave, but if the intent is to eventually allow these to\nbe mutually exclusive:\n\n - no diff\n - combined\n - dense combined\n - individual diff against each parent\n\nbut orthogonal to the selection of the parent-set (none, all, or\nselected ones) then e.g. \"all\" makes less sense for \"individual diff\nagainst each parent\". I don't have a good succinct name suggestion,\nthough.\n\nTBH, I would be happy enough with any of the suggestions in the thread,\nso I am really just finishing the thought here, and not trying to derail\nprogress. :)\n\n-Peff\n"},{"id":"403127","messageId":"87k0yau7sz.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200807082643.GA34012@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T09:25:16Z","receivedAt":"2020-08-07T09:25:23Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote:\n>\n>> > I'm just\n>> > raising the issue now because we'll be locked into the semantics of this\n>> > option, which may not be able to express the full set of what's possible\n>> > (so we'd be stuck adding another option later).\n>> \n>> Yeah, but a good thing is that we won't have to worry about this\n>> until much later, as long as we would just be introducing \"diff\n>> against no parents\" and nothing else (or together with \"diff against\n>> all parents\", which would make it easier to explain \"-m\").\n>\n> Agreed. My only question is whether the possibility of later having\n> those other options might influence how we name the two options we add\n> now. I think it's clear to all of us in this thread how those two easy\n> options should behave, but if the intent is to eventually allow these to\n> be mutually exclusive:\n>\n> - no diff\n> - combined\n> - dense combined\n> - individual diff against each parent\n>\n> but orthogonal to the selection of the parent-set (none, all, or\n> selected ones) then e.g. \"all\" makes less sense for \"individual diff\n> against each parent\". I don't have a good succinct name suggestion,\n> though.\n\nI have \"split\" and \"separate\" in mind, the latter likely shortened to\n\"sep\".\n\nOverall:\n\n--diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n\n-- Sergey\n"},{"id":"403137","messageId":"xmqqlfiqgxn7.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"20200807082643.GA34012@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T17:43:08Z","receivedAt":"2020-08-07T17:43:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Agreed. My only question is whether the possibility of later having\n> those other options might influence how we name the two options we add\n> now. I think it's clear to all of us in this thread how those two easy\n> options should behave, but if the intent is to eventually allow these to\n> be mutually exclusive:\n>\n> - no diff\n> - combined\n> - dense combined\n> - individual diff against each parent\n>\n> but orthogonal to the selection of the parent-set (none, all, or\n> selected ones) then e.g. \"all\" makes less sense for \"individual diff\n> against each parent\". I don't have a good succinct name suggestion,\n> though.\n>\n> TBH, I would be happy enough with any of the suggestions in the thread,\n> so I am really just finishing the thought here, and not trying to derail\n> progress. :)\n\nI agree in principle that the above is a good framework to think\nabout the issue around \"what to do with diff when showing a merge\ncommit\", but I suspect that overly spending our effort to cover the\npossibilities become mostly useless mental exercise, mostly because\n(1) comparing with second parent is mostly useful only when the\nmerge was done in the wrong direction (i.e. an attempt by a leaf\ncontributor to \"catch up to the trunc\"), (2) octopus merges are rare\ncuriosity and discouraged due to bisect efficiency anyway, and (3)\neven when looking at an octopus merge, omitting some and using only\na few selected parents to view with --cc/-c has dubious usefulness,\nas the postimage has to show contributions from all the parents\nplus \"evil\" adjustment anyway (iow, the primary effect of omitting\nparents while viewing --cc/-c is to make it fuzzy which part of\napparently \"evil\" adjustment is what the merge did vs what the\nhidden parents did). These are all examples that show not all the\ncombinations are useful.\n\nSo...\n"},{"id":"403138","messageId":"87364ymji7.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqlfiqgxn7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T17:52:00Z","receivedAt":"2020-08-07T17:52:06Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Agreed. My only question is whether the possibility of later having\n>> those other options might influence how we name the two options we add\n>> now. I think it's clear to all of us in this thread how those two easy\n>> options should behave, but if the intent is to eventually allow these to\n>> be mutually exclusive:\n>>\n>> - no diff\n>> - combined\n>> - dense combined\n>> - individual diff against each parent\n>>\n>> but orthogonal to the selection of the parent-set (none, all, or\n>> selected ones) then e.g. \"all\" makes less sense for \"individual diff\n>> against each parent\". I don't have a good succinct name suggestion,\n>> though.\n>>\n>> TBH, I would be happy enough with any of the suggestions in the thread,\n>> so I am really just finishing the thought here, and not trying to derail\n>> progress. :)\n>\n> I agree in principle that the above is a good framework to think\n> about the issue around \"what to do with diff when showing a merge\n> commit\", but I suspect that overly spending our effort to cover the\n> possibilities become mostly useless mental exercise, mostly because\n> (1) comparing with second parent is mostly useful only when the\n> merge was done in the wrong direction (i.e. an attempt by a leaf\n> contributor to \"catch up to the trunc\"), (2) octopus merges are rare\n> curiosity and discouraged due to bisect efficiency anyway, and (3)\n> even when looking at an octopus merge, omitting some and using only\n> a few selected parents to view with --cc/-c has dubious usefulness,\n> as the postimage has to show contributions from all the parents\n> plus \"evil\" adjustment anyway (iow, the primary effect of omitting\n> parents while viewing --cc/-c is to make it fuzzy which part of\n> apparently \"evil\" adjustment is what the merge did vs what the\n> hidden parents did). These are all examples that show not all the\n> combinations are useful.\n>\n> So...\n\nSo, does\n\n--diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n\nmake sense as covering all the current features?\n\nI've put variations that came to my mind. Probably we'd better just select\none for each case.\n\nThanks,\n-- Sergey\n"},{"id":"403141","messageId":"xmqqh7tegu0m.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87364ymji7.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T19:01:29Z","receivedAt":"2020-08-07T19:01:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n>> I agree in principle that the above is a good framework to think\n>> about the issue around \"what to do with diff when showing a merge\n>> commit\", but I suspect that overly spending our effort to cover the\n>> possibilities become mostly useless mental exercise, mostly because\n>> (1) comparing with second parent is mostly useful only when the\n>> merge was done in the wrong direction (i.e. an attempt by a leaf\n>> contributor to \"catch up to the trunc\"), (2) octopus merges are rare\n>> curiosity and discouraged due to bisect efficiency anyway, and (3)\n>> even when looking at an octopus merge, omitting some and using only\n>> a few selected parents to view with --cc/-c has dubious usefulness,\n>> as the postimage has to show contributions from all the parents\n>> plus \"evil\" adjustment anyway (iow, the primary effect of omitting\n>> parents while viewing --cc/-c is to make it fuzzy which part of\n>> apparently \"evil\" adjustment is what the merge did vs what the\n>> hidden parents did). These are all examples that show not all the\n>> combinations are useful.\n>>\n>> So...\n>\n> So, does\n>\n> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n>\n> make sense as covering all the current features?\n\nIf we are primarily interested in theoretical completeness, it may.\nIf we are interested more in practical usefulness, I am not sure if\nsuch a \"full flexibility\" matrix is a good way to present the\nfeature to the end-users.\n"},{"id":"403142","messageId":"87tuxel17m.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqh7tegu0m.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T19:12:29Z","receivedAt":"2020-08-07T19:12:35Z","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>>> I agree in principle that the above is a good framework to think\n>>> about the issue around \"what to do with diff when showing a merge\n>>> commit\", but I suspect that overly spending our effort to cover the\n>>> possibilities become mostly useless mental exercise, mostly because\n>>> (1) comparing with second parent is mostly useful only when the\n>>> merge was done in the wrong direction (i.e. an attempt by a leaf\n>>> contributor to \"catch up to the trunc\"), (2) octopus merges are rare\n>>> curiosity and discouraged due to bisect efficiency anyway, and (3)\n>>> even when looking at an octopus merge, omitting some and using only\n>>> a few selected parents to view with --cc/-c has dubious usefulness,\n>>> as the postimage has to show contributions from all the parents\n>>> plus \"evil\" adjustment anyway (iow, the primary effect of omitting\n>>> parents while viewing --cc/-c is to make it fuzzy which part of\n>>> apparently \"evil\" adjustment is what the merge did vs what the\n>>> hidden parents did). These are all examples that show not all the\n>>> combinations are useful.\n>>>\n>>> So...\n>>\n>> So, does\n>>\n>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n>>\n>> make sense as covering all the current features?\n>\n> If we are primarily interested in theoretical completeness, it may.\n> If we are interested more in practical usefulness, I am not sure if\n> such a \"full flexibility\" matrix is a good way to present the\n> feature to the end-users.\n\nI thought it's just a -c, -cc, and -m in better wording. No any\nmatrix:\n\n-c = --diff-merges=combined\n-cc = --diff-merges=dense\n-m = --diff-merges=split\n\nJust separate mutually exclusive options assembled into one multi-value\noption, so it's explicit they are mutually exclusive. I don't see any\nmatrix here.\n\nThanks,\n-- Sergey\n"},{"id":"403143","messageId":"20200807192001.GA1308960@coredump.intra.peff.net","threadId":"53938","inReplyTo":"87tuxel17m.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-07T19:20:01Z","receivedAt":"2020-08-07T19:20:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 07, 2020 at 10:12:29PM +0300, Sergey Organov wrote:\n\n> I thought it's just a -c, -cc, and -m in better wording. No any\n> matrix:\n> \n> -c = --diff-merges=combined\n> -cc = --diff-merges=dense\n> -m = --diff-merges=split\n> \n> Just separate mutually exclusive options assembled into one multi-value\n> option, so it's explicit they are mutually exclusive. I don't see any\n> matrix here.\n\nFWIW, those names make sense to me. Coupled with \"none\" or \"off\" for\ndisabling all of them.\n\n-Peff\n"},{"id":"403145","messageId":"87imdul0h7.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"20200807192001.GA1308960@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T19:28:20Z","receivedAt":"2020-08-07T19:28:26Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 07, 2020 at 10:12:29PM +0300, Sergey Organov wrote:\n>\n>> I thought it's just a -c, -cc, and -m in better wording. No any\n>> matrix:\n>> \n>> -c = --diff-merges=combined\n>> -cc = --diff-merges=dense\n>> -m = --diff-merges=split\n>> \n>> Just separate mutually exclusive options assembled into one multi-value\n>> option, so it's explicit they are mutually exclusive. I don't see any\n>> matrix here.\n>\n> FWIW, those names make sense to me. Coupled with \"none\" or \"off\" for\n> disabling all of them.\n\nThanks, we have \"off\" along with your --no-diff-merges in already\nsubmitted patches, and I see no harm in adding \"none\" as synonym, as\nJunio seems to prefer it over \"off\".\n\nThanks,\n-- Sergey\n"},{"id":"403146","messageId":"xmqq8seqgrx3.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87tuxel17m.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T19:46:48Z","receivedAt":"2020-08-07T19:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n>>> So, does\n>>>\n>>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n>>>\n>>> make sense as covering all the current features?\n>>\n>> If we are primarily interested in theoretical completeness, it may.\n>> If we are interested more in practical usefulness, I am not sure if\n>> such a \"full flexibility\" matrix is a good way to present the\n>> feature to the end-users.\n>\n> I thought it's just a -c, -cc, and -m in better wording. No any\n> matrix:\n>\n> -c = --diff-merges=combined\n> -cc = --diff-merges=dense\n> -m = --diff-merges=split\n>\n> Just separate mutually exclusive options assembled into one multi-value\n> option, so it's explicit they are mutually exclusive. I don't see any\n> matrix here.\n\nOh, matrix comes from specifying the set of parents in a separate\nparameter. If we are not doing that, then you cannot even express\n\"when showing a merge, compare only with the first parent\", no?\n\nAnd when you add --diff-parents=1 (i.e. diff with first-parent), you\nare opening the interface up so that it can express dubious\ncombinations like --diff-merges=dense-combined --diff-parents=1,3\n(i.e. --cc but exclude the second parent as one of the preimages).\n\nYou also have a redundant combination, e.g.\n\n --diff-merges=(off,combined,dense-combined,each) --diff-parents=\"\"\n\nwould be the same as --diff-merges=off without saying which parents\nto compare with.\n\n\n\n\n"},{"id":"403148","messageId":"87a6z6kxmm.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqq8seqgrx3.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T20:29:53Z","receivedAt":"2020-08-07T20:29:58Z","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>>>> So, does\n>>>>\n>>>> --diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)\n>>>>\n>>>> make sense as covering all the current features?\n>>>\n>>> If we are primarily interested in theoretical completeness, it may.\n>>> If we are interested more in practical usefulness, I am not sure if\n>>> such a \"full flexibility\" matrix is a good way to present the\n>>> feature to the end-users.\n>>\n>> I thought it's just a -c, -cc, and -m in better wording. No any\n>> matrix:\n>>\n>> -c = --diff-merges=combined\n>> -cc = --diff-merges=dense\n>> -m = --diff-merges=split\n>>\n>> Just separate mutually exclusive options assembled into one multi-value\n>> option, so it's explicit they are mutually exclusive. I don't see any\n>> matrix here.\n>\n> Oh, matrix comes from specifying the set of parents in a separate\n> parameter. If we are not doing that, then you cannot even express\n> \"when showing a merge, compare only with the first parent\", no?\n>\n> And when you add --diff-parents=1 (i.e. diff with first-parent), you\n> are opening the interface up so that it can express dubious\n> combinations like --diff-merges=dense-combined --diff-parents=1,3\n> (i.e. --cc but exclude the second parent as one of the preimages).\n\nI had no intention to introduce --diff-parents, at least for now, and\nmaybe never. What I said about it was theoretical discussion rather than\nactual proposal.\n\nIf we agree on the above, I intended to instead propose something like:\n\n--diff-merges=first-parent or just =first\n\nfor a start.\n\nThanks,\n-- Sergey\n"},{"id":"403149","messageId":"xmqqv9hufaxc.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87a6z6kxmm.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T20:39:11Z","receivedAt":"2020-08-07T20:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> I had no intention to introduce --diff-parents, at least for now, and\n> maybe never. What I said about it was theoretical discussion rather than\n> actual proposal.\n>\n> If we agree on the above, I intended to instead propose something like:\n>\n> --diff-merges=first-parent or just =first\n\nOK, so the combined, combined-dense and split were meant to work\nwith all the parents, and off is the only one that means comparison\nwith no parents. That makes sense.\n\n"},{"id":"403154","messageId":"87y2mqjh9f.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqv9hufaxc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T21:08:44Z","receivedAt":"2020-08-07T21:08:50Z","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>> I had no intention to introduce --diff-parents, at least for now, and\n>> maybe never. What I said about it was theoretical discussion rather than\n>> actual proposal.\n>>\n>> If we agree on the above, I intended to instead propose something like:\n>>\n>> --diff-merges=first-parent or just =first\n>\n> OK, so the combined, combined-dense and split were meant to work\n> with all the parents, and off is the only one that means comparison\n> with no parents. That makes sense.\n\nYeah, exactly, thanks!\n\nThe only question regarding it I then have for now is what are\npreferences for names selection inside single option? Abbreviated yet\nsomewhat sensible, or verbose? I mean:\n\n--diff-merges=first vs --diff-merges=first-parent\n\n--diff-merges=comb vs --diff-merges=combined\n\netc. What's better?\n\nThanks,\n-- Sergey\n"},{"id":"403161","messageId":"xmqqft8yf7gj.fsf@gitster.c.googlers.com","threadId":"53938","inReplyTo":"87y2mqjh9f.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T21:54:04Z","receivedAt":"2020-08-07T21:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov writes:\n\n> The only question regarding it I then have for now is what are\n> preferences for names selection inside single option? Abbreviated yet\n> somewhat sensible, or verbose? I mean:\n>\n> --diff-merges=first vs --diff-merges=first-parent\n>\n> --diff-merges=comb vs --diff-merges=combined\n>\n> etc. What's better?\n\nIf we were to shoot for easy-to-type, we could go for ultra-short\nabbreviations like 'no', 'c', 'cc', 'each' (the last one is the\ntraditional \"-m\" when used without \"--first-parent\"; diff with each\nparent) and later add 'fp', but in a sense we are already lost the\neasy-to-type goal by \"--diff-merges\" being sufficiently long.\n\nI personally wouldn't choose \"first\" or \"first-parent\", but just use\n\"1\", so that we could support \"2\" when viewing a merge that was done\nin the wrong direction with \"git show\", though. IOW, even though I\nsaid that \"use these parents but not those\" (i.e. set of parents)\nsmells overkill, at least to me, I think specifying a single parent\nthat is not necessarily the first one would be a reasonable thing to\ndo.\n\nSo, if I were to vote, it would be\n\n \"--diff-merges=\" ( first-parent | 1 |\n\t\t combined | c |\n dense-combined | cc |\n each-parent | m )\n \"--no-diff-merges\"\n\nleaving some room to add '2' and later.\n"},{"id":"403177","messageId":"87imdujbrz.fsf@osv.gnss.ru","threadId":"53938","inReplyTo":"xmqqft8yf7gj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/7] making log --first-parent imply -m","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-07T23:07:12Z","receivedAt":"2020-08-07T23:07:21Z","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 only question regarding it I then have for now is what are\n>> preferences for names selection inside single option? Abbreviated yet\n>> somewhat sensible, or verbose? I mean:\n>>\n>> --diff-merges=first vs --diff-merges=first-parent\n>>\n>> --diff-merges=comb vs --diff-merges=combined\n>>\n>> etc. What's better?\n>\n> If we were to shoot for easy-to-type, we could go for ultra-short\n> abbreviations like 'no', 'c', 'cc', 'each' (the last one is the\n> traditional \"-m\" when used without \"--first-parent\"; diff with each\n> parent) and later add 'fp', but in a sense we are already lost the\n> easy-to-type goal by \"--diff-merges\" being sufficiently long.\n\nDo I get it right that there are no common guidelines and every case is\nto be considered separately?\n\nAnyway:\n\n$ git log -d\nfatal: unrecognized argument: -d\n$ git show -d\nfatal: unrecognized argument: -d\n\nso it seems we still have -d to (ab)use for, say \"-d 1\" or \"-d m\", if we\ndecide to.\n\n>\n> I personally wouldn't choose \"first\" or \"first-parent\", but just use\n> \"1\", so that we could support \"2\" when viewing a merge that was done\n> in the wrong direction with \"git show\", though.\n\nI fail to see how using \"first-parent\" would deny using a number either\nlater or right now, though based on your own rating of octopus merges\nwith which I whole-heartedly agree, the only thing we'd ever need seems\nto be \"second-parent\", or 2.\n\n> IOW, even though I said that \"use these parents but not those\" (i.e.\n> set of parents) smells overkill, at least to me, I think specifying a\n> single parent that is not necessarily the first one would be a\n> reasonable thing to do.\n\nThe second parent, I'd agree. Others? Well, that's more a completeness\ntheoretical issue to me rather than practical one. More relevant to\nplumbing than to porcelain thereof. And, as additional safety, we still\nhave that \"all|every|each|split\" -m that will show needed diff among\nothers.\n\n>\n> So, if I were to vote, it would be\n>\n> \"--diff-merges=\" ( first-parent | 1 |\n> \t\t combined | c |\n> dense-combined | cc |\n> each-parent | m )\n> \"--no-diff-merges\"\n>\n> leaving some room to add '2' and later.\n\nThanks for voting!\n\nI believe we still have the room for digits even if we use first-parent,\nand then won't\n\n \"--first-parent implies --diff-merges=first-parent\"\n\nsound really cool?\n\nOverall, I incline to support short (traditional and numbered) variants\nalong with new longer spellings at the same time, similar to short and\nlong options.\n\nThanks,\n-- Sergey\n"}]}