On Mon, May 9, 2022 at 8:23 AM Taylor Blau <me@xxxxxxxxxxxx> 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.
