{"thread":{"id":"57822","subject":"Bug: `git show` honors path filters only for the first commit","startedAt":"2022-04-30T02:22:24Z","lastAt":"2022-05-20T17:23:33Z","messageCount":10,"participants":["Daniel Li","Junio C Hamano","René Scharfe","Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"454646","messageId":"CAHVT7hW28jMcphDPhcUG==mycCWDaAt46wWo68=oTcSvebHWwg@mail.gmail.com","threadId":"57822","inReplyTo":null,"subject":"Bug: `git show` honors path filters only for the first commit","fromName":"Daniel Li","fromEmail":"dan@danielyli.com","sentAt":"2022-04-30T02:22:06Z","receivedAt":"2022-04-30T02:22:24Z","isPatch":false,"sender":{"key":"dan@danielyli.com","avatar":null},"body":"git version: 2.36.0\nOS: macOS Monterey 12.2.1\nInstalled via: homebrew\n\nBug summary: When `git show` is invoked with more than one commit, it\nonly respects the `<path>` filters for the first commit.\n\nThis is best illustrated with an example. In git's own source repo,\ninvoke `git show --oneline --name-only ecc7c8841d 961b130d20\nd3115660b4` and observe the files these commits touch (the `--oneline\n--name-only` arguments are to make this example short; they're not\nrelevant to the bug):\n\n    $ git show --oneline --numstat ecc7c8841d 961b130d20 d3115660b4\n    ecc7c8841d repo_read_index: add config to expect files outside\nsparse patterns\n    2       0       Documentation/config.txt\n    27      0       Documentation/config/sparse.txt\n    1       0       cache.h\n    14      0       config.c\n    1       0       environment.c\n    2       1       sparse-index.c\n    19      0       t/t1090-sparse-checkout-scope.sh\n    961b130d20 branch: add --recurse-submodules option for branch creation\n    3       0       Documentation/config/advice.txt\n    26      11      Documentation/config/submodule.txt\n    18      1       Documentation/git-branch.txt\n    1       0       advice.c\n    1       0       advice.h\n    141     0       branch.c\n    29      0       branch.h\n    38      6       builtin/branch.c\n    38      0       builtin/submodule--helper.c\n    61      0       submodule-config.c\n    34      0       submodule-config.h\n    9       2       submodule.c\n    3       0       submodule.h\n    292     0       t/t3207-branch-submodule.sh\n    d3115660b4 branch: add flags and config to inherit tracking\n    2       1       Documentation/config/branch.txt\n    17      7       Documentation/git-branch.txt\n    1       1       Documentation/git-checkout.txt\n    1       1       Documentation/git-switch.txt\n    42      7       branch.c\n    2       1       branch.h\n    4       2       builtin/branch.c\n    4       2       builtin/checkout.c\n    3       0       config.c\n    16      0       parse-options-cb.c\n    2       0       parse-options.h\n    10      1       t/t2017-checkout-orphan.sh\n    23      0       t/t2027-checkout-track.sh\n    28      0       t/t2060-switch.sh\n    33      0       t/t3200-branch.sh\n    17      0       t/t7201-co.sh\n\nNow invoke the same command but filtered on files under the\n`Documentation/` directory. Observe that this path is only respected\nfor the first commit:\n\n    $ git show --oneline --numstat ecc7c8841d 961b130d20 d3115660b4 --\nDocumentation\n    ecc7c8841d repo_read_index: add config to expect files outside\nsparse patterns\n    2       0       Documentation/config.txt\n    27      0       Documentation/config/sparse.txt\n    961b130d20 branch: add --recurse-submodules option for branch creation\n    3       0       Documentation/config/advice.txt\n    26      11      Documentation/config/submodule.txt\n    18      1       Documentation/git-branch.txt\n    1       0       advice.c\n    1       0       advice.h\n    141     0       branch.c\n    29      0       branch.h\n    38      6       builtin/branch.c\n    38      0       builtin/submodule--helper.c\n    61      0       submodule-config.c\n    34      0       submodule-config.h\n    9       2       submodule.c\n    3       0       submodule.h\n    292     0       t/t3207-branch-submodule.sh\n    d3115660b4 branch: add flags and config to inherit tracking\n    2       1       Documentation/config/branch.txt\n    17      7       Documentation/git-branch.txt\n    1       1       Documentation/git-checkout.txt\n    1       1       Documentation/git-switch.txt\n    42      7       branch.c\n    2       1       branch.h\n    4       2       builtin/branch.c\n    4       2       builtin/checkout.c\n    3       0       config.c\n    16      0       parse-options-cb.c\n    2       0       parse-options.h\n    10      1       t/t2017-checkout-orphan.sh\n    23      0       t/t2027-checkout-track.sh\n    28      0       t/t2060-switch.sh\n    33      0       t/t3200-branch.sh\n    17      0       t/t7201-co.sh\n\nThe expected output should be:\n\n    $ git show --oneline --numstat ecc7c8841d 961b130d20 d3115660b4 --\nDocumentation\n    ecc7c8841d repo_read_index: add config to expect files outside\nsparse patterns\n    2       0       Documentation/config.txt\n    27      0       Documentation/config/sparse.txt\n    961b130d20 branch: add --recurse-submodules option for branch creation\n    3       0       Documentation/config/advice.txt\n    26      11      Documentation/config/submodule.txt\n    18      1       Documentation/git-branch.txt\n    d3115660b4 branch: add flags and config to inherit tracking\n    2       1       Documentation/config/branch.txt\n    17      7       Documentation/git-branch.txt\n    1       1       Documentation/git-checkout.txt\n    1       1       Documentation/git-switch.txt\n\nBonus: Surprisingly, the `<path>` *is* respected for commits that\ndon't have any files satisfying the `<path>`. For example, the\nfollowing command correctly excludes commit `f36d4f8316` from the\noutput because it doesn't contain any files under `Documentation/`:\n\n    $ git show --oneline --numstat ecc7c8841d f36d4f8316 961b130d20\nd3115660b4 -- Documentation\n    ecc7c8841d repo_read_index: add config to expect files outside\nsparse patterns\n    2       0       Documentation/config.txt\n    27      0       Documentation/config/sparse.txt\n    961b130d20 branch: add --recurse-submodules option for branch creation\n    3       0       Documentation/config/advice.txt\n    26      11      Documentation/config/submodule.txt\n    18      1       Documentation/git-branch.txt\n    1       0       advice.c\n    1       0       advice.h\n    141     0       branch.c\n    29      0       branch.h\n    38      6       builtin/branch.c\n    38      0       builtin/submodule--helper.c\n    61      0       submodule-config.c\n    34      0       submodule-config.h\n    9       2       submodule.c\n    3       0       submodule.h\n    292     0       t/t3207-branch-submodule.sh\n    d3115660b4 branch: add flags and config to inherit tracking\n    2       1       Documentation/config/branch.txt\n    17      7       Documentation/git-branch.txt\n    1       1       Documentation/git-checkout.txt\n    1       1       Documentation/git-switch.txt\n    42      7       branch.c\n    2       1       branch.h\n    4       2       builtin/branch.c\n    4       2       builtin/checkout.c\n    3       0       config.c\n    16      0       parse-options-cb.c\n    2       0       parse-options.h\n    10      1       t/t2017-checkout-orphan.sh\n    23      0       t/t2027-checkout-track.sh\n    28      0       t/t2060-switch.sh\n    33      0       t/t3200-branch.sh\n    17      0       t/t7201-co.sh\n\nCheers,\n\nDan Li\n\n--\n\nDaniel Li\ndan@danielyli.com\n"},{"id":"454672","messageId":"xmqqzgk388tt.fsf@gitster.g","threadId":"57822","inReplyTo":"CAHVT7hW28jMcphDPhcUG==mycCWDaAt46wWo68=oTcSvebHWwg@mail.gmail.com","subject":"Re: Bug: `git show` honors path filters only for the first commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-30T04:59:58Z","receivedAt":"2022-04-30T05:00:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Li <dan@danielyli.com> writes:\n\n> git version: 2.36.0\n> OS: macOS Monterey 12.2.1\n> Installed via: homebrew\n\nI think this is the same regression as the recently talked about\n\"diff-tree --stdin\" aka \"gitk\" regression.\n\nhttps://lore.kernel.org/git/xmqq7d7bsu2n.fsf@gitster.g/\n\ne900d494 (diff: add an API for deferred freeing, 2021-02-11), broke\ncmd_log_walk(), and we started to lose some setting that was parsed\nfrom the command line and stored in the diff_options structure after\ncmd_log_walk() runs just once.  But \"git show A B\" runs the function\nonce for each commit.   A recent change in 2.36.0 made it worse by\nadding <pathspec> to the set of setting that gets lost after\ncmd_log_walk() runs once.\n\nThanks for a report.\n\n\n"},{"id":"454673","messageId":"xmqqo80j87g0.fsf_-_@gitster.g","threadId":"57822","inReplyTo":"xmqqzgk388tt.fsf@gitster.g","subject":"[PATCH] 2.36 show regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-30T05:29:51Z","receivedAt":"2022-04-30T05:30:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This only surfaced as a regression after 2.36 release, but the\nbreakage was already there with us for at least a year.\n\ne900d494 (diff: add an API for deferred freeing, 2021-02-11)\nintroduced a mechanism to delay freeing resources held in\ndiff_options struct that need to be kept as long as the struct will\nbe reused to compute diff.  \"git log -p\" was taught to utilize the\nmechanism but it was done with an incorrect assumption that the\nunderlying helper function, cmd_log_walk(), is called only once,\nand it is OK to do the freeing at the end of it.\n\nAlas, for \"git show A B\", the function is called once for each\ncommit given, so it is not OK to free the resources until we finish\ncalling it for all the commits given from the command line.\n\nDuring 2.36 release cycle, we started clearing the <pathspec> as\npart of this freeing, which made the bug a lot more visible.\n\nFix this breakage by tweaking how cmd_log_walk() frees the resources\nat the end and using a variant of it that does not immediately free\nthe resources to show each commit object from the command line in\n\"git show\".\n\nProtect the fix with a few new tests.\n\nReported-by: Daniel Li <dan@danielyli.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> Daniel Li <dan@danielyli.com> writes:\n>\n>> git version: 2.36.0\n>> OS: macOS Monterey 12.2.1\n>> Installed via: homebrew\n>\n> I think this is the same regression as the recently talked about\n> \"diff-tree --stdin\" aka \"gitk\" regression.\n>\n> https://lore.kernel.org/git/xmqq7d7bsu2n.fsf@gitster.g/\n>\n> e900d494 (diff: add an API for deferred freeing, 2021-02-11), broke\n> cmd_log_walk(), and we started to lose some setting that was parsed\n> from the command line and stored in the diff_options structure after\n> cmd_log_walk() runs just once.  But \"git show A B\" runs the function\n> once for each commit.   A recent change in 2.36.0 made it worse by\n> adding <pathspec> to the set of setting that gets lost after\n> cmd_log_walk() runs once.\n>\n> Thanks for a report.\n\n builtin/log.c           | 23 ++++++++++++++++++-----\n t/t4013-diff-various.sh | 19 +++++++++++++++++++\n 2 files changed, 37 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c211d66d1d..6696c4cfd0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -417,7 +417,7 @@ static void finish_early_output(struct rev_info *rev)\n \tshow_early_header(rev, \"done\", n);\n }\n \n-static int cmd_log_walk(struct rev_info *rev)\n+static int cmd_log_walk_no_free(struct rev_info *rev)\n {\n \tstruct commit *commit;\n \tint saved_nrl = 0;\n@@ -444,7 +444,6 @@ static int cmd_log_walk(struct rev_info *rev)\n \t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n-\trev->diffopt.no_free = 1;\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tif (!log_tree_commit(rev, commit) && rev->max_count >= 0)\n \t\t\t/*\n@@ -469,8 +468,6 @@ static int cmd_log_walk(struct rev_info *rev)\n \t}\n \trev->diffopt.degraded_cc_to_c = saved_dcctc;\n \trev->diffopt.needed_rename_limit = saved_nrl;\n-\trev->diffopt.no_free = 0;\n-\tdiff_free(&rev->diffopt);\n \n \tif (rev->remerge_diff) {\n \t\ttmp_objdir_destroy(rev->remerge_objdir);\n@@ -484,6 +481,17 @@ static int cmd_log_walk(struct rev_info *rev)\n \treturn diff_result_code(&rev->diffopt, 0);\n }\n \n+static int cmd_log_walk(struct rev_info *rev)\n+{\n+\tint retval;\n+\n+\trev->diffopt.no_free = 1;\n+\tretval = cmd_log_walk_no_free(rev);\n+\trev->diffopt.no_free = 0;\n+\tdiff_free(&rev->diffopt);\n+\treturn retval;\n+}\n+\n static int git_log_config(const char *var, const char *value, void *cb)\n {\n \tconst char *slot_name;\n@@ -680,6 +688,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \n \tcount = rev.pending.nr;\n \tobjects = rev.pending.objects;\n+\trev.diffopt.no_free = 1;\n \tfor (i = 0; i < count && !ret; i++) {\n \t\tstruct object *o = objects[i].item;\n \t\tconst char *name = objects[i].name;\n@@ -725,12 +734,16 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.pending.nr = rev.pending.alloc = 0;\n \t\t\trev.pending.objects = NULL;\n \t\t\tadd_object_array(o, name, &rev.pending);\n-\t\t\tret = cmd_log_walk(&rev);\n+\t\t\tret = cmd_log_walk_no_free(&rev);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tret = error(_(\"unknown type: %d\"), o->type);\n \t\t}\n \t}\n+\n+\trev.diffopt.no_free = 0;\n+\tdiff_free(&rev.diffopt);\n+\n \tfree(objects);\n \treturn ret;\n }\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 750aee17ea..7a44d5d595 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -542,6 +542,25 @@ test_expect_success 'diff-tree --stdin with log formatting' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show A B ... -- <pathspec>' '\n+\t# side touches dir/sub, file0, and file3\n+\t# master^ touches dir/sub, and file1\n+\t# master^^ touches dir/sub, file0, and file2\n+\tgit show --name-only --format=\"<%s>\" side master^ master^^ -- dir >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t<Side>\n+\n+\tdir/sub\n+\t<Third>\n+\n+\tdir/sub\n+\t<Second>\n+\n+\tdir/sub\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'diff -I<regex>: setup' '\n \tgit checkout master &&\n \ttest_seq 50 >file0 &&\n-- \n2.36.0-256-g547811d5a1\n\n"},{"id":"454677","messageId":"c36896a1-6247-123b-4fa3-b7eb24af1897@web.de","threadId":"57822","inReplyTo":"xmqqo80j87g0.fsf_-_@gitster.g","subject":"[PATCH] 2.36 format-patch regression fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-30T10:32:44Z","receivedAt":"2022-04-30T10:33:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\nway to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\nhave diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\nthat mechanism mandatory.\n\ngit format-patch only sets no_free when --output is given, causing it to\nforget pathspecs after the first commit.  Set no_free unconditionally\ninstead.\n\nThe existing test was unable to detect this breakage because it checks\nstderr for the absence of a certain string, but format-patch writes to\nstdout.  Also the test was not checking the case of one commit modifying\nmultiple files and a pathspec limiting the diff.  Replace it with a more\nthorough one.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/log.c           |  9 ++-------\n t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++--\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c211d66d1d..9acc130594 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1883,6 +1883,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.diff = 1;\n \trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n+\trev.diffopt.no_free = 1;\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\n@@ -2008,13 +2009,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n\n \tif (use_stdout) {\n \t\tsetup_pager();\n-\t} else if (rev.diffopt.close_file) {\n-\t\t/*\n-\t\t * The diff code parsed --output; it has already opened the\n-\t\t * file, but we must instruct it not to close after each diff.\n-\t\t */\n-\t\trev.diffopt.no_free = 1;\n-\t} else {\n+\t} else if (!rev.diffopt.close_file) {\n \t\tint saved;\n\n \t\tif (!output_directory)\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7dc5a5c736..fbec8ad2ef 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -926,11 +926,40 @@ test_expect_success 'format-patch --numstat should produce a patch' '\n '\n\n test_expect_success 'format-patch -- <path>' '\n-\tgit format-patch main..side -- file 2>error &&\n-\t! grep \"Use .--\" error\n+\trm -f *.patch &&\n+\tgit checkout -b pathspec main &&\n+\n+\techo file_a 1 >file_a &&\n+\techo file_b 1 >file_b &&\n+\tgit add file_a file_b &&\n+\tgit commit -m pathspec_initial &&\n+\n+\techo file_a 2 >>file_a &&\n+\tgit add file_a &&\n+\tgit commit -m pathspec_a &&\n+\n+\techo file_b 2 >>file_b &&\n+\tgit add file_b &&\n+\tgit commit -m pathspec_b &&\n+\n+\techo file_a 3 >>file_a &&\n+\techo file_b 3 >>file_b &&\n+\tgit add file_a file_b &&\n+\tgit commit -m pathspec_ab &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t0001-pathspec_initial.patch\n+\t0002-pathspec_a.patch\n+\t0003-pathspec_ab.patch\n+\tEOF\n+\n+\tgit format-patch main..pathspec -- file_a >output &&\n+\ttest_cmp expect output &&\n+\t! grep file_b *.patch\n '\n\n test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '\n+\tgit checkout side &&\n \tgit format-patch --ignore-if-in-upstream HEAD\n '\n\n--\n2.35.3\n\n"},{"id":"454680","messageId":"2c988c7b-0efe-4222-4a43-8124fe1a9da6@web.de","threadId":"57822","inReplyTo":"xmqqo80j87g0.fsf_-_@gitster.g","subject":"[PATCH] 2.36 fast-export regression fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-30T14:31:43Z","receivedAt":"2022-04-30T14:32:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\nway to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\nhave diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\nthat mechanism mandatory.\n\ngit fast-export doesn't set no_free, so path limiting stopped working\nafter the first commit.  Set the flag and add a basic test to make sure\nonly changes to the specified files are exported.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/fast-export.c  | 1 +\n t/t9350-fast-export.sh | 7 +++++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex a7d72697fb..1355b5a96d 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -1261,6 +1261,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\n \trevs.diffopt.flags.recursive = 1;\n+\trevs.diffopt.no_free = 1;\n \twhile ((commit = get_revision(&revs)))\n \t\thandle_commit(commit, &revs, &paths_of_changed_objects);\n\ndiff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh\nindex 7b7a18dd2c..fc99703fc5 100755\n--- a/t/t9350-fast-export.sh\n+++ b/t/t9350-fast-export.sh\n@@ -500,6 +500,13 @@ test_expect_success 'path limiting with import-marks does not lose unmodified fi\n \tgrep file0 actual\n '\n\n+test_expect_success 'path limiting works' '\n+\tgit fast-export simple -- file >actual &&\n+\tsed -ne \"s/^M .* //p\" <actual | sort -u >actual.files &&\n+\techo file >expect &&\n+\ttest_cmp expect actual.files\n+'\n+\n test_expect_success 'avoid corrupt stream with non-existent mark' '\n \ttest_create_repo avoid_non_existent_mark &&\n \t(\n--\n2.35.3\n"},{"id":"454684","messageId":"20220430163232.ytvwru4fnylow2jk@carlos-mbp.lan","threadId":"57822","inReplyTo":"c36896a1-6247-123b-4fa3-b7eb24af1897@web.de","subject":"Re: [PATCH] 2.36 format-patch regression fix","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-04-30T16:32:32Z","receivedAt":"2022-04-30T16:32:39Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Apr 30, 2022 at 12:32:44PM +0200, René Scharfe wrote:\n> e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\n> way to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\n> have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\n> that mechanism mandatory.\n> \n> git format-patch only sets no_free when --output is given, causing it to\n> forget pathspecs after the first commit.  Set no_free unconditionally\n> instead.\n\nI remember when I saw the first commit long ago, and thought; well that is\nvery round about way to reintroduce the UNLEAK removal that might have made\nit visible.\n\nHaven't looked too closely, but considering that we were warned[1] the\ninterface was hard to use and might cause problems later and it did.\n\nwouldn't it a better and more secure solution to UNLEAK and remove all this\ncode, at least until it could be refactored cleanly, of course?\n\nCarlo\n\n[1] https://lore.kernel.org/git/YCUFNVj7qlt9wzlX@coredump.intra.peff.net/\n"},{"id":"454688","messageId":"xmqq35hu8feo.fsf@gitster.g","threadId":"57822","inReplyTo":"2c988c7b-0efe-4222-4a43-8124fe1a9da6@web.de","subject":"Re: [PATCH] 2.36 fast-export regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-30T20:50:07Z","receivedAt":"2022-04-30T20:50:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\n> way to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\n> have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\n> that mechanism mandatory.\n>\n> git fast-export doesn't set no_free, so path limiting stopped working\n> after the first commit.  Set the flag and add a basic test to make sure\n> only changes to the specified files are exported.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n\nBoth format-patch fix and this one look good to me.\n\nWe have UNLEAK() in format-patch so that the fix will not result in\nadditional false positives from the leak checker.  But this one, we\ndidn't have UNLEAK() so this may start tickling the leak checker\nagain.  We may need to add UNLEAK() at some point.\n\nStopping a leak checker from complaining about a known singleton\nleak that is at the top-level (i.e. does not become bigger with the\ndata) by spending extra cycles is pointless, compared to using\nUNLEAK() to do the same, and it is doubly misguided if it breaks the\nbehaviour X-<.\n\nWill queue both, thanks.\n"},{"id":"454696","messageId":"6af1aed1-ab13-ee0e-e979-d2f826ec776a@web.de","threadId":"57822","inReplyTo":"20220430163232.ytvwru4fnylow2jk@carlos-mbp.lan","subject":"Re: [PATCH] 2.36 format-patch regression fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-05-01T09:35:16Z","receivedAt":"2022-05-01T09:36:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.04.22 um 18:32 schrieb Carlo Marcelo Arenas Belón:\n> On Sat, Apr 30, 2022 at 12:32:44PM +0200, René Scharfe wrote:\n>> e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\n>> way to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\n>> have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\n>> that mechanism mandatory.\n>>\n>> git format-patch only sets no_free when --output is given, causing it to\n>> forget pathspecs after the first commit.  Set no_free unconditionally\n>> instead.\n>\n> I remember when I saw the first commit long ago, and thought; well that is\n> very round about way to reintroduce the UNLEAK removal that might have made\n> it visible.\n>\n> Haven't looked too closely, but considering that we were warned[1] the\n> interface was hard to use and might cause problems later and it did.\n>\n> wouldn't it a better and more secure solution to UNLEAK and remove all this\n> code, at least until it could be refactored cleanly, of course?\n\nSilently self-destructing pathspecs are a safety hazard indeed.\n\nno_free also affects freeing ignore_regex and parseopts, and even\nclosing the output file.  I don't know about the file, but leaking the\nfirst two is harmless.  So removing the flag is safe as long as we make\nsure the output file is closed as needed.\n\nA safe diff_free() would only be called a particular diffopt once, when\nit's no longer needed.  It could check for reuse by setting a flag the\nfirst time, like in the patch below.  1426 tests in 163 test scripts\nfail for me with it applied on top of the regression fixes from this\nthread.\n\nRemoving the diff_free() calls from diff.c::diff_flush() and\nlog-tree.c::log_tree_commit() reduces this to just one or two in t7527\n(seems to be flaky).  Perhaps this is still salvageable?\n\n>\n> Carlo\n>\n> [1] https://lore.kernel.org/git/YCUFNVj7qlt9wzlX@coredump.intra.peff.net/\n\n\n---\n diff.c | 3 +++\n diff.h | 1 +\n 2 files changed, 4 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex ef7159968b..01296829b5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6458,10 +6458,13 @@ void diff_free(struct diff_options *options)\n \tif (options->no_free)\n \t\treturn;\n\n+\tif (options->is_dead)\n+\t\tBUG(\"double diff_free() on %p\", (void *)options);\n \tdiff_free_file(options);\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n \tFREE_AND_NULL(options->parseopts);\n+\toptions->is_dead = 1;\n }\n\n void diff_flush(struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex 8ae18e5ab1..c31d32ba19 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -398,6 +398,7 @@ struct diff_options {\n \tstruct strmap *additional_path_headers;\n\n \tint no_free;\n+\tint is_dead;\n };\n\n unsigned diff_filter_bit(char status);\n--\n2.35.3\n\n"},{"id":"455570","messageId":"220520.86pmk81a9z.gmgdl@evledraar.gmail.com","threadId":"57822","inReplyTo":"6af1aed1-ab13-ee0e-e979-d2f826ec776a@web.de","subject":"the state of diff_free() and release_revisions() (was: [PATCH] 2.36 format-patch regression fix)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-20T15:23:01Z","receivedAt":"2022-05-20T15:42:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, May 01 2022, René Scharfe wrote:\n\n> Am 30.04.22 um 18:32 schrieb Carlo Marcelo Arenas Belón:\n>> On Sat, Apr 30, 2022 at 12:32:44PM +0200, René Scharfe wrote:\n>>> e900d494dc (diff: add an API for deferred freeing, 2021-02-11) added a\n>>> way to allow reusing diffopts: the no_free bit.  244c27242f (diff.[ch]:\n>>> have diff_free() call clear_pathspec(opts.pathspec), 2022-02-16) made\n>>> that mechanism mandatory.\n>>>\n>>> git format-patch only sets no_free when --output is given, causing it to\n>>> forget pathspecs after the first commit.  Set no_free unconditionally\n>>> instead.\n>>\n>> I remember when I saw the first commit long ago, and thought; well that is\n>> very round about way to reintroduce the UNLEAK removal that might have made\n>> it visible.\n>>\n>> Haven't looked too closely, but considering that we were warned[1] the\n>> interface was hard to use and might cause problems later and it did.\n>>\n>> wouldn't it a better and more secure solution to UNLEAK and remove all this\n>> code, at least until it could be refactored cleanly, of course?\n>\n> Silently self-destructing pathspecs are a safety hazard indeed.\n>\n> no_free also affects freeing ignore_regex and parseopts, and even\n> closing the output file.  I don't know about the file, but leaking the\n> first two is harmless.  So removing the flag is safe as long as we make\n> sure the output file is closed as needed.\n>\n> A safe diff_free() would only be called a particular diffopt once, when\n> it's no longer needed.  It could check for reuse by setting a flag the\n> first time, like in the patch below.  1426 tests in 163 test scripts\n> fail for me with it applied on top of the regression fixes from this\n> thread.\n>\n> Removing the diff_free() calls from diff.c::diff_flush() and\n> log-tree.c::log_tree_commit() reduces this to just one or two in t7527\n> (seems to be flaky).  Perhaps this is still salvageable?\n\nThanks both for handling this, and sorry that I was away at the time.\n\nAFAICT the current status in this area is that with 2cc712324d5 (Merge\nbranch 'rs/fast-export-pathspec-fix', 2022-05-04) and 5048b20d1c2 (Merge\nbranch 'rs/format-patch-pathspec-fix', 2022-05-04) merged the known bugs\nrelated to this have been fixed, along with 3da993f2e63 (Merge branch\n'jc/diff-tree-stdin-fix', 2022-04-28).\n\n\"This\" being my e900d494dcf (diff: add an API for deferred freeing,\n2021-02-11), and 244c27242f4 (diff.[ch]: have diff_free() call\nclear_pathspec(opts.pathspec), 2022-02-16) for the diff-tree case.\n\nNot coincidentally around the same time my ab/plug-leak-in-revisions got\nun-marked for \"next\" from [1] to [2], and I'm looking for a path forward\nfor this whole thing...\n\n1. https://lore.kernel.org/git/xmqqbkwyz78z.fsf@gitster.g/\n2. https://lore.kernel.org/git/xmqqwnfcskw2.fsf@gitster.g/\n\n>> [1] https://lore.kernel.org/git/YCUFNVj7qlt9wzlX@coredump.intra.peff.net/\n>\n>\n> ---\n>  diff.c | 3 +++\n>  diff.h | 1 +\n>  2 files changed, 4 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index ef7159968b..01296829b5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6458,10 +6458,13 @@ void diff_free(struct diff_options *options)\n>  \tif (options->no_free)\n>  \t\treturn;\n>\n> +\tif (options->is_dead)\n> +\t\tBUG(\"double diff_free() on %p\", (void *)options);\n>  \tdiff_free_file(options);\n>  \tdiff_free_ignore_regex(options);\n>  \tclear_pathspec(&options->pathspec);\n>  \tFREE_AND_NULL(options->parseopts);\n> +\toptions->is_dead = 1;\n>  }\n>\n>  void diff_flush(struct diff_options *options)\n> diff --git a/diff.h b/diff.h\n> index 8ae18e5ab1..c31d32ba19 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -398,6 +398,7 @@ struct diff_options {\n>  \tstruct strmap *additional_path_headers;\n>\n>  \tint no_free;\n> +\tint is_dead;\n>  };\n>\n>  unsigned diff_filter_bit(char status);\n\nYes, that's quite scary. It shows that in general diff_free() isn't\nreentrant-safe, but that we do call it repeatedly again.\n\nHowever if we patch it like this instead we can see that (gulp!) we just\nbarely putter along, according to our test coverage at least. I.e. we\ndon't end up calling the parts of it that would be unsafe to call again:\n\t\n\tdiff --git a/diff.c b/diff.c\n\tindex ef7159968b6..0fe8bc5fade 100644\n\t--- a/diff.c\n\t+++ b/diff.c\n\t@@ -6438,14 +6438,23 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n\t \n\t static void diff_free_file(struct diff_options *options)\n\t {\n\t-\tif (options->close_file)\n\t+\tif (options->close_file) {\n\t+\t\tif (options->is_dead)\n\t+\t\t\tBUG(\"double diff_free() on %p\", (void *)options);\n\t \t\tfclose(options->file);\n\t+\t}\n\t }\n\t \n\t static void diff_free_ignore_regex(struct diff_options *options)\n\t {\n\t \tint i;\n\t \n\t+\tif (!options->ignore_regex_nr && !options->ignore_regex)\n\t+\t\treturn;\n\t+\n\t+\tif (options->is_dead)\n\t+\t\tBUG(\"double diff_free() on %p\", (void *)options);\n\t+\n\t \tfor (i = 0; i < options->ignore_regex_nr; i++) {\n\t \t\tregfree(options->ignore_regex[i]);\n\t \t\tfree(options->ignore_regex[i]);\n\t@@ -6462,6 +6471,7 @@ void diff_free(struct diff_options *options)\n\t \tdiff_free_ignore_regex(options);\n\t \tclear_pathspec(&options->pathspec);\n\t \tFREE_AND_NULL(options->parseopts);\n\t+\toptions->is_dead = 1;\n\t }\n\t \n\t void diff_flush(struct diff_options *options)\n\t@@ -6560,7 +6570,6 @@ void diff_flush(struct diff_options *options)\n\t free_queue:\n\t \tfree(q->queue);\n\t \tDIFF_QUEUE_CLEAR(q);\n\t-\tdiff_free(options);\n\t \n\t \t/*\n\t \t * Report the content-level differences with HAS_CHANGES;\n\tdiff --git a/diff.h b/diff.h\n\tindex 8ae18e5ab1e..c31d32ba192 100644\n\t--- a/diff.h\n\t+++ b/diff.h\n\t@@ -398,6 +398,7 @@ struct diff_options {\n\t \tstruct strmap *additional_path_headers;\n\t \n\t \tint no_free;\n\t+\tint is_dead;\n\t };\n\t \n\t unsigned diff_filter_bit(char status);\n\nI'd really like to fix this properly, but AFAICT the best way to do that\nis to:\n\n A. Get ab/plug-leak-in-revisions merged down\n B. Fix diff_free() on top of that\n\nBefore I knew of these bugs I'd already written patches to get rid of\nthat whole \"no_free\" business. In retrospect it was completely the wrong\nthing to do, but in hindsight something like it was needed to fix those\nleaks as long as we didn't have a revisions_release().\n\nI.e. the tricky cases where I ended up needing to set \"no_free\" are ones\nwhere all the complexity neatly goes away once we start releasing the\n\"struct rev_info\" properly, as it contains the data we'd like to\ndiff_free() at the end.\n\nHow does that plan sound, and is there anything I've missed?\n\nI could also re-roll ab/plug-leak-in-revisions to include a fix that\nmakes it safe in the interim, i.e.:\n\n\tdiff --git a/diff.c b/diff.c\n\tindex ef7159968b6..2bc7ee81e4e 100644\n\t--- a/diff.c\n\t+++ b/diff.c\n\t@@ -6438,8 +6438,12 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n\t \n\t static void diff_free_file(struct diff_options *options)\n\t {\n\t-\tif (options->close_file)\n\t+\tif (options->close_file) {\n\t \t\tfclose(options->file);\n\t+\n\t+\t\toptions->file = NULL;\n\t+\t\toptions->close_file = 0;\n\t+\t}\n\t }\n\t \n\t static void diff_free_ignore_regex(struct diff_options *options)\n\t@@ -6450,7 +6454,8 @@ static void diff_free_ignore_regex(struct diff_options *options)\n\t \t\tregfree(options->ignore_regex[i]);\n\t \t\tfree(options->ignore_regex[i]);\n\t \t}\n\t-\tfree(options->ignore_regex);\n\t+\toptions->ignore_regex_nr = 0;\n\t+\tFREE_AND_NULL(options->ignore_regex);\n\t }\n\t \n\t void diff_free(struct diff_options *options)\n\nBut as long as we're not adding new API users of it until the follow-up\nafter ab/plug-leak-in-revisions we should also be safe for now, but\nperhaps it's prudent to do it anyway.\n\nI *could* potentially produce a shorter series than\nab/plug-leak-in-revisions to narrowly try to remove \"no_free\" from\ndiff.c first, but it would basically need to first introduce a\nrelease_revisions(), and without the other revisions API leaks being\nfixed testing it would be much tricker. I'd really prefer not to do\nthat.\n\nHow does all that sound?\n"},{"id":"455578","messageId":"xmqqczg8jeya.fsf@gitster.g","threadId":"57822","inReplyTo":"220520.86pmk81a9z.gmgdl@evledraar.gmail.com","subject":"Re: the state of diff_free() and release_revisions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-20T17:23:25Z","receivedAt":"2022-05-20T17:23:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> AFAICT the current status in this area is that with 2cc712324d5 (Merge\n> branch 'rs/fast-export-pathspec-fix', 2022-05-04) and 5048b20d1c2 (Merge\n> branch 'rs/format-patch-pathspec-fix', 2022-05-04) merged the known bugs\n> related to this have been fixed, along with 3da993f2e63 (Merge branch\n> 'jc/diff-tree-stdin-fix', 2022-04-28).\n\nThere is another possible known breakage around \"diff -I --cc\"\nreported, no?\n\nhttps://lore.kernel.org/git/a6a14213-bc82-d6fb-43dd-5a423c40a4f8@web.de/\n\n> Not coincidentally around the same time my ab/plug-leak-in-revisions got\n> un-marked for \"next\" from [1] to [2], and I'm looking for a path forward\n> for this whole thing...\n\nI think this was not because it had known bugs but only because we\nwanted to avoid distractions and too much churns in the tree.\n\nI think it is a good time to reactivate it; Phillip Wood took a\nserious look at the latest round, if I am reading [*] correctly.\n\n* https://lore.kernel.org/git/2cc2d026-1496-1ed9-838b-6fdf8cfba285@gmail.com/\n\nThanks.\n\n"}]}