{"thread":{"id":"61922","subject":"[PATCH] diff-tree: fix crash when used with --remerge-diff","startedAt":"2024-08-08T13:20:54Z","lastAt":"2024-08-09T15:46:11Z","messageCount":7,"participants":["blanet via GitGitGadget","Elijah Newren","Junio C Hamano","Xing Xin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"500463","messageId":"pull.1771.git.1723123250958.gitgitgadget@gmail.com","threadId":"61922","inReplyTo":null,"subject":"[PATCH] diff-tree: fix crash when used with --remerge-diff","fromName":"blanet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-08T13:20:50Z","receivedAt":"2024-08-08T13:20:54Z","isPatch":true,"sender":{"key":"name:blanet","avatar":null},"body":"From: Xing Xin <xingxin.xx@bytedance.com>\n\nWhen using \"git-diff-tree\" to get the tree diff for merge commits with\nthe diff format set to `remerge`, a bug is triggered as shown below:\n\n    $ git diff-tree -r --remerge-diff 363337e6eb\n    363337e6eb812d0c0d785ed4261544f35559ff8b\n    BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n\nThis bug is reported by `log-tree.c:do_remerge_diff`, where a bug check\nadded in commit 7b90ab467a (log: clean unneeded objects during log\n--remerge-diff, 2022-02-02) detects the absence of `remerge_objdir` when\nattempting to clean up temporary objects generated during the remerge\nprocess.\n\nAfter some further digging, I find that the remerge-related diff options\nwere introduced in db757e8b8d (show, log: provide a --remerge-diff\ncapability, 2022-02-02), which also affect the setup of `rev_info` for\n\"git-diff-tree\", but were not accounted for in the original\nimplementation (inferred from the commit message).\n\nThis commit fixes the bug by adding initialization logic for\n`remerge_objdir` in `builtin/diff-tree.c`, mirroring the logic in\n`builtin/log.c:cmd_log_walk_no_free`. A final cleanup for\n`remerge_objdir` is also included.\n\nSigned-off-by: Xing Xin <xingxin.xx@bytedance.com>\n---\n    diff-tree: fix crash when used with --remerge-diff\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1771\n\n builtin/diff-tree.c     | 13 +++++++++++++\n t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 0d3c611aac0..813be486dad 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -9,6 +9,7 @@\n #include \"repository.h\"\n #include \"revision.h\"\n #include \"tree.h\"\n+#include \"tmp-objdir.h\"\n \n static struct rev_info log_tree_opt;\n \n@@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \topt->diffopt.rotate_to_strict = 1;\n \n+\tif (opt->remerge_diff) {\n+\t\topt->remerge_objdir = tmp_objdir_create(\"remerge-diff\");\n+\t\tif (!opt->remerge_objdir)\n+\t\t\tdie(_(\"unable to create temporary object directory\"));\n+\t\ttmp_objdir_replace_primary_odb(opt->remerge_objdir, 1);\n+\t}\n+\n \t/*\n \t * NOTE!  We expect \"a..b\" to expand to \"^a b\" but it is\n \t * perfectly valid for revision range parser to yield \"b ^a\",\n@@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdiff_free(&opt->diffopt);\n \t}\n \n+\tif (opt->remerge_diff) {\n+\t\ttmp_objdir_destroy(opt->remerge_objdir);\n+\t\topt->remerge_objdir = NULL;\n+\t}\n+\n \treturn diff_result_code(&opt->diffopt);\n }\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 07323ebafe0..ca8f999caba 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -110,6 +110,41 @@ test_expect_success 'can filter out additional headers with pickaxe' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'remerge-diff also works for git-diff-tree' '\n+\t# With a clean merge\n+\tgit diff-tree -r -p --remerge-diff --no-commit-id bc_resolution >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# With both a resolved conflict and an unrelated change\n+\tcat <<-EOF >tmp &&\n+\tdiff --git a/numbers b/numbers\n+\tremerge CONFLICT (content): Merge conflict in numbers\n+\tindex a1fb731..6875544 100644\n+\t--- a/numbers\n+\t+++ b/numbers\n+\t@@ -1,13 +1,9 @@\n+\t 1\n+\t 2\n+\t-<<<<<<< b0ed5cb (change_a)\n+\t-three\n+\t-=======\n+\t-tres\n+\t->>>>>>> 6cd3f82 (change_b)\n+\t+drei\n+\t 4\n+\t 5\n+\t 6\n+\t 7\n+\t-eight\n+\t+acht\n+\t 9\n+\tEOF\n+\tsed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >expect &&\n+\tgit diff-tree -r -p --remerge-diff --no-commit-id ab_resolution >tmp &&\n+\tsed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup non-content conflicts' '\n \tgit switch --orphan base &&\n \n\nbase-commit: 406f326d271e0bacecdb00425422c5fa3f314930\n-- \ngitgitgadget\n"},{"id":"500483","messageId":"CABPp-BGo7-P+3w=Y2Mifox4xztzMhgLKBtnrrF9R1XM9ZDPqqw@mail.gmail.com","threadId":"61922","inReplyTo":"pull.1771.git.1723123250958.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff-tree: fix crash when used with --remerge-diff","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-08-08T16:03:53Z","receivedAt":"2024-08-08T16:04:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 8, 2024 at 6:20 AM blanet via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Xing Xin <xingxin.xx@bytedance.com>\n>\n> When using \"git-diff-tree\" to get the tree diff for merge commits with\n> the diff format set to `remerge`, a bug is triggered as shown below:\n>\n>     $ git diff-tree -r --remerge-diff 363337e6eb\n>     363337e6eb812d0c0d785ed4261544f35559ff8b\n>     BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n\nWow, this bug is around for 2.5 years, and then we both independently\nnotice and fix it within 3 weeks of each other:\nhttps://github.com/git/git/commit/e5890667c7598e813edee0ac4e76d6e3cdd525ec\n\nMy patch is incomplete as it's missing a testcase, and you submitted\nfirst, so let's stick with your fix, though.\n\n> This bug is reported by `log-tree.c:do_remerge_diff`, where a bug check\n> added in commit 7b90ab467a (log: clean unneeded objects during log\n> --remerge-diff, 2022-02-02) detects the absence of `remerge_objdir` when\n> attempting to clean up temporary objects generated during the remerge\n> process.\n>\n> After some further digging, I find that the remerge-related diff options\n> were introduced in db757e8b8d (show, log: provide a --remerge-diff\n> capability, 2022-02-02), which also affect the setup of `rev_info` for\n> \"git-diff-tree\", but were not accounted for in the original\n> implementation (inferred from the commit message).\n>\n> This commit fixes the bug by adding initialization logic for\n> `remerge_objdir` in `builtin/diff-tree.c`, mirroring the logic in\n> `builtin/log.c:cmd_log_walk_no_free`. A final cleanup for\n> `remerge_objdir` is also included.\n\nThe commit message from my patch also included an explanation for why\ndiff-tree was the only caller that was missing the necessary logic\n(see the last paragraph, which kind of references the one before it as\nwell).\n\n> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>\n> ---\n>     diff-tree: fix crash when used with --remerge-diff\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1771\n>\n>  builtin/diff-tree.c     | 13 +++++++++++++\n>  t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++\n>  2 files changed, 48 insertions(+)\n>\n> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> index 0d3c611aac0..813be486dad 100644\n> --- a/builtin/diff-tree.c\n> +++ b/builtin/diff-tree.c\n> @@ -9,6 +9,7 @@\n>  #include \"repository.h\"\n>  #include \"revision.h\"\n>  #include \"tree.h\"\n> +#include \"tmp-objdir.h\"\n\nThe includes other than this one are in alphabetical order; can you\nmove this a line before?\n\nAlso, as an aside, folks in this project often just put includes at\nthe end, but I think it's a bad practice.  Whenever someone needs to\nbackport fixes or merge separate patch topics into seen/next/etc. or\neven merge not-yet-upstream topics with newer upstream versions, this\npractice increases the odds of unnecessary conflicts.  And it makes it\nharder for the next person who comes along to spot whether a header is\nalready included (and sometimes leaves us including headers twice).\nWhile each case is a small amount of toil so we tend to overlook it,\nit's totally unnecessary toil in many cases.  Putting includes in\nalphabetical order (other than the one include required to be first,\ngit-compat-util.h or its documented stand-ins) can often remove this\nunnecessary toil.  Anyway, thanks for letting me vent.  :-)\n\n>  static struct rev_info log_tree_opt;\n>\n> @@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>\n>         opt->diffopt.rotate_to_strict = 1;\n>\n> +       if (opt->remerge_diff) {\n> +               opt->remerge_objdir = tmp_objdir_create(\"remerge-diff\");\n> +               if (!opt->remerge_objdir)\n> +                       die(_(\"unable to create temporary object directory\"));\n> +               tmp_objdir_replace_primary_odb(opt->remerge_objdir, 1);\n> +       }\n> +\n>         /*\n>          * NOTE!  We expect \"a..b\" to expand to \"^a b\" but it is\n>          * perfectly valid for revision range parser to yield \"b ^a\",\n> @@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>                 diff_free(&opt->diffopt);\n>         }\n>\n> +       if (opt->remerge_diff) {\n> +               tmp_objdir_destroy(opt->remerge_objdir);\n> +               opt->remerge_objdir = NULL;\n> +       }\n> +\n>         return diff_result_code(&opt->diffopt);\n>  }\n\nYour fix exactly matches mine, other than the header include location.\n\n> diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\n> index 07323ebafe0..ca8f999caba 100755\n> --- a/t/t4069-remerge-diff.sh\n> +++ b/t/t4069-remerge-diff.sh\n> @@ -110,6 +110,41 @@ test_expect_success 'can filter out additional headers with pickaxe' '\n>         test_must_be_empty actual\n>  '\n>\n> +test_expect_success 'remerge-diff also works for git-diff-tree' '\n> +       # With a clean merge\n> +       git diff-tree -r -p --remerge-diff --no-commit-id bc_resolution >actual &&\n> +       test_must_be_empty actual &&\n> +\n> +       # With both a resolved conflict and an unrelated change\n> +       cat <<-EOF >tmp &&\n> +       diff --git a/numbers b/numbers\n> +       remerge CONFLICT (content): Merge conflict in numbers\n> +       index a1fb731..6875544 100644\n> +       --- a/numbers\n> +       +++ b/numbers\n> +       @@ -1,13 +1,9 @@\n> +        1\n> +        2\n> +       -<<<<<<< b0ed5cb (change_a)\n> +       -three\n> +       -=======\n> +       -tres\n> +       ->>>>>>> 6cd3f82 (change_b)\n> +       +drei\n> +        4\n> +        5\n> +        6\n> +        7\n> +       -eight\n> +       +acht\n> +        9\n> +       EOF\n> +       sed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >expect &&\n> +       git diff-tree -r -p --remerge-diff --no-commit-id ab_resolution >tmp &&\n> +       sed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_expect_success 'setup non-content conflicts' '\n>         git switch --orphan base &&\n\nTest looks good too.\n\nI'll be happy to add my Reviewed-by if you fix the header include order.\n"},{"id":"500510","messageId":"xmqqr0azgc7j.fsf@gitster.g","threadId":"61922","inReplyTo":"CABPp-BGo7-P+3w=Y2Mifox4xztzMhgLKBtnrrF9R1XM9ZDPqqw@mail.gmail.com","subject":"Re: [PATCH] diff-tree: fix crash when used with --remerge-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-08T17:24:00Z","receivedAt":"2024-08-08T17:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> The commit message from my patch also included an explanation for why\n> diff-tree was the only caller that was missing the necessary logic\n> (see the last paragraph, which kind of references the one before it as\n> well).\n\n... which we may want to resurrect.\n\n> Test looks good too.\n>\n> I'll be happy to add my Reviewed-by if you fix the header include order.\n\nThanks for a review.\n"},{"id":"500531","messageId":"pull.1771.v2.git.1723188292498.gitgitgadget@gmail.com","threadId":"61922","inReplyTo":"pull.1771.git.1723123250958.gitgitgadget@gmail.com","subject":"[PATCH v2] diff-tree: fix crash when used with --remerge-diff","fromName":"blanet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-09T07:24:52Z","receivedAt":"2024-08-09T07:24:56Z","isPatch":true,"sender":{"key":"name:blanet","avatar":null},"body":"From: Xing Xin <xingxin.xx@bytedance.com>\n\nWhen using \"git-diff-tree\" to get the tree diff for merge commits with\nthe diff format set to `remerge`, a bug is triggered as shown below:\n\n  $ git diff-tree -r --remerge-diff 363337e6eb\n  363337e6eb812d0c0d785ed4261544f35559ff8b\n  BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n\nThis bug is reported by `log-tree.c:do_remerge_diff`, where a bug check\nadded in commit 7b90ab467a (log: clean unneeded objects during log\n--remerge-diff, 2022-02-02) detects the absence of `remerge_objdir` when\nattempting to clean up temporary objects generated during the remerge\nprocess.\n\nAfter some further digging, I find that the remerge-related diff options\nwere introduced in db757e8b8d (show, log: provide a --remerge-diff\ncapability, 2022-02-02), which also affect the setup of `rev_info` for\n\"git-diff-tree\", but were not accounted for in the original\nimplementation (inferred from the commit message).\n\nElijah Newren, the author of the remerge diff feature, notes that other\ncallers of `log-tree.c:log_tree_commit` (the only caller of\n`log-tree.c:do_remerge_diff`) also exist, but:\n\n  `builtin/am.c`: manually sets all flags; remerge_diff is not among them\n  `sequencer.c`: manually sets all flags; remerge_diff is not among them\n\nso `builtin/diff-tree.c` really is the only caller that was overlooked\nwhen remerge-diff functionality was added.\n\nThis commit resolves the crash by adding `remerge_objdir` setup logic to\n`builtin/diff-tree.c`, mirroring `builtin/log.c:cmd_log_walk_no_free`.\nIt also includes the necessary cleanup for `remerge_objdir`.\n\nReviewed-by: Elijah Newren <newren@gmail.com>\nSigned-off-by: Xing Xin <xingxin.xx@bytedance.com>\n---\n    diff-tree: fix crash when used with --remerge-diff\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1771\n\nRange-diff vs v1:\n\n 1:  f0b86faa275 ! 1:  57f0b1247d8 diff-tree: fix crash when used with --remerge-diff\n     @@ Commit message\n          When using \"git-diff-tree\" to get the tree diff for merge commits with\n          the diff format set to `remerge`, a bug is triggered as shown below:\n      \n     -        $ git diff-tree -r --remerge-diff 363337e6eb\n     -        363337e6eb812d0c0d785ed4261544f35559ff8b\n     -        BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n     +      $ git diff-tree -r --remerge-diff 363337e6eb\n     +      363337e6eb812d0c0d785ed4261544f35559ff8b\n     +      BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n      \n          This bug is reported by `log-tree.c:do_remerge_diff`, where a bug check\n          added in commit 7b90ab467a (log: clean unneeded objects during log\n     @@ Commit message\n          \"git-diff-tree\", but were not accounted for in the original\n          implementation (inferred from the commit message).\n      \n     -    This commit fixes the bug by adding initialization logic for\n     -    `remerge_objdir` in `builtin/diff-tree.c`, mirroring the logic in\n     -    `builtin/log.c:cmd_log_walk_no_free`. A final cleanup for\n     -    `remerge_objdir` is also included.\n     +    Elijah Newren, the author of the remerge diff feature, notes that other\n     +    callers of `log-tree.c:log_tree_commit` (the only caller of\n     +    `log-tree.c:do_remerge_diff`) also exist, but:\n      \n     +      `builtin/am.c`: manually sets all flags; remerge_diff is not among them\n     +      `sequencer.c`: manually sets all flags; remerge_diff is not among them\n     +\n     +    so `builtin/diff-tree.c` really is the only caller that was overlooked\n     +    when remerge-diff functionality was added.\n     +\n     +    This commit resolves the crash by adding `remerge_objdir` setup logic to\n     +    `builtin/diff-tree.c`, mirroring `builtin/log.c:cmd_log_walk_no_free`.\n     +    It also includes the necessary cleanup for `remerge_objdir`.\n     +\n     +    Reviewed-by: Elijah Newren <newren@gmail.com>\n          Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>\n      \n       ## builtin/diff-tree.c ##\n      @@\n     + #include \"read-cache-ll.h\"\n       #include \"repository.h\"\n       #include \"revision.h\"\n     - #include \"tree.h\"\n      +#include \"tmp-objdir.h\"\n     + #include \"tree.h\"\n       \n       static struct rev_info log_tree_opt;\n     - \n      @@ builtin/diff-tree.c: int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n       \n       \topt->diffopt.rotate_to_strict = 1;\n\n\n builtin/diff-tree.c     | 13 +++++++++++++\n t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 0d3c611aac0..b8df1d4b79b 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -8,6 +8,7 @@\n #include \"read-cache-ll.h\"\n #include \"repository.h\"\n #include \"revision.h\"\n+#include \"tmp-objdir.h\"\n #include \"tree.h\"\n \n static struct rev_info log_tree_opt;\n@@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \topt->diffopt.rotate_to_strict = 1;\n \n+\tif (opt->remerge_diff) {\n+\t\topt->remerge_objdir = tmp_objdir_create(\"remerge-diff\");\n+\t\tif (!opt->remerge_objdir)\n+\t\t\tdie(_(\"unable to create temporary object directory\"));\n+\t\ttmp_objdir_replace_primary_odb(opt->remerge_objdir, 1);\n+\t}\n+\n \t/*\n \t * NOTE!  We expect \"a..b\" to expand to \"^a b\" but it is\n \t * perfectly valid for revision range parser to yield \"b ^a\",\n@@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdiff_free(&opt->diffopt);\n \t}\n \n+\tif (opt->remerge_diff) {\n+\t\ttmp_objdir_destroy(opt->remerge_objdir);\n+\t\topt->remerge_objdir = NULL;\n+\t}\n+\n \treturn diff_result_code(&opt->diffopt);\n }\ndiff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh\nindex 07323ebafe0..ca8f999caba 100755\n--- a/t/t4069-remerge-diff.sh\n+++ b/t/t4069-remerge-diff.sh\n@@ -110,6 +110,41 @@ test_expect_success 'can filter out additional headers with pickaxe' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'remerge-diff also works for git-diff-tree' '\n+\t# With a clean merge\n+\tgit diff-tree -r -p --remerge-diff --no-commit-id bc_resolution >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\t# With both a resolved conflict and an unrelated change\n+\tcat <<-EOF >tmp &&\n+\tdiff --git a/numbers b/numbers\n+\tremerge CONFLICT (content): Merge conflict in numbers\n+\tindex a1fb731..6875544 100644\n+\t--- a/numbers\n+\t+++ b/numbers\n+\t@@ -1,13 +1,9 @@\n+\t 1\n+\t 2\n+\t-<<<<<<< b0ed5cb (change_a)\n+\t-three\n+\t-=======\n+\t-tres\n+\t->>>>>>> 6cd3f82 (change_b)\n+\t+drei\n+\t 4\n+\t 5\n+\t 6\n+\t 7\n+\t-eight\n+\t+acht\n+\t 9\n+\tEOF\n+\tsed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >expect &&\n+\tgit diff-tree -r -p --remerge-diff --no-commit-id ab_resolution >tmp &&\n+\tsed -e \"s/[0-9a-f]\\{7,\\}/HASH/g\" tmp >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup non-content conflicts' '\n \tgit switch --orphan base &&\n \n\nbase-commit: 406f326d271e0bacecdb00425422c5fa3f314930\n-- \ngitgitgadget\n"},{"id":"500532","messageId":"4bc36fb9.5f47.19136073c64.Coremail.bupt_xingxin@163.com","threadId":"61922","inReplyTo":"CABPp-BGo7-P+3w=Y2Mifox4xztzMhgLKBtnrrF9R1XM9ZDPqqw@mail.gmail.com","subject":"Re:Re: [PATCH] diff-tree: fix crash when used with --remerge-diff","fromName":"Xing Xin","fromEmail":"bupt_xingxin@163.com","sentAt":"2024-08-09T07:25:29Z","receivedAt":"2024-08-09T07:25:39Z","isPatch":true,"sender":{"key":"bupt_xingxin@163.com","avatar":null},"body":"At 2024-08-09 00:03:53, \"Elijah Newren\" <newren@gmail.com> wrote:\n>On Thu, Aug 8, 2024 at 6:20 AM blanet via GitGitGadget\n><gitgitgadget@gmail.com> wrote:\n>>\n>> From: Xing Xin <xingxin.xx@bytedance.com>\n>>\n>> When using \"git-diff-tree\" to get the tree diff for merge commits with\n>> the diff format set to `remerge`, a bug is triggered as shown below:\n>>\n>>     $ git diff-tree -r --remerge-diff 363337e6eb\n>>     363337e6eb812d0c0d785ed4261544f35559ff8b\n>>     BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?\n>\n>Wow, this bug is around for 2.5 years, and then we both independently\n>notice and fix it within 3 weeks of each other:\n>https://github.com/git/git/commit/e5890667c7598e813edee0ac4e76d6e3cdd525ec\n>\n>My patch is incomplete as it's missing a testcase, and you submitted\n>first, so let's stick with your fix, though.\n\nWow, such an interesting coincidence! And thanks for your quick reply!\n\n>> This bug is reported by `log-tree.c:do_remerge_diff`, where a bug check\n>> added in commit 7b90ab467a (log: clean unneeded objects during log\n>> --remerge-diff, 2022-02-02) detects the absence of `remerge_objdir` when\n>> attempting to clean up temporary objects generated during the remerge\n>> process.\n>>\n>> After some further digging, I find that the remerge-related diff options\n>> were introduced in db757e8b8d (show, log: provide a --remerge-diff\n>> capability, 2022-02-02), which also affect the setup of `rev_info` for\n>> \"git-diff-tree\", but were not accounted for in the original\n>> implementation (inferred from the commit message).\n>>\n>> This commit fixes the bug by adding initialization logic for\n>> `remerge_objdir` in `builtin/diff-tree.c`, mirroring the logic in\n>> `builtin/log.c:cmd_log_walk_no_free`. A final cleanup for\n>> `remerge_objdir` is also included.\n>\n>The commit message from my patch also included an explanation for why\n>diff-tree was the only caller that was missing the necessary logic\n>(see the last paragraph, which kind of references the one before it as\n>well).\n\nYour explanations better illustrate the impact of this bug,  I've quoted them\nin the new patch commit message.\n\n[snip]\n\n>> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n>> index 0d3c611aac0..813be486dad 100644\n>> --- a/builtin/diff-tree.c\n>> +++ b/builtin/diff-tree.c\n>> @@ -9,6 +9,7 @@\n>>  #include \"repository.h\"\n>>  #include \"revision.h\"\n>>  #include \"tree.h\"\n>> +#include \"tmp-objdir.h\"\n>\n>The includes other than this one are in alphabetical order; can you\n>move this a line before?\n>\n>Also, as an aside, folks in this project often just put includes at\n>the end, but I think it's a bad practice.  Whenever someone needs to\n>backport fixes or merge separate patch topics into seen/next/etc. or\n>even merge not-yet-upstream topics with newer upstream versions, this\n>practice increases the odds of unnecessary conflicts.  And it makes it\n>harder for the next person who comes along to spot whether a header is\n>already included (and sometimes leaves us including headers twice).\n>While each case is a small amount of toil so we tend to overlook it,\n>it's totally unnecessary toil in many cases.  Putting includes in\n>alphabetical order (other than the one include required to be first,\n>git-compat-util.h or its documented stand-ins) can often remove this\n>unnecessary toil.  Anyway, thanks for letting me vent.  :-)\n\nNoted. I'll move the new include to the correct position. Thanks for the\nguidance!\n\n>>  static struct rev_info log_tree_opt;\n>>\n>> @@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>>\n>>         opt->diffopt.rotate_to_strict = 1;\n>>\n>> +       if (opt->remerge_diff) {\n>> +               opt->remerge_objdir = tmp_objdir_create(\"remerge-diff\");\n>> +               if (!opt->remerge_objdir)\n>> +                       die(_(\"unable to create temporary object directory\"));\n>> +               tmp_objdir_replace_primary_odb(opt->remerge_objdir, 1);\n>> +       }\n>> +\n>>         /*\n>>          * NOTE!  We expect \"a..b\" to expand to \"^a b\" but it is\n>>          * perfectly valid for revision range parser to yield \"b ^a\",\n>> @@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>>                 diff_free(&opt->diffopt);\n>>         }\n>>\n>> +       if (opt->remerge_diff) {\n>> +               tmp_objdir_destroy(opt->remerge_objdir);\n>> +               opt->remerge_objdir = NULL;\n>> +       }\n>> +\n>>         return diff_result_code(&opt->diffopt);\n>>  }\n>\n>Your fix exactly matches mine, other than the header include location.\n\nHigh five! :-)\n\n[snip]\n\nXing Xin\n"},{"id":"500541","messageId":"xmqq5xs9g19w.fsf@gitster.g","threadId":"61922","inReplyTo":"pull.1771.v2.git.1723188292498.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] diff-tree: fix crash when used with --remerge-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T15:32:27Z","receivedAt":"2024-08-09T15:32:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"blanet via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Elijah Newren, the author of the remerge diff feature, notes that other\n> callers of `log-tree.c:log_tree_commit` (the only caller of\n> `log-tree.c:do_remerge_diff`) also exist, but:\n>\n>   `builtin/am.c`: manually sets all flags; remerge_diff is not among them\n>   `sequencer.c`: manually sets all flags; remerge_diff is not among them\n>\n> so `builtin/diff-tree.c` really is the only caller that was overlooked\n> when remerge-diff functionality was added.\n\nThat is more than OK as a band-aid, and I'll take the patch as-is,\nbut I have to wonder if we do even better in a future follow-up\npatch.\n\nAny time do_remerge_diff() is entered, we know that either the end\nuser (from the command line) or the hard-coded caller (like\nam/sequencer cited above) wants us to do the remerge-diff, which in\nturn requires us to have the temporary object directory rotated into\nthe status of the primary object store.  And there is nothing in\nthat object directory rotation code that requires caller-specific\ncustomization---it is the same \"create remerge-diff directory as\ntmp-objdir, rotate it into the alt object store chain as the\nprimary\" regardless of the actual caller).\n\nSo wouldn't it work well if we\n\n (1) at the beginning of do_remerge_diff(), only once for a rev_info\n     structure:\n   (1-a) lazily do the \"object directory rotation\"\n   (1-b) set up an atexit handler to clear the temporary object\n         store\n (2) remove all the \"ah, we need to prepare and tear down the\n     temporary object store for _this_ operation\" we have sprinkled\n     in different code paths (including the one added by the fix we\n     are looking at).\n\nThat way, we won't have to worry about adding future remerge_diff\nusers, including existing hard-coded callers.\n\nANyway, thanks for the fix.  It is very pleasing to see contributors\nworking well together.\n"},{"id":"500546","messageId":"xmqqy155em2n.fsf@gitster.g","threadId":"61922","inReplyTo":"xmqq5xs9g19w.fsf@gitster.g","subject":"Re: [PATCH v2] diff-tree: fix crash when used with --remerge-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-09T15:46:08Z","receivedAt":"2024-08-09T15:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> but I have to wonder if we do even better in a future follow-up\n> patch.\n\n\"if we do\" -> \"if we can do\".\n\n> So wouldn't it work well if we\n>\n>  (1) at the beginning of do_remerge_diff(), only once for a rev_info\n>      structure:\n>    (1-a) lazily do the \"object directory rotation\"\n>    (1-b) set up an atexit handler to clear the temporary object\n>          store\n\nAn atexit handler may not be enough, when a program wants to start\ncreating a real object after we did a remerge-diff but before\nexiting. So we'd probably need to allow an explicit \"ok, we are\ndone\" clean-up call for such a program, too.\n\nAnd the atexit handler can call the same clean-up function if the\nprogram hasn't called it explicitly.  For logically read-only\noperations like diff-tree, they do not have to worry about rotating\nthe real object store back to the primary status as soon as\npossible.\n\n"}]}