Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default

From: Jeff King <peff@peff.net>
Date: Nov 14, 2023, 19:43 UTC
Message-ID: <20231114194310.GC2092538@coredump.intra.peff.net>
In-Reply-To: <xmqqzfzgxops.fsf@gitster.g>

On Wed, Nov 15, 2023 at 01:51:43AM +0900, Junio C Hamano wrote:

Show 17 quoted lines

> >> Both of these are expected failures: we knowingly corrupt the repository
> >> and circumvent git-gc(1)/git-maintenance(1), thus no commit-graphs are
> >> updated. If we stick with the new stance that repository corruption
> >> should not require us to pessimize the common case,...
> >
> > Yeah, just like we try to be extra careful while running fsck,
> > because "--missing" is about finding these "corrupt" cases,
> > triggering the paranoia mode upon seeing the option would make
> > sense, no?  It would fix the failure in 6022, right?
> >
> > Thanks for working on this.
> 
> Just to make sure we do not miscommunicate, I do not think we want
> to trigger the paranoia mode only in our tests.  We want to be
> paranoid to help real users who used "--missing" for their real use,
> so enabling PARANOIA in the test script is a wrong approach.  We
> should enable it inside "rev-list --missing" codepath.

Yeah. Just like we auto-enabled GIT_REF_PARANOIA for git-gc, etc, I think we should do the same here.

As we are closing in on the v2.43 release, there's one thing I'm not sure about regarding release planning. Are these test cases that _used_ to detect the corruption, but now don't? I.e., I would have expected that disabling GIT_COMMIT_GRAPH_PARANOIA would take us back to the same state as v2.42. But I think it doesn't because of the hunk in e04838ea82 (commit-graph: introduce envvar to disable commit existence checks, 2023-10-31) that makes the has_object() call conditional (and now defaults to off).

What I'm getting as it that I think we have three options for v2.43:

  1. Ship what has been in the release candidates, which has a known
     performance regression (though the severity is up for debate).

  2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
     know that loosens some cases versus 2.42, which may be considered a
     regression.

  3. Sort it out before the release. We're getting pretty close to do
     a lot new work there, but I think the changes should be small-ish.
     The nuclear option is ejecting the topic and re-doing it in the
     next cycle.

I don't have a really strong preference between the three.

-Peff