Re: [PATCH 0/5] Duplicate entry hardening

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jun 01, 2026 at 09:33:10PM +0900, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > We had some corrupt trees with duplicate entries in real world repositories,
> > which triggered an assertion failure in merge-ort. Further, the corrupt tree
> > creation in the third party tool would have been avoided had verify_cache()
> > correctly checked for D/F conflicts. Provide fixes for both issues,
> > including 3 preparatory changes for the merge-ort fix.
> >
> > Elijah Newren (5):
> >   merge-ort: propagate callback errors from traverse_trees_wrapper()
> >   merge-ort: drop unnecessary show_all_errors from collect_merge_info()
> >   merge-ort: free diff pairs queue in clear_or_reinit_internal_opts()
> >   merge-ort: abort merge when trees have duplicate entries
> >   cache-tree: fix verify_cache() to catch non-adjacent D/F conflicts
> 
> This is a fix to an important corner of our system, but somehow left
> in "Needs review" state for much longer than I would have liked, so
> even though I am officially on vacation ;-), I took some time to
> read these through (by the way it was a pleasant read, thank you).

Honestly, I always shy away from the merge-related subsystems. It has a
lot of subtleties that I don't have any experience with, so I never
really consider my input to be helpful here.

> I wonder if we create a rule like
> 
>     Those of you who have more than 30 commits in our project are
>     expected to review one topic (or more) from other contributors
>     for every three patches you send and ask for reviews by others.

Heh, that would make me condense patch series into fewer patches ;)

> it would help balance the patch vs review ratio, perhaps?

It's a good question. I typically try to aim for reviewing series on the
mailing list at least every second day, and I always encourage other
folks in my team to do the same. But recently I (well, rather we)
haven't really been able to due to the current situation at GitLab,
which forces us to put almost all of our focus towards a different
project for a while.

Overall I agree that everyone who is a core contributor should also make
reviews part of their regular worflow. At least for corporate
contributors that might also make it easier to communicate this to their
respective employers. Regardless of that, my expectation is that there
will be times where it works well, and other times where it works less
well.

Patrick