Re: [PATCH v2] diff-tree: fix crash when used with --remerge-diff

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2024, 15:32 UTC
Message-ID: <xmqq5xs9g19w.fsf@gitster.g>
In-Reply-To: <pull.1771.v2.git.1723188292498.gitgitgadget@gmail.com>

"blanet via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> Elijah Newren, the author of the remerge diff feature, notes that other
> callers of `log-tree.c:log_tree_commit` (the only caller of
> `log-tree.c:do_remerge_diff`) also exist, but:
>
>   `builtin/am.c`: manually sets all flags; remerge_diff is not among them
>   `sequencer.c`: manually sets all flags; remerge_diff is not among them
>
> so `builtin/diff-tree.c` really is the only caller that was overlooked
> when remerge-diff functionality was added.

That is more than OK as a band-aid, and I'll take the patch as-is, but I have to wonder if we do even better in a future follow-up patch.

Any time do_remerge_diff() is entered, we know that either the end user (from the command line) or the hard-coded caller (like am/sequencer cited above) wants us to do the remerge-diff, which in turn requires us to have the temporary object directory rotated into the status of the primary object store. And there is nothing in that object directory rotation code that requires caller-specific customization---it is the same "create remerge-diff directory as tmp-objdir, rotate it into the alt object store chain as the primary" regardless of the actual caller).

So wouldn't it work well if we

 (1) at the beginning of do_remerge_diff(), only once for a rev_info
     structure:
   (1-a) lazily do the "object directory rotation"
   (1-b) set up an atexit handler to clear the temporary object
         store
 (2) remove all the "ah, we need to prepare and tear down the
     temporary object store for _this_ operation" we have sprinkled
     in different code paths (including the one added by the fix we
     are looking at).

That way, we won't have to worry about adding future remerge_diff users, including existing hard-coded callers.

ANyway, thanks for the fix. It is very pleasing to see contributors working well together.