From: Xing Xin <bupt_xingxin@163.com>
Date: Fri, 09 Aug 2024 07:25:29 GMT
Subject: Re:Re: [PATCH] diff-tree: fix crash when used with --remerge-diff
Message-ID: <4bc36fb9.5f47.19136073c64.Coremail.bupt_xingxin@163.com>
In-Reply-To: <CABPp-BGo7-P+3w=Y2Mifox4xztzMhgLKBtnrrF9R1XM9ZDPqqw@mail.gmail.com>

At 2024-08-09 00:03:53, "Elijah Newren" <newren@gmail.com> wrote:
>On Thu, Aug 8, 2024 at 6:20 AM blanet via GitGitGadget
><gitgitgadget@gmail.com> wrote:
>>
>> From: Xing Xin <xingxin.xx@bytedance.com>
>>
>> When using "git-diff-tree" to get the tree diff for merge commits with
>> the diff format set to `remerge`, a bug is triggered as shown below:
>>
>>     $ git diff-tree -r --remerge-diff 363337e6eb
>>     363337e6eb812d0c0d785ed4261544f35559ff8b
>>     BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?
>
>Wow, this bug is around for 2.5 years, and then we both independently
>notice and fix it within 3 weeks of each other:
>https://github.com/git/git/commit/e5890667c7598e813edee0ac4e76d6e3cdd525ec
>
>My patch is incomplete as it's missing a testcase, and you submitted
>first, so let's stick with your fix, though.

Wow, such an interesting coincidence! And thanks for your quick reply!

>> This bug is reported by `log-tree.c:do_remerge_diff`, where a bug check
>> added in commit 7b90ab467a (log: clean unneeded objects during log
>> --remerge-diff, 2022-02-02) detects the absence of `remerge_objdir` when
>> attempting to clean up temporary objects generated during the remerge
>> process.
>>
>> After some further digging, I find that the remerge-related diff options
>> were introduced in db757e8b8d (show, log: provide a --remerge-diff
>> capability, 2022-02-02), which also affect the setup of `rev_info` for
>> "git-diff-tree", but were not accounted for in the original
>> implementation (inferred from the commit message).
>>
>> This commit fixes the bug by adding initialization logic for
>> `remerge_objdir` in `builtin/diff-tree.c`, mirroring the logic in
>> `builtin/log.c:cmd_log_walk_no_free`. A final cleanup for
>> `remerge_objdir` is also included.
>
>The commit message from my patch also included an explanation for why
>diff-tree was the only caller that was missing the necessary logic
>(see the last paragraph, which kind of references the one before it as
>well).

Your explanations better illustrate the impact of this bug,  I've quoted them
in the new patch commit message.

[snip]

>> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
>> index 0d3c611aac0..813be486dad 100644
>> --- a/builtin/diff-tree.c
>> +++ b/builtin/diff-tree.c
>> @@ -9,6 +9,7 @@
>>  #include "repository.h"
>>  #include "revision.h"
>>  #include "tree.h"
>> +#include "tmp-objdir.h"
>
>The includes other than this one are in alphabetical order; can you
>move this a line before?
>
>Also, as an aside, folks in this project often just put includes at
>the end, but I think it's a bad practice.  Whenever someone needs to
>backport fixes or merge separate patch topics into seen/next/etc. or
>even merge not-yet-upstream topics with newer upstream versions, this
>practice increases the odds of unnecessary conflicts.  And it makes it
>harder for the next person who comes along to spot whether a header is
>already included (and sometimes leaves us including headers twice).
>While each case is a small amount of toil so we tend to overlook it,
>it's totally unnecessary toil in many cases.  Putting includes in
>alphabetical order (other than the one include required to be first,
>git-compat-util.h or its documented stand-ins) can often remove this
>unnecessary toil.  Anyway, thanks for letting me vent.  :-)

Noted. I'll move the new include to the correct position. Thanks for the
guidance!

>>  static struct rev_info log_tree_opt;
>>
>> @@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>>
>>         opt->diffopt.rotate_to_strict = 1;
>>
>> +       if (opt->remerge_diff) {
>> +               opt->remerge_objdir = tmp_objdir_create("remerge-diff");
>> +               if (!opt->remerge_objdir)
>> +                       die(_("unable to create temporary object directory"));
>> +               tmp_objdir_replace_primary_odb(opt->remerge_objdir, 1);
>> +       }
>> +
>>         /*
>>          * NOTE!  We expect "a..b" to expand to "^a b" but it is
>>          * perfectly valid for revision range parser to yield "b ^a",
>> @@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>>                 diff_free(&opt->diffopt);
>>         }
>>
>> +       if (opt->remerge_diff) {
>> +               tmp_objdir_destroy(opt->remerge_objdir);
>> +               opt->remerge_objdir = NULL;
>> +       }
>> +
>>         return diff_result_code(&opt->diffopt);
>>  }
>
>Your fix exactly matches mine, other than the header include location.

High five! :-)

[snip]

Xing Xin