Re: [PATCH 2/2] remerge-diff: clean up temporary objdir at a central place

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2024, 21:16 UTC
Message-ID: <xmqqed5tx2vv.fsf@gitster.g>
In-Reply-To: <CABPp-BGmeg3x-F2ZjLzqcDYzSMOfz8Q3BDKUtB=vMO90yJRb3A@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 15 quoted lines

>> I forgot to add that I am not happy with this "centralized tear
>> down" step, even though I am reasonably happy with the "lazy set-up"
>> step.  I wonder why the remerge-diff related members have to exist
>> in the rev_info structure in the first place, instead of being in
>> the diffopt structure?  Moving them to diffopt may make the end
>> result much more pleasant to read.
>
> I'm not sure they need to exist in the rev_info structure.  I was
> probably thinking that e.g. log --remerge-diff would "need to do lots
> of diffs" but I only needed the temporary store setup once, and once
> in my mind matched better with rev_info.  I wasn't aware of
> diff_options.no_free or anything surrounding it.  If we can do the
> temporary store setup in diffopt and only do it once for all N diffs
> in a `git log --remerge-diff` run, then we could move this stuff from
> rev_info to diffopt.

After staring at the data structure around this area for a while, I think these two patches are OK, especially within the current structure. rev_info structure has a set of bits that are marked as "Diff flags" (some of which are not about diff at all, though; for example, verbose_header and no_commit_id belong more to the "Format info" group), and those related to merges do belong there, as they need more than "here are two tree-like things I want to compare".

So, I'll mark these two patches for 'next' now.

Thanks.