{"thread":{"id":"54157","subject":"[PATCH] revision: add separate field for \"-m\" of \"diff-index -m\"","startedAt":"2020-08-29T19:48:29Z","lastAt":"2020-10-23T04:03:25Z","messageCount":12,"participants":["Sergey Organov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404760","messageId":"20200829194634.23306-1-sorganov@gmail.com","threadId":"54157","inReplyTo":null,"subject":"[PATCH] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-29T19:46:34Z","receivedAt":"2020-08-29T19:48:29Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Historically, in \"diff-index -m\", \"-m\" does not mean \"do not ignore merges\", but\n\"match missing\". Despite this, diff-index abuses 'ignore_merges' field being set\nby \"-m\", that in turn causes more troubles.\n\nAdd separate 'diff_index_match_missing' field for diff-index to use and set it\nwhen we encounter \"-m\" option. This field won't then be cleared when primary\nmeaning of \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be\naffected by future option(s) that might drive 'ignore_merges' field.\n\nUse this new field from diff-lib:do_oneway_diff() instead of abusing\n'ignore_merges' field.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n diff-lib.c | 10 ++--------\n revision.c |  6 ++++++\n revision.h |  1 +\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 25fd2dee19c4..8c40111b5f35 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -404,14 +404,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n-\t/*\n-\t * Backward compatibility wart - \"diff-index -m\" does\n-\t * not mean \"do not ignore merges\", but \"match_missing\".\n-\t *\n-\t * But with the revision flag parsing, that's found in\n-\t * \"!revs->ignore_merges\".\n-\t */\n-\tmatch_missing = !revs->ignore_merges;\n+\n+\tmatch_missing = revs->diff_index_match_missing;\n \n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\ndiff --git a/revision.c b/revision.c\nindex 6aa7f4f56755..95f9cfddb02c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2325,6 +2325,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diffopt.flags.tree_in_recursive = 1;\n \t} else if (!strcmp(arg, \"-m\")) {\n \t\trevs->ignore_merges = 0;\n+\t\t/*\n+\t\t * Backward compatibility wart - \"diff-index -m\" does\n+\t\t * not mean \"do not ignore merges\", but \"match_missing\",\n+\t\t * so set separate flag for it.\n+\t\t */\n+\t\trevs->diff_index_match_missing = 1;\n \t} else if (!strcmp(arg, \"-c\")) {\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 0;\ndiff --git a/revision.h b/revision.h\nindex f412ae85ebaf..979dddbdaf7c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -188,6 +188,7 @@ struct rev_info {\n \tunsigned int\tdiff:1,\n \t\t\tfull_diff:1,\n \t\t\tshow_root_diff:1,\n+\t\t\tdiff_index_match_missing:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n \t\t\tignore_merges:1,\n-- \n2.25.1\n\n"},{"id":"404761","messageId":"20200829201140.23425-1-sorganov@gmail.com","threadId":"54157","inReplyTo":"20200829194634.23306-1-sorganov@gmail.com","subject":"[PATCH v2] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-29T20:11:40Z","receivedAt":"2020-08-29T20:12:23Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Historically, in \"diff-index -m\", \"-m\" does not mean \"do not ignore merges\", but\n\"match missing\". Despite this, diff-index abuses 'ignore_merges' field being set\nby \"-m\", that in turn causes more troubles.\n\nAdd separate 'diff_index_match_missing' field for diff-index to use and set it\nwhen we encounter \"-m\" option. This field won't then be cleared when primary\nmeaning of \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be\naffected by future option(s) that might drive 'ignore_merges' field.\n\nUse this new field from diff-lib:do_oneway_diff() instead of abusing\n'ignore_merges' field.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n\nv2: rebased from 'maint' onto 'master'\n\n diff-lib.c | 10 ++--------\n revision.c |  6 ++++++\n revision.h |  1 +\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 50521e2093fc..f2aee78e7aa2 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -405,14 +405,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n-\t/*\n-\t * Backward compatibility wart - \"diff-index -m\" does\n-\t * not mean \"do not ignore merges\", but \"match_missing\".\n-\t *\n-\t * But with the revision flag parsing, that's found in\n-\t * \"!revs->ignore_merges\".\n-\t */\n-\tmatch_missing = !revs->ignore_merges;\n+\n+\tmatch_missing = revs->diff_index_match_missing;\n \n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\ndiff --git a/revision.c b/revision.c\nindex 96630e31867d..64b16f7d1033 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2345,6 +2345,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diffopt.flags.tree_in_recursive = 1;\n \t} else if (!strcmp(arg, \"-m\")) {\n \t\trevs->ignore_merges = 0;\n+\t\t/*\n+\t\t * Backward compatibility wart - \"diff-index -m\" does\n+\t\t * not mean \"do not ignore merges\", but \"match_missing\",\n+\t\t * so set separate flag for it.\n+\t\t */\n+\t\trevs->diff_index_match_missing = 1;\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;\ndiff --git a/revision.h b/revision.h\nindex c1e5bcf139d7..5ae8254ffaed 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -188,6 +188,7 @@ struct rev_info {\n \tunsigned int\tdiff:1,\n \t\t\tfull_diff:1,\n \t\t\tshow_root_diff:1,\n+\t\t\tdiff_index_match_missing:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n \t\t\tcombine_merges:1,\n-- \n2.25.1\n\n"},{"id":"404782","messageId":"xmqqo8mrh12b.fsf@gitster.c.googlers.com","threadId":"54157","inReplyTo":"20200829201140.23425-1-sorganov@gmail.com","subject":"Re: [PATCH v2] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T04:49:32Z","receivedAt":"2020-08-31T04:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Historically, in \"diff-index -m\", \"-m\" does not mean \"do not ignore merges\", but\n> \"match missing\". Despite this, diff-index abuses 'ignore_merges' field being set\n> by \"-m\", that in turn causes more troubles.\n\n\"causes more troubles\"?  When there is no trouble, and no \"more\"\ntrouble, concretely mentioned, it is a quite weak justfiication.\n\nThere is no reason to say \"historically\" here, as it has been like\nso from beginning of the time, it still is so and it is relied\nupon.  \"diff-{files,index,tree}\" are about comparing two things, and\nnot about history (where a \"merge\" might influence \"now we are\nshowing this commit.  which parent do we compare it with?\"), so\ngiving short-and-sweet \"-m\" its own meaning that is sensible within\nthe context of \"diff\" was and is perfectly sensible thing to do.\n\nWhat is worth fixing is not \"-m\" in diff-index means \"match missing\"\nwhile \"-m\" in log wants to mean \"show merges\".  It is that, even both\ncommands use the same option parsing machinery, and the use of these\ntwo options are mutually exclusive so there is no risk of confusion,\nthe flag internally used to record the presense of the \"em\" option is\nnot named neutrally (e.g. \"revs->seen_em_option\").\n\n\tThe \"log\" family of commands and \"diff\" family of commands\n\tshare the same command line parsiong machinery.  For the\n\tformer, \"-m\" means \"show merges\" while for the latter it\n\tmeans \"match missing\".  Tnis is not a problem at the UI\n\tlevel, as \"show/not show merges\" is meaningless in the\n\tcontext of \"diff\", and similarly \"match/not match missing\"\n\tis meaningless in the context of \"log\".\n\n\tBut there are two problems with this arrangement.\n\n\t1. the field the presense of the option on the command line\n\t   is recorded in has to be given a name.  It is currently\n\t   called \"ignore_merges\", which gives an incorrect\n\t   impression that using it for \"diff\" family is somehow a\n\t   mistake, and renaming it to \"match_missing\" would not be\n\t   a solution, as it will give an incorrect impression that\n\t   \"log\" family is abusing it.  However, naming the field to\n\t   something neutral, e.g. \"em_option\", would make the code\n\t   harder to understand.\n\n\t2. because it uses the same command line parser, giving a\n    \t   default for \"diff -m\" in a way that is different from the\n    \t   default for \"log -m\" is quite cumbersome if they use the\n    \t   same field to record it.\n\n\tIntroduce a separate \"match_missing\" field, and flip it and\n\t\"ignore_merges\" when we see the \"-m\" option on the command\n\tline.  That way, even when ignore_merges's default is\n\taffected by end-user configuration, the default for\n\t\"match_missing\" would not be affected.\n\nI think the above would be in line with what you wanted to say but\ndidn't, and I think it supports the split fairly well.\n\nI have a very strong objection against changing the built-in default\nof \"log -m\", but I do agree that this split of the single field into\ntwo is a fairly good idea.  So I do not want to be in the position\nthat must reject this change because \"log -m\" and \"diff-index -m\"\nwill never be on by default.  Basing the justification of this\nchange on end-user configurability would be a good way to sidestep\nthe issue, and avoids taking this change hostage to the discussion\non what should be the built-in default for \"log/diff-index -m\".\n"},{"id":"404811","messageId":"87a6ybugpe.fsf@osv.gnss.ru","threadId":"54157","inReplyTo":"xmqqo8mrh12b.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-31T12:45:33Z","receivedAt":"2020-08-31T12:45:43Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Historically, in \"diff-index -m\", \"-m\" does not mean \"do not ignore\n>> merges\", but\n>> \"match missing\". Despite this, diff-index abuses 'ignore_merges'\n>> field being set\n>> by \"-m\", that in turn causes more troubles.\n>\n> \"causes more troubles\"?  When there is no trouble, and no \"more\"\n> trouble, concretely mentioned, it is a quite weak justfiication.\n\nWell, existed comment says \"Backward compatibility wart\" that sounds\nlike a trouble to me already. No?\n\nThen, since \"--[no-]diff-merges\" is introduced, we have:\n\n$ git diff-index HEAD\n:100644 000000 4aec621a6d1a9a5892f0b4b6feb2ed329fd04bf2 0000000000000000000000000000000000000000 D\tmain/main.cc\n$ git diff-index -m HEAD\n$ git diff-index -m --no-diff-merges HEAD\n:100644 000000 4aec621a6d1a9a5892f0b4b6feb2ed329fd04bf2 0000000000000000000000000000000000000000 D\tmain/main.cc\n\nthat sounds like yet another trouble. That's why I used \"more trouble\" in my\ncommit message.\n\nIf you say \"compatibility wart\" is not a trouble by itself, -- I'm fine\nwith it, -- then \"more\" in my commit message is misplaced indeed.\n\n>\n> There is no reason to say \"historically\" here, as it has been like\n> so from beginning of the time, it still is so and it is relied\n> upon.  \"diff-{files,index,tree}\" are about comparing two things, and\n> not about history (where a \"merge\" might influence \"now we are\n> showing this commit.  which parent do we compare it with?\"), so\n> giving short-and-sweet \"-m\" its own meaning that is sensible within\n> the context of \"diff\" was and is perfectly sensible thing to do.\n\nWell, if \"historically\" makes you feel uncomfortable, -- I'm willing to\nget rid of it.\n\n>\n> What is worth fixing is not \"-m\" in diff-index means \"match missing\"\n> while \"-m\" in log wants to mean \"show merges\".  It is that, even both\n> commands use the same option parsing machinery, and the use of these\n> two options are mutually exclusive so there is no risk of confusion,\n> the flag internally used to record the presense of the \"em\" option is\n> not named neutrally (e.g. \"revs->seen_em_option\").\n>\n> \tThe \"log\" family of commands and \"diff\" family of commands\n> \tshare the same command line parsiong machinery.  For the\n> \tformer, \"-m\" means \"show merges\" while for the latter it\n> \tmeans \"match missing\".  Tnis is not a problem at the UI\n> \tlevel, as \"show/not show merges\" is meaningless in the\n> \tcontext of \"diff\", and similarly \"match/not match missing\"\n> \tis meaningless in the context of \"log\".\n>\n> \tBut there are two problems with this arrangement.\n>\n> \t1. the field the presense of the option on the command line\n> \t   is recorded in has to be given a name.  It is currently\n> \t   called \"ignore_merges\", which gives an incorrect\n> \t   impression that using it for \"diff\" family is somehow a\n> \t   mistake, and renaming it to \"match_missing\" would not be\n> \t   a solution, as it will give an incorrect impression that\n> \t   \"log\" family is abusing it.  However, naming the field to\n> \t   something neutral, e.g. \"em_option\", would make the code\n> \t   harder to understand.\n>\n> \t2. because it uses the same command line parser, giving a\n>     \t   default for \"diff -m\" in a way that is different from the\n>     \t   default for \"log -m\" is quite cumbersome if they use the\n>     \t   same field to record it.\n>\n> \tIntroduce a separate \"match_missing\" field, and flip it and\n> \t\"ignore_merges\" when we see the \"-m\" option on the command\n> \tline.  That way, even when ignore_merges's default is\n> \taffected by end-user configuration, the default for\n> \t\"match_missing\" would not be affected.\n>\n> I think the above would be in line with what you wanted to say but\n> didn't, and I think it supports the split fairly well.\n>\n> I have a very strong objection against changing the built-in default\n> of \"log -m\", but I do agree that this split of the single field into\n> two is a fairly good idea.  So I do not want to be in the position\n> that must reject this change because \"log -m\" and \"diff-index -m\"\n> will never be on by default.  Basing the justification of this\n> change on end-user configurability would be a good way to sidestep\n> the issue, and avoids taking this change hostage to the discussion\n> on what should be the built-in default for \"log/diff-index -m\".\n\nThis change has nothing to do with defaults. It rather about correct and\nclear code.\n\nI'll re-roll with better commit message.\n\nThanks,\n-- Sergey\n"},{"id":"404812","messageId":"20200831125350.26472-1-sorganov@gmail.com","threadId":"54157","inReplyTo":"20200829194634.23306-1-sorganov@gmail.com","subject":"[PATCH v3] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-31T12:53:50Z","receivedAt":"2020-08-31T12:54:22Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Add separate 'diff_index_match_missing' field for diff-index to use and set it\nwhen we encounter \"-m\" option. This field won't then be cleared when another\nmeaning of \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be\naffected by future option(s) that might drive 'ignore_merges' field.\n\nUse this new field from diff-lib:do_oneway_diff() instead of reusing\n'ignore_merges' field.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n\nv3: improve commit message\n\nv2: rebased from 'maint' onto 'master'\n\n diff-lib.c | 10 ++--------\n revision.c |  6 ++++++\n revision.h |  1 +\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 50521e2093fc..f2aee78e7aa2 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -405,14 +405,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n-\t/*\n-\t * Backward compatibility wart - \"diff-index -m\" does\n-\t * not mean \"do not ignore merges\", but \"match_missing\".\n-\t *\n-\t * But with the revision flag parsing, that's found in\n-\t * \"!revs->ignore_merges\".\n-\t */\n-\tmatch_missing = !revs->ignore_merges;\n+\n+\tmatch_missing = revs->diff_index_match_missing;\n \n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\ndiff --git a/revision.c b/revision.c\nindex 96630e31867d..64b16f7d1033 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2345,6 +2345,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diffopt.flags.tree_in_recursive = 1;\n \t} else if (!strcmp(arg, \"-m\")) {\n \t\trevs->ignore_merges = 0;\n+\t\t/*\n+\t\t * Backward compatibility wart - \"diff-index -m\" does\n+\t\t * not mean \"do not ignore merges\", but \"match_missing\",\n+\t\t * so set separate flag for it.\n+\t\t */\n+\t\trevs->diff_index_match_missing = 1;\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;\ndiff --git a/revision.h b/revision.h\nindex c1e5bcf139d7..5ae8254ffaed 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -188,6 +188,7 @@ struct rev_info {\n \tunsigned int\tdiff:1,\n \t\t\tfull_diff:1,\n \t\t\tshow_root_diff:1,\n+\t\t\tdiff_index_match_missing:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n \t\t\tcombine_merges:1,\n-- \n2.25.1\n\n"},{"id":"404816","messageId":"xmqqd036hhs2.fsf@gitster.c.googlers.com","threadId":"54157","inReplyTo":"87a6ybugpe.fsf@osv.gnss.ru","subject":"Re: [PATCH v2] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T17:00:45Z","receivedAt":"2020-08-31T17:00:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> $ git diff-index -m --no-diff-merges HEAD\n> :100644 000000 4aec621a6d1a9a5892f0b4b6feb2ed329fd04bf2 0000000000000000000000000000000000000000 D\tmain/main.cc\n\nAt the first glance, this looked like a good justification for this\npatch.\n\n> If you say \"compatibility wart\" is not a trouble by itself, -- I'm fine\n> with it, -- then \"more\" in my commit message is misplaced indeed.\n\nYeah, when I wrote the \"compatibility wart\" comment originally, I\nwas describing \"this needs a tricky code because two independent\noptions happen to share the command line parser\" and nothing more.\n\nI was not reacting to \"more\", by the way.  I was reacting the lack\nof concrete problem description.  \"A '-m' option given to the\n'diff-index' command can be defeated by giving '--no-diff-merges'\nlater\" you showed above can be a good replacement for \"causes more\ntroubles\".\n\nBut in the ideal world, \"--[no-]diff-merges\" should be rejected as\nan irrelevant/unrecognised option to the \"diff\" family of commands\n(as I said in the message you are responding to, it is only relevant\nto the \"log\" family of commands where the diff machinery is solely\nto compare between (some of) its parents and in that context, what,\nif anything, kind of special treatment is made for merge commits\nmakes sense as an optional instruction to the command).  Splitting\nthe field into two fields, setting both fields upon \"-m\" but\ntoggling only one with longhand \"--[no-]diff-merges\" would allow the\ncode to notice and make the above command line silently turn the\n\"--[no-]diff-merges\" into a no-op, so in that sense it would be a\ngood first step, but an ideal solution would probably need to know\nif we are parsing for the \"log\" family or for the \"diff\" family and\nerror out upon seeing a \"log\"-only option like \"--[no-]diff-merges\"\nwhen checking the command line option for \"diff\".\n\n> This change has nothing to do with defaults. It rather about correct and\n> clear code.\n\nOK, I misread your intention.  Sorry about that.\n\nThanks.\n\n"},{"id":"404819","messageId":"xmqq4koihgpk.fsf@gitster.c.googlers.com","threadId":"54157","inReplyTo":"20200831125350.26472-1-sorganov@gmail.com","subject":"Re: [PATCH v3] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T17:23:51Z","receivedAt":"2020-08-31T17:24:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Add separate 'diff_index_match_missing' field for diff-index to use and set it\n> when we encounter \"-m\" option. This field won't then be cleared when another\n> meaning of \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be\n> affected by future option(s) that might drive 'ignore_merges' field.\n>\n> Use this new field from diff-lib:do_oneway_diff() instead of reusing\n> 'ignore_merges' field.\n>\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n\nMuch easier to reason about.  As I said, I think we would ideally\nwant to detect and diagnose --[no-]diff-merges on the command line\nof \"diff\" or \"diff-{files,index,trees}\" as an error, but for now\nthis is a good first step.\n\n>  \t} else if (!strcmp(arg, \"-m\")) {\n>  \t\trevs->ignore_merges = 0;\n> +\t\t/*\n> +\t\t * Backward compatibility wart - \"diff-index -m\" does\n> +\t\t * not mean \"do not ignore merges\", but \"match_missing\",\n> +\t\t * so set separate flag for it.\n> +\t\t */\n> +\t\trevs->diff_index_match_missing = 1;\n\nHalf the wart has been removed thanks to this patch and the rest of\nthe code can look at the right field for their purpose.  The parsing,\nunless we make a bigger change that allows us to detect and diagnose\n\"diff-index --no-diff-merges\" as an error, still needs to be tricky\nand may deserve a comment.\n\nThe comment should apply to and treat both fields equally, perhaps\nlike this:\n\n\t} else if (!strcmp(arg, \"-m\")) {\n\t\t/*\n\t\t * To \"diff-index\", \"-m\" means \"match missing\", and to\n\t\t * the \"log\" family of commands, it means \"keep merges\".\n\t\t * Set both fields appropriately.\n\t\t */\n\t\trevs->ignore_merges = 0;\n\t\trevs->match_missing = 1;\n\t}\n\nBy the way, let's drop diff_index_ prefix from the name of the new\nfield.  I do not see a strong reason to object to a possible update\nto \"diff-files\" to match the behaviour of \"diff-index\".  \n\nIn a sparsely checked out working tree (e.g. start from \"clone\n--no-checkout\"), you can check out only paths that you want to\nmodify, edit them, and then \"git diff-files -m\" would be able to\nshow useful result without having to show deletions to all other\nfiles you are not interested in.  And that is exactly the same use\ncase as \"git diff-index -m HEAD\" was invented for.\n\nThanks.\n\n> diff --git a/revision.h b/revision.h\n> index c1e5bcf139d7..5ae8254ffaed 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -188,6 +188,7 @@ struct rev_info {\n>  \tunsigned int\tdiff:1,\n>  \t\t\tfull_diff:1,\n>  \t\t\tshow_root_diff:1,\n> +\t\t\tdiff_index_match_missing:1,\n>  \t\t\tno_commit_id:1,\n>  \t\t\tverbose_header:1,\n>  \t\t\tcombine_merges:1,\n"},{"id":"404828","messageId":"87wo1eob5s.fsf@osv.gnss.ru","threadId":"54157","inReplyTo":"xmqq4koihgpk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-31T19:41:51Z","receivedAt":"2020-08-31T19:41:59Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Add separate 'diff_index_match_missing' field for diff-index to use\n>> and set it\n>> when we encounter \"-m\" option. This field won't then be cleared when another\n>> meaning of \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be\n>> affected by future option(s) that might drive 'ignore_merges' field.\n>>\n>> Use this new field from diff-lib:do_oneway_diff() instead of reusing\n>> 'ignore_merges' field.\n>>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>\n> Much easier to reason about.  As I said, I think we would ideally\n> want to detect and diagnose --[no-]diff-merges on the command line\n> of \"diff\" or \"diff-{files,index,trees}\" as an error, but for now\n> this is a good first step.\n>\n>>  \t} else if (!strcmp(arg, \"-m\")) {\n>>  \t\trevs->ignore_merges = 0;\n>> +\t\t/*\n>> +\t\t * Backward compatibility wart - \"diff-index -m\" does\n>> +\t\t * not mean \"do not ignore merges\", but \"match_missing\",\n>> +\t\t * so set separate flag for it.\n>> +\t\t */\n>> +\t\trevs->diff_index_match_missing = 1;\n>\n> Half the wart has been removed thanks to this patch and the rest of\n> the code can look at the right field for their purpose.  The parsing,\n> unless we make a bigger change that allows us to detect and diagnose\n> \"diff-index --no-diff-merges\" as an error, still needs to be tricky\n> and may deserve a comment.\n>\n> The comment should apply to and treat both fields equally, perhaps\n> like this:\n>\n> \t} else if (!strcmp(arg, \"-m\")) {\n> \t\t/*\n> \t\t * To \"diff-index\", \"-m\" means \"match missing\", and to\n> \t\t * the \"log\" family of commands, it means \"keep merges\".\n> \t\t * Set both fields appropriately.\n> \t\t */\n> \t\trevs->ignore_merges = 0;\n> \t\trevs->match_missing = 1;\n> \t}\n>\n> By the way, let's drop diff_index_ prefix from the name of the new\n> field.  I do not see a strong reason to object to a possible update\n> to \"diff-files\" to match the behaviour of \"diff-index\".  \n>\n> In a sparsely checked out working tree (e.g. start from \"clone\n> --no-checkout\"), you can check out only paths that you want to\n> modify, edit them, and then \"git diff-files -m\" would be able to\n> show useful result without having to show deletions to all other\n> files you are not interested in.  And that is exactly the same use\n> case as \"git diff-index -m HEAD\" was invented for.\n\nFine with me, thanks! I'll re-roll soon.\n\nThanks,\n-- Sergey\n"},{"id":"404829","messageId":"20200831201422.27189-1-sorganov@gmail.com","threadId":"54157","inReplyTo":"20200829194634.23306-1-sorganov@gmail.com","subject":"[PATCH v4] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-31T20:14:22Z","receivedAt":"2020-08-31T20:14:36Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Add separate 'match_missing' field for diff-index to use and set it when we\nencounter \"-m\" option. This field won't then be cleared when another meaning of\n\"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be affected by\nfuture option(s) that might drive 'ignore_merges' field.\n\nUse this new field from diff-lib:do_oneway_diff() instead of reusing\n'ignore_merges' field.\n\nSigned-off-by: Sergey Organov <sorganov@gmail.com>\n---\n\nv4: fix new field name to be 'match_missing', and improve comment in the code to\n    treat both involved bits similarily\n\nv3: improve commit message\n\nv2: rebased from 'maint' onto 'master'\n\n diff-lib.c | 10 ++--------\n revision.c |  6 ++++++\n revision.h |  1 +\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 50521e2093fc..5d5d3dafab33 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -405,14 +405,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n-\t/*\n-\t * Backward compatibility wart - \"diff-index -m\" does\n-\t * not mean \"do not ignore merges\", but \"match_missing\".\n-\t *\n-\t * But with the revision flag parsing, that's found in\n-\t * \"!revs->ignore_merges\".\n-\t */\n-\tmatch_missing = !revs->ignore_merges;\n+\n+\tmatch_missing = revs->match_missing;\n \n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\ndiff --git a/revision.c b/revision.c\nindex 96630e31867d..73e3d14cc165 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2344,7 +2344,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\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\t/*\n+\t\t * To \"diff-index\", \"-m\" means \"match missing\", and to the \"log\"\n+\t\t * family of commands, it means \"show full diff for merges\". Set\n+\t\t * both fields appropriately.\n+\t\t */\n \t\trevs->ignore_merges = 0;\n+\t\trevs->match_missing = 1;\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;\ndiff --git a/revision.h b/revision.h\nindex c1e5bcf139d7..f6bf860d19e5 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -188,6 +188,7 @@ struct rev_info {\n \tunsigned int\tdiff:1,\n \t\t\tfull_diff:1,\n \t\t\tshow_root_diff:1,\n+\t\t\tmatch_missing:1,\n \t\t\tno_commit_id:1,\n \t\t\tverbose_header:1,\n \t\t\tcombine_merges:1,\n-- \n2.25.1\n\n"},{"id":"404830","messageId":"xmqqzh6aeedr.fsf@gitster.c.googlers.com","threadId":"54157","inReplyTo":"20200831201422.27189-1-sorganov@gmail.com","subject":"Re: [PATCH v4] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T20:42:24Z","receivedAt":"2020-08-31T20:42:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Organov <sorganov@gmail.com> writes:\n\n> Add separate 'match_missing' field for diff-index to use and set it when we\n> encounter \"-m\" option. This field won't then be cleared when another meaning of\n> \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be affected by\n> future option(s) that might drive 'ignore_merges' field.\n>\n> Use this new field from diff-lib:do_oneway_diff() instead of reusing\n> 'ignore_merges' field.\n>\n> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n> ---\n\nLooks good.  Will queue.  Thanks.\n"},{"id":"404831","messageId":"878sdulecd.fsf@osv.gnss.ru","threadId":"54157","inReplyTo":"xmqqzh6aeedr.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] revision: add separate field for \"-m\" of \"diff-index -m\"","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-08-31T21:01:22Z","receivedAt":"2020-08-31T21:01:30Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Sergey Organov <sorganov@gmail.com> writes:\n>\n>> Add separate 'match_missing' field for diff-index to use and set it when we\n>> encounter \"-m\" option. This field won't then be cleared when another\n>> meaning of\n>> \"-m\" is reverted (e.g., by \"--no-diff-merges\"), nor it will be affected by\n>> future option(s) that might drive 'ignore_merges' field.\n>>\n>> Use this new field from diff-lib:do_oneway_diff() instead of reusing\n>> 'ignore_merges' field.\n>>\n>> Signed-off-by: Sergey Organov <sorganov@gmail.com>\n>> ---\n>\n> Looks good.  Will queue.  Thanks.\n\nNice! Thanks a lot!\n\n"},{"id":"408201","messageId":"xmqqd019k2rb.fsf@gitster.c.googlers.com","threadId":"54157","inReplyTo":"20200831201422.27189-1-sorganov@gmail.com","subject":"[PATCH] revision: wording tweak in comment for parsing \"-m\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-23T04:03:20Z","receivedAt":"2020-10-23T04:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We do not mean to say that the --ignore-merges (-m) option is to\nshow a patch with infinite number of context lines, but \"show full\ndiff\" can give such a wrong impression.  An alternative may be to\nsay \"show diff with each parent for merges\", but when combined with\nthe --first-parent option, the -m option does not mean that at all.\nWith these in mind, it seems that \"do not hide\" would probably be a\ngood enough compromise to clarify.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 73e3d14cc1..c34e1d0bed 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2346,8 +2346,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"-m\")) {\n \t\t/*\n \t\t * To \"diff-index\", \"-m\" means \"match missing\", and to the \"log\"\n-\t\t * family of commands, it means \"show full diff for merges\". Set\n-\t\t * both fields appropriately.\n+\t\t * family of commands, it means \"do not hide diff for merges\".\n+\t\t * Set both fields appropriately.\n \t\t */\n \t\trevs->ignore_merges = 0;\n \t\trevs->match_missing = 1;\n-- \n2.28.0-509-g776ef846e8\n\n"}]}