Re: [PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts

From: Elijah Newren <newren@gmail.com>
Date: Dec 30, 2021, 23:35 UTC
Message-ID: <CABPp-BHDsd66zE4GwNB45HSCffSD5gryUYtdL+9t4LdVCDk0Vw@mail.gmail.com>
In-Reply-To: <xmqqa6ghog5x.fsf@gitster.g>

On Thu, Dec 30, 2021 at 2:56 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

>
> Derrick Stolee <stolee@gmail.com> writes:
>
> > This breakdown of the cases is informative, and I like how self-contained
> > the change is.
> >  ....
> >
> > This patch looks good to me. Thanks!
> >
> > Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
>
> Thanks, both.
>
> A related tangent, but I was looking at the data structure involved
> and noticed that the casting between structure types "merged_info"
> and "conflict_info" looked a bit ugly.

Yes, that's true.

Show 12 quoted lines

> It might be worth cleaning them up into
>
>  (A) a union with two struct, with "clean" member in the union to
>      switch between the two structures; or
>
>  (B) a single structure that looks like "conflict_info" but inlines
>      members of "merged_info" into it.
>
> The latter may be cleaner and simpler, and the unified data type
> would be the "merge info", which may be representing cleanly merged
> path, or conflicted path, and would justify conditional use of some
> members based on the value of the .clean member.

These are heavily used data structures.  Note that:
  sizeof(struct conflict_info) = 216
  sizeof(struct merged_info) = 64
In particular, we have to allocate one or the other of these for every
path (both file and directory) involved in the merge.  Since the
former is 3.375 times bigger than the latter, and the vast majority of
paths involved in a merge usually do not conflict (think of files only
changed on one side), using just one combined struct would require
more than 3x the amount of memory.  So I'd rather avoid (B).

(A) may work, but I'd still have to allocate merged_info instead of the union type to avoid the memory increase. And since we have an amount of memory allocated that is smaller than the union, when accessing it via the union, Stolee would probably still want all the same casting safeguards (as a safety check to avoid out-of-bounds accesses) that I think you're complaining about.