{"thread":{"id":"57880","subject":"Bug: combined diff with --ignore-matching-lines","startedAt":"2022-05-14T09:33:02Z","lastAt":"2022-06-21T16:56:04Z","messageCount":12,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"455285","messageId":"a6a14213-bc82-d6fb-43dd-5a423c40a4f8@web.de","threadId":"57880","inReplyTo":null,"subject":"Bug: combined diff with --ignore-matching-lines","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-05-14T09:32:31Z","receivedAt":"2022-05-14T09:33:02Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hi all,\n\ngit diff segfaults when it's asked to produce a combined diff and ignore\ncertain lines with --ignore-matching-lines/-I, e.g.:\n\n $ git diff -I DEF_VER v2.33.3 v2.33.3^@\n zsh: segmentation fault ./git-diff -I DEF_VER v2.33.3 v2.33.3^@\n\nThat's because combine-diff.c::diff_tree_combined() copies a diffopt\nwithout making a deep copy of the ignore_regex array and frees it, then\nlater tries to use it.\n\nThe segfault can be fixed by adding \"diffopt.no_free = 1;\" or reverting\nc45dc9cf30 (diff: plug memory leak from regcomp() on {log,diff} -I,\n2021-02-11).\n\nBut even with that the only thing the command ignores is the option -I;\nthe GIT-VERSION-GEN changes in the middle should have been omitted:\n\n $ git diff -I DEF_VER v2.33.3 v2.33.3^@\n diff --cc Documentation/RelNotes/2.33.3.txt\n index 0000000000,0000000000..e2bada12a1\n new file mode 100644\n --- /dev/null\n +++ b/Documentation/RelNotes/2.33.3.txt\n @@@ -1,0 -1,0 +1,4 @@@\n ++Git Documentation/RelNotes/2.33.3.txt Release Notes\n ++=========================\n ++\n ++This release merges up the fixes that appear in v2.33.3.\n diff --cc GIT-VERSION-GEN\n index d81eab5f00,e7efe58866..86a3a2870c\n --- a/GIT-VERSION-GEN\n +++ b/GIT-VERSION-GEN\n @@@ -1,7 -1,7 +1,7 @@@\n #!/bin/sh\n\n GVF=GIT-VERSION-FILE\n - DEF_VER=v2.33.2\n -DEF_VER=v2.32.2\n ++DEF_VER=v2.33.3\n\n LF='\n '\n diff --cc RelNotes\n index 8e79de2efe,4ac68388c3..899139d9ec\n --- a/RelNotes\n +++ b/RelNotes\n @@@ -1,1 -1,1 +1,1 @@@\n - Documentation/RelNotes/2.33.2.txt\n -Documentation/RelNotes/2.32.2.txt\n ++Documentation/RelNotes/2.33.3.txt\n\nJust setting need_generic_pathscan is not enough. Ideas?\n\nRené\n\n"},{"id":"455801","messageId":"patch-1.1-f7fd645468c-20220523T182954Z-avarab@gmail.com","threadId":"57880","inReplyTo":"a6a14213-bc82-d6fb-43dd-5a423c40a4f8@web.de","subject":"[PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-23T18:31:28Z","receivedAt":"2022-05-23T18:48:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in c45dc9cf30 (diff: plug memory leak from regcomp()\non {log,diff} -I, 2021-02-11), as noted in [1] there was a logic error\nwhere we'd free the regex too soon.\n\nNow we'll ensure that diff_free() can be called repeatedly\ninstead. We'd ultimately like to do away with the \"no_free\" confusion\nsurrounding it, and to attempt to free() things only once, as outlined\nin [2]. But in the meantime this will fix the segfault.\n\nSince the rest of diff_free() was already safe re-invoke this only\naffected the -I and --output= options. In both cases our\nbehavior before wasn't very sensible. When producing a combined diff\nwe'll go through combined-diff.c, which doesn't handle many of the\noptions that the corresponding diff.c codepaths do.\n\nThus we're here testing that -I is ignored in this case, and\nlikewise for --output=, but since this is what we were doing\nbefore c45dc9cf30 let's accept it for now.\n\n1. https://lore.kernel.org/git/a6a14213-bc82-d6fb-43dd-5a423c40a4f8@web.de/\n2. https://lore.kernel.org/git/220520.86pmk81a9z.gmgdl@evledraar.gmail.com/\n\nReported-by: René Scharfe \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Sat, May 14 2022, René Scharfe wrote:\n\n> Hi all,\n>\n> git diff segfaults when it's asked to produce a combined diff and ignore\n> certain lines with --ignore-matching-lines/-I, e.g.:\n>\n> $ git diff -I DEF_VER v2.33.3 v2.33.3^@\n> zsh: segmentation fault ./git-diff -I DEF_VER v2.33.3 v2.33.3^@\n\n diff.c | 9 ++++++---\n t/t4013-diff-various.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e71cf758861..183c9f21305 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6432,8 +6432,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \n static void diff_free_file(struct diff_options *options)\n {\n-\tif (options->close_file)\n-\t\tfclose(options->file);\n+\tif (!options->close_file)\n+\t\treturn;\n+\toptions->close_file = 0;\n+\tfclose(options->file);\n }\n \n static void diff_free_ignore_regex(struct diff_options *options)\n@@ -6444,7 +6446,8 @@ static void diff_free_ignore_regex(struct diff_options *options)\n \t\tregfree(options->ignore_regex[i]);\n \t\tfree(options->ignore_regex[i]);\n \t}\n-\tfree(options->ignore_regex);\n+\toptions->ignore_regex_nr = 0;\n+\tFREE_AND_NULL(options->ignore_regex);\n }\n \n void diff_free(struct diff_options *options)\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 056e922164d..b556d185f53 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -614,4 +614,19 @@ test_expect_success 'diff -I: detect malformed regex' '\n \ttest_i18ngrep \"invalid regex given to -I: \" error\n '\n \n+test_expect_success 'diff -I: combined diff does not segfault' '\n+\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n+\tgit diff $revs >expect &&\n+\tgit diff -I . $revs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --output=: combined diff does not segfault' '\n+\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n+\tgit diff --output=expect.file $revs >expect.out &&\n+\tgit diff $revs >actual &&\n+\ttest_cmp expect.out actual &&\n+\ttest_must_be_empty expect.file\n+'\n+\n test_done\n-- \n2.36.1.1038.gde12b200317\n\n"},{"id":"455815","messageId":"xmqqleusqaff.fsf@gitster.g","threadId":"57880","inReplyTo":"patch-1.1-f7fd645468c-20220523T182954Z-avarab@gmail.com","subject":"Re: [PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T20:08:20Z","receivedAt":"2022-05-23T20:08:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Fix a regression in c45dc9cf30 (diff: plug memory leak from regcomp()\n> on {log,diff} -I, 2021-02-11), as noted in [1] there was a logic error\n> where we'd free the regex too soon.\n>\n> Now we'll ensure that diff_free() can be called repeatedly\n> instead. We'd ultimately like to do away with the \"no_free\" confusion\n> surrounding it, and to attempt to free() things only once, as outlined\n> in [2]. But in the meantime this will fix the segfault.\n\nHmph, repeated calls to diff_free_file() now closes the file upon\nthe first call. I would have expected that such a resource would be\nreleased when all the references go away, i.e. upon the last call.\nThe same thing for the ignore-regex array.\n\nClearing the \"options->close_file\" bit, and using FREE_AND_NULL(),\nwould hide a breakage that could be caused by this change, doesn't\nit, because any use-after-release will say \"ah, no need to close the\nfile\" and \"oh, there is no regex\". The former is not so worrisome,\nbut the latter may be---we may no longer have regex because the\nfirst call to diff_free_ignore_regex() has cleared it and the code\nthat wants to use the regex, if exists, would happily say \"oh, there\nis no regex\", instead of exposing the use-after-release breakage to\na segfault.\n\n> Thus we're here testing that -I is ignored in this case, and\n> likewise for --output=, but since this is what we were doing\n> before c45dc9cf30 let's accept it for now.\n\nIt is true that the result of applying this patch is equivalent to\nc45dc9cf (diff: plug memory leak from regcomp() on {log,diff} -I,\n2021-02-11), but doesn't that merely point at the commit as the\nsource of behaviour breakage? With ignore-regex leaking before that\ncommit, wouldn't we have been using ignore-regex with combined diff\nmachinery?\n\nSorry, but I am failing to convince myself that this is not sweep\nthe issue under the rug.\n\n> 1. https://lore.kernel.org/git/a6a14213-bc82-d6fb-43dd-5a423c40a4f8@web.de/\n> 2. https://lore.kernel.org/git/220520.86pmk81a9z.gmgdl@evledraar.gmail.com/\n>\n> Reported-by: René Scharfe \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> On Sat, May 14 2022, René Scharfe wrote:\n>\n>> Hi all,\n>>\n>> git diff segfaults when it's asked to produce a combined diff and ignore\n>> certain lines with --ignore-matching-lines/-I, e.g.:\n>>\n>> $ git diff -I DEF_VER v2.33.3 v2.33.3^@\n>> zsh: segmentation fault ./git-diff -I DEF_VER v2.33.3 v2.33.3^@\n>\n> diff.c | 9 ++++++---\n> t/t4013-diff-various.sh | 15 +++++++++++++++\n> 2 files changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index e71cf758861..183c9f21305 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6432,8 +6432,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n> \n> static void diff_free_file(struct diff_options *options)\n> {\n> -\tif (options->close_file)\n> -\t\tfclose(options->file);\n> +\tif (!options->close_file)\n> +\t\treturn;\n> +\toptions->close_file = 0;\n> +\tfclose(options->file);\n> }\n> \n> static void diff_free_ignore_regex(struct diff_options *options)\n> @@ -6444,7 +6446,8 @@ static void diff_free_ignore_regex(struct diff_options *options)\n> \t\tregfree(options->ignore_regex[i]);\n> \t\tfree(options->ignore_regex[i]);\n> \t}\n> -\tfree(options->ignore_regex);\n> +\toptions->ignore_regex_nr = 0;\n> +\tFREE_AND_NULL(options->ignore_regex);\n> }\n> \n> void diff_free(struct diff_options *options)\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index 056e922164d..b556d185f53 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -614,4 +614,19 @@ test_expect_success 'diff -I: detect malformed regex' '\n> \ttest_i18ngrep \"invalid regex given to -I: \" error\n> '\n> \n> +test_expect_success 'diff -I: combined diff does not segfault' '\n> +\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n> +\tgit diff $revs >expect &&\n> +\tgit diff -I . $revs >actual &&\n> +\ttest_cmp expect actual\n\nAnd indeed this casts such a broken behaviour in stone.\n\n> +'\n> +\n> +test_expect_success 'diff --output=: combined diff does not segfault' '\n> +\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n> +\tgit diff --output=expect.file $revs >expect.out &&\n> +\tgit diff $revs >actual &&\n> +\ttest_cmp expect.out actual &&\n> +\ttest_must_be_empty expect.file\n\nSo is this one.\n\n> +'\n> +\n> test_done\n\n"},{"id":"455886","messageId":"220524.86leurw3my.gmgdl@evledraar.gmail.com","threadId":"57880","inReplyTo":"xmqqleusqaff.fsf@gitster.g","subject":"Re: [PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T11:38:16Z","receivedAt":"2022-05-24T11:50:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 23 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Fix a regression in c45dc9cf30 (diff: plug memory leak from regcomp()\n>> on {log,diff} -I, 2021-02-11), as noted in [1] there was a logic error\n>> where we'd free the regex too soon.\n>>\n>> Now we'll ensure that diff_free() can be called repeatedly\n>> instead. We'd ultimately like to do away with the \"no_free\" confusion\n>> surrounding it, and to attempt to free() things only once, as outlined\n>> in [2]. But in the meantime this will fix the segfault.\n>\n> Hmph, repeated calls to diff_free_file() now closes the file upon\n> the first call. I would have expected that such a resource would be\n> released when all the references go away, i.e. upon the last call.\n> The same thing for the ignore-regex array.\n\nYes, that would be much more sensible. But as noted:\n\n When producing a combined diff we'll go through combined-diff.c,\n which doesn't handle many of the options that the corresponding\n diff.c codepaths do.\n\nI.e. the \"right\" thing to do in this case would require a much more\ninvolved fix. We've somehow ended up not supporting --output=, -I\nand probably many other options in the combined-diff mode, which both in\ntesting and in this part of the implementation seems to have become an\nafterthought.\n\nSo before any changes of mine we silently ignore those options, and in\nthose particular cases the \"right\" thing to do if we're not growing new\nfeatures would probably be to error out early if they were provided in\nthe combined diff mode.\n\nBut as a minimal fix just tailoring diff_free() towards the\nnot-combined-diff.c case seems to be the smallest & most correct thing\nto do for now to address the segfault & the immediate issue.\n\n> Clearing the \"options->close_file\" bit, and using FREE_AND_NULL(),\n> would hide a breakage that could be caused by this change, doesn't\n> it, because any use-after-release will say \"ah, no need to close the\n> file\" and \"oh, there is no regex\". The former is not so worrisome,\n> but the latter may be---we may no longer have regex because the\n> first call to diff_free_ignore_regex() has cleared it and the code\n> that wants to use the regex, if exists, would happily say \"oh, there\n> is no regex\", instead of exposing the use-after-release breakage to\n> a segfault.\n\nYes, this wouldn't make much sense if we were supporting the file output\nand -I in the combined-diff.c case, but AFAICT the two cases are:\n\n\n 1. The \"normal\" diff case, where we set those up once, and diff_free()\n them once.\n\n 2. The \"combined-diff.c\" case, where we might call diff_free() N times,\n but it's all to produce the diff itself, not for those options.\n\n\n>> Thus we're here testing that -I is ignored in this case, and\n>> likewise for --output=, but since this is what we were doing\n>> before c45dc9cf30 let's accept it for now.\n>\n> It is true that the result of applying this patch is equivalent to\n> c45dc9cf (diff: plug memory leak from regcomp() on {log,diff} -I,\n> 2021-02-11), but doesn't that merely point at the commit as the\n> source of behaviour breakage? With ignore-regex leaking before that\n> commit, wouldn't we have been using ignore-regex with combined diff\n> machinery?\n\nNo, because -I never did anything with the combined diff machinery,\nneither did --output.\n\n> Sorry, but I am failing to convince myself that this is not sweep\n> the issue under the rug.\n\nI think that's a fair summary, much of it was already under the rug,\nwe're sweeping some of the remainin parts under it :)\n\nI think that whole combined-diff interaction really needs to be fix, not\njust for the diff_free() case, but e.g. we should either error out or\nsupport options that we're silently ignoring now.\n\nBut as noted in\nhttps://lore.kernel.org/git/220520.86pmk81a9z.gmgdl@evledraar.gmail.com/\nI do have patches queued up locally that form a better basis for fixing\nthese issues. I.e. once we fix this segfault and have\nrelease_revisions() it'll be easy to get rid of that \"no_free\" case in\ndiff_free().\n\n>> [...]\n>> void diff_free(struct diff_options *options)\n>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n>> index 056e922164d..b556d185f53 100755\n>> --- a/t/t4013-diff-various.sh\n>> +++ b/t/t4013-diff-various.sh\n>> @@ -614,4 +614,19 @@ test_expect_success 'diff -I: detect malformed regex' '\n>> \ttest_i18ngrep \"invalid regex given to -I: \" error\n>> '\n>> \n>> +test_expect_success 'diff -I: combined diff does not segfault' '\n>> +\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n>> +\tgit diff $revs >expect &&\n>> +\tgit diff -I . $revs >actual &&\n>> +\ttest_cmp expect actual\n>\n> And indeed this casts such a broken behaviour in stone.\n>\n>> +'\n>> +\n>> +test_expect_success 'diff --output=: combined diff does not segfault' '\n>> +\trevs=\"HEAD~2 HEAD~ HEAD\" &&\n>> +\tgit diff --output=expect.file $revs >expect.out &&\n>> +\tgit diff $revs >actual &&\n>> +\ttest_cmp expect.out actual &&\n>> +\ttest_must_be_empty expect.file\n>\n> So is this one.\n\nI was on the fence about adding these tests, since I expected you to\ncomment on this aspect of them. I.e. we could just ignore the output\nhere and narrowly see if we segfault.\n\nBut since we had no tests at all for this before, and intentional or not\nthis behavior of combined-diff is long-standing behavior (that nobody\nseems to have noticed or cared about) I think it's good to have tests\nthat check the \"expected\" (as in what we did before my c45dc9cf30)\noutput.\n"},{"id":"455938","messageId":"xmqqmtf6hgae.fsf@gitster.g","threadId":"57880","inReplyTo":"220524.86leurw3my.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T19:38:49Z","receivedAt":"2022-05-24T19:38:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I.e. the \"right\" thing to do in this case would require a much more\n> involved fix. We've somehow ended up not supporting --output=, -I\n> and probably many other options in the combined-diff mode, which both in\n> testing and in this part of the implementation seems to have become an\n> afterthought.\n\nOK, a hopefully final question.\n\nHow much less involved is it to add a new code (without doing\nanything in this patch) to detect and die on the combination of\ncombined-diff with these two options, so that we can document the\nfact that we do not support them? It would give us much better way\nforward than leaving the command silently ignore and give result\nthat is not in line with what was asked, wouldn't it? That way, the\nmuch more involved \"fix\" will turn into a change to add a missing\nfeature.\n\nThanks.\n\n"},{"id":"455950","messageId":"220524.86v8tuvfl1.gmgdl@evledraar.gmail.com","threadId":"57880","inReplyTo":"xmqqmtf6hgae.fsf@gitster.g","subject":"Re: [PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T20:17:24Z","receivedAt":"2022-05-24T20:30:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I.e. the \"right\" thing to do in this case would require a much more\n>> involved fix. We've somehow ended up not supporting --output=, -I\n>> and probably many other options in the combined-diff mode, which both in\n>> testing and in this part of the implementation seems to have become an\n>> afterthought.\n>\n> OK, a hopefully final question.\n>\n> How much less involved is it to add a new code (without doing\n> anything in this patch)\n\n...yeah, I think for this one it makes sense to narrowly focus on the\nsegfault...\n\n> to detect and die on the combination of\n> combined-diff with these two options, so that we can document the\n> fact that we do not support them? It would give us much better way\n> forward than leaving the command silently ignore and give result\n> that is not in line with what was asked, wouldn't it? That way, the\n> much more involved \"fix\" will turn into a change to add a missing\n> feature.\n\nI think not much, it's rather trivial for the case where we invoke \"git\ndiff\", I.e. just adding something to the \"builtin_diff_combined()\"\nbranch in builtin/diff.c to detect these two cases specifically.\n\nI haven't looked in any depth into how we might reach code in\ncombine-diff.c through other means, and if any of it can set these two\nindirectly somewhere else (i.e. other things that take diff options).\n\nI also wonder if I'm just wrong in my assessment that it's a Bad Thing\nthat we take some of these without ever doing anything with them in some\nmodes, e.g.:\n\n\tgit log --oneline -I foo\n\nThis will never do anything with that \"-I foo\" by definition, but would\nas soon as you add -p, should we error without -p (or other diff-showing\noptions).\n\nThe same goes for range-diff, format-patch, --remerge-diff and any\nnumber of other things where we take the full set of options, but only\ndo something with a limited subset of them.\n\nIt is helpful in some cases if we were more anal about it, e.g. when I\nwas wondering why -I didn't do anything with the combined diff, but also\nhandy for scripting and one-liners if you can tweak the command-line\nback & forth without it being so strict.\n\nSo I don't know. Maybe I'm just trying to talk myself out of pulling on\nthat (bound to be long) thread, but I'm coming more around to this just\nbeing a non-issue beyond the narrow and needed fix for diff_free() in\nparticular.\n\nI.e. the more general approach of chasing down options that don't do\nanything for a given \"diff mode\". We might still want to error on some\nparticular ones, such as -I with the combined diff (but not with\n--oneline, or whatever).\n"},{"id":"457496","messageId":"90d4611c-a8b1-dc15-a02a-8f5ba378d572@web.de","threadId":"57880","inReplyTo":"220524.86v8tuvfl1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] diff: fix a segfault in >2 tree -I and --output=","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-18T11:12:13Z","receivedAt":"2022-06-18T11:12:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.05.22 um 22:17 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, May 24 2022, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> I.e. the \"right\" thing to do in this case would require a much more\n>>> involved fix. We've somehow ended up not supporting --output=, -I\n>>> and probably many other options in the combined-diff mode, which both in\n>>> testing and in this part of the implementation seems to have become an\n>>> afterthought.\n>>\n>> OK, a hopefully final question.\n>>\n>> How much less involved is it to add a new code (without doing\n>> anything in this patch)\n>\n> ...yeah, I think for this one it makes sense to narrowly focus on the\n> segfault...\n>\n>> to detect and die on the combination of\n>> combined-diff with these two options, so that we can document the\n>> fact that we do not support them? It would give us much better way\n>> forward than leaving the command silently ignore and give result\n>> that is not in line with what was asked, wouldn't it? That way, the\n>> much more involved \"fix\" will turn into a change to add a missing\n>> feature.\n>\n> I think not much, it's rather trivial for the case where we invoke \"git\n> diff\", I.e. just adding something to the \"builtin_diff_combined()\"\n> branch in builtin/diff.c to detect these two cases specifically.\n>\n> I haven't looked in any depth into how we might reach code in\n> combine-diff.c through other means, and if any of it can set these two\n> indirectly somewhere else (i.e. other things that take diff options).\n\nSo let's add those checks there.\n\n> I also wonder if I'm just wrong in my assessment that it's a Bad Thing\n> that we take some of these without ever doing anything with them in some\n> modes, e.g.:\n>\n> \tgit log --oneline -I foo\n>\n> This will never do anything with that \"-I foo\" by definition, but would\n> as soon as you add -p, should we error without -p (or other diff-showing\n> options).\n\nWhich definition? The documentation says:\n\n -I, --ignore-matching-lines=\n Ignore changes whose all lines match . This option may be specified more than once.\n\nThat sounds to me like it would affect history simplification, and thus\ngit log --oneline. (Which seems expensive, but that's a different\nconcern.) So based on that I'd expect at least a warning if -I is\nignored.\n\n> The same goes for range-diff, format-patch, --remerge-diff and any\n> number of other things where we take the full set of options, but only\n> do something with a limited subset of them.\n>\n> It is helpful in some cases if we were more anal about it, e.g. when I\n> was wondering why -I didn't do anything with the combined diff, but also\n> handy for scripting and one-liners if you can tweak the command-line\n> back & forth without it being so strict.\n>\n> So I don't know. Maybe I'm just trying to talk myself out of pulling on\n> that (bound to be long) thread, but I'm coming more around to this just\n> being a non-issue beyond the narrow and needed fix for diff_free() in\n> particular.\n>\n> I.e. the more general approach of chasing down options that don't do\n> anything for a given \"diff mode\". We might still want to error on some\n> particular ones, such as -I with the combined diff (but not with\n> --oneline, or whatever).\n\nSupporting all combinations would be ideal. Reporting unsupported\ncombinations would be the next best thing. I wonder if we passed the\npoint of having so many options for e.g. git log that assessing all\nof their pairings has become impractical, though. :-/\n\nRené\n"},{"id":"457497","messageId":"49bcadd1-7dc2-d5ee-36a5-59456450aeca@web.de","threadId":"57880","inReplyTo":"220524.86v8tuvfl1.gmgdl@evledraar.gmail.com","subject":"[PATCH 1/2] combine-diff: abort if --ignore-matching-lines is given","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-18T11:12:28Z","receivedAt":"2022-06-18T11:12:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The code for combined diffs doesn't currently support ignoring changes\nthat match a regex. Abort and report that fact instead of running into\na segfault.\n\nSigned-off-by: René Scharfe \n---\n combine-diff.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex b724f02123..11df1d7f39 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1498,6 +1498,10 @@ void diff_tree_combined(const struct object_id *oid,\n \tint i, num_paths, needsep, show_log_first, num_parent = parents->nr;\n \tint need_generic_pathscan;\n\n+\tif (opt->ignore_regex_nr)\n+\t\tdie(\"combined diff and '%s' cannot be used together\",\n+\t\t \"--ignore-matching-lines\");\n+\n \t/* nothing to do, if no parents */\n \tif (!num_parent)\n \t\treturn;\n--\n2.36.1\n"},{"id":"457498","messageId":"493bbdb3-be73-9aa2-e3f6-82508d15ef88@web.de","threadId":"57880","inReplyTo":"220524.86v8tuvfl1.gmgdl@evledraar.gmail.com","subject":"[PATCH 2/2] combine-diff: abort if --output is given","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-18T11:12:34Z","receivedAt":"2022-06-18T11:12:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The code for combined diffs currently only writes to stdout. Abort and\nreport that fact instead of silently ignoring the --output option. The\n(empty) output file has already been created at that point, though.\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: René Scharfe \n---\n combine-diff.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 11df1d7f39..b0ece95480 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1501,6 +1501,9 @@ void diff_tree_combined(const struct object_id *oid,\n \tif (opt->ignore_regex_nr)\n \t\tdie(\"combined diff and '%s' cannot be used together\",\n \t\t \"--ignore-matching-lines\");\n+\tif (opt->close_file)\n+\t\tdie(\"combined diff and '%s' cannot be used together\",\n+\t\t \"--output\");\n\n \t/* nothing to do, if no parents */\n \tif (!num_parent)\n--\n2.36.1\n"},{"id":"457620","messageId":"xmqqedzijago.fsf@gitster.g","threadId":"57880","inReplyTo":"49bcadd1-7dc2-d5ee-36a5-59456450aeca@web.de","subject":"Re: [PATCH 1/2] combine-diff: abort if --ignore-matching-lines is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T15:35:35Z","receivedAt":"2022-06-21T15:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The code for combined diffs doesn't currently support ignoring changes\n> that match a regex. Abort and report that fact instead of running into\n> a segfault.\n>\n> Signed-off-by: René Scharfe \n> ---\n> combine-diff.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n\nMakes sense.\n\n> diff --git a/combine-diff.c b/combine-diff.c\n> index b724f02123..11df1d7f39 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1498,6 +1498,10 @@ void diff_tree_combined(const struct object_id *oid,\n> \tint i, num_paths, needsep, show_log_first, num_parent = parents->nr;\n> \tint need_generic_pathscan;\n>\n> +\tif (opt->ignore_regex_nr)\n> +\t\tdie(\"combined diff and '%s' cannot be used together\",\n> +\t\t \"--ignore-matching-lines\");\n\n\"X cannot be used together _with_ Y\" perhaps?\n\n> \t/* nothing to do, if no parents */\n> \tif (!num_parent)\n> \t\treturn;\n> --\n> 2.36.1\n"},{"id":"457625","messageId":"95d7609e-4cb5-c87e-0120-144f1f094241@web.de","threadId":"57880","inReplyTo":"xmqqedzijago.fsf@gitster.g","subject":"Re: [PATCH 1/2] combine-diff: abort if --ignore-matching-lines is given","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-21T15:58:50Z","receivedAt":"2022-06-21T16:00:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.06.22 um 17:35 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> The code for combined diffs doesn't currently support ignoring changes\n>> that match a regex. Abort and report that fact instead of running into\n>> a segfault.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> combine-diff.c | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>\n> Makes sense.\n>\n>> diff --git a/combine-diff.c b/combine-diff.c\n>> index b724f02123..11df1d7f39 100644\n>> --- a/combine-diff.c\n>> +++ b/combine-diff.c\n>> @@ -1498,6 +1498,10 @@ void diff_tree_combined(const struct object_id *oid,\n>> \tint i, num_paths, needsep, show_log_first, num_parent = parents->nr;\n>> \tint need_generic_pathscan;\n>>\n>> +\tif (opt->ignore_regex_nr)\n>> +\t\tdie(\"combined diff and '%s' cannot be used together\",\n>> +\t\t \"--ignore-matching-lines\");\n>\n> \"X cannot be used together _with_ Y\" perhaps?\n\nNot sure, but that type of message was recently unified (most common\ncase: \"options '%s' and '%s' cannot be used together\") and \"with\" is\nonly used in an untranslated BUG:\n\n$ git grep -e \"cannot be used together\" --and --not -e options '*.c'\nbuiltin/add.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nbuiltin/am.c: \"cannot be used together\"),\nbuiltin/checkout.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nbuiltin/clean.c: die(_(\"-x and -X cannot be used together\"));\nbuiltin/commit.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nbuiltin/commit.c: die(_(\"reword option of '%s' and path '%s' cannot be used together\"), \"--fixup\", *argv);\nbuiltin/commit.c: die(_(\"reword option of '%s' and '%s' cannot be used together\"),\nbuiltin/describe.c: die(_(\"option '%s' and commit-ishes cannot be used together\"), \"--dirty\");\nbuiltin/describe.c: die(_(\"option '%s' and commit-ishes cannot be used together\"), \"--broken\");\nbuiltin/rebase.c: \"cannot be used together\"));\nbuiltin/reset.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nbuiltin/rev-list.c: die(_(\"marked counting and '%s' cannot be used together\"), \"--objects\");\nbuiltin/rm.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nbuiltin/stash.c: die(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\nrevision.c: BUG(\"--single-worktree cannot be used together with submodule\");\nupload-pack.c: die(\"git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together\");\n\nSo there doesn't seem to be a fully consistent style amongst these\nspecial cases. How about this?\n\n\t\tdie(_(\"option '%s' and combined diffs cannot be used together\"),\n\n>\n>> \t/* nothing to do, if no parents */\n>> \tif (!num_parent)\n>> \t\treturn;\n>> --\n>> 2.36.1\n"},{"id":"457631","messageId":"xmqqsfnygdlz.fsf@gitster.g","threadId":"57880","inReplyTo":"95d7609e-4cb5-c87e-0120-144f1f094241@web.de","subject":"Re: [PATCH 1/2] combine-diff: abort if --ignore-matching-lines is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-21T16:55:52Z","receivedAt":"2022-06-21T16:56:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>>> +\tif (opt->ignore_regex_nr)\n>>> +\t\tdie(\"combined diff and '%s' cannot be used together\",\n>>> +\t\t \"--ignore-matching-lines\");\n>>\n>> \"X cannot be used together _with_ Y\" perhaps?\n>\n> Not sure, but that type of message was recently unified (most common\n> case: \"options '%s' and '%s' cannot be used together\") and \"with\" is\n> only used in an untranslated BUG:\n\nAh, sorry, I completely misread the original as \"combined diff\ncannot be used together with ignore-matching-lines\".\n\nSorry for the noise.\n"}]}