Re: Crashes in t/t4058-diff-duplicates.sh

[Elijah Newren <newren@gmail.com>]

On Mon, May 9, 2022 at 8:23 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, May 06, 2022 at 09:14:07PM -0700, Elijah Newren wrote:
> > > That commit talks about "trees with duplicate entries".  Does it
> > > mean a bad history where a tree object has two or more entries under
> > > the same name?
> >
> > Yes.
> >
> > > We should of course be catching these things at fsck
> > > time and rejecting at network transfer time, but I agree it is not a
> > > good excuse for us to segfault.  We should diagnose it as a broken
> > > tree object and actively refuse to proceed by calling die().
>
> Elijah would be able to comment more authoritatively than I could about
> whether or not these are easily detect-able. If they are, then I think
> it'd be worth doing so and calling die(). But they may be tricker, I
> don't know.

It's been a couple years, so I don't remember much.  I think the way I
discovered these issues was that in order to make sure some other code
changes of mine didn't regress on some issues, I was attempting to
recreate problematic cases that had been covered by the code I was
restructuring.  The existing tests related to that code had some
problems, so I was modifying/creating my own testcases, and I
misunderstood the setup of those tests and the checks behind them and
ended up creating trees broken in a *different* way and which was not
covered by the existing code anywhere.  I was already a few tangents
from the focus of my work at the time (the new merge backend), so I
don't think I investigated whether these were easily detectable.  I do
remember being concerned that the necessary checks might be expensive,
and feeling that it'd be unfortunate to add expensive checks for
issues that no one had ever triggered in 15.5 years, and which I only
discovered due to intentionally trying to create a broken tree but
accidentally creating the wrong type of broken tree.

As it was, the new merge backend took a few years of work, and I
probably followed too many tangents along the way.  This particular
issue was a case where it clearly didn't touch code I was modifying
(the merge or diff machinery) and instead triggered in unpack-trees.c
and cache-tree.c.  So, I decided to simply document it in case others
wanted to investigate.

Long story short, I can't comment about the difficulty of detecting
and working around these.  If you've read this email and the commit
message I wrote at the time, then you know everything I remember about
the issue.