Re: [PATCH 0/2] When fetching, warn if in commit graph but not obj db

From: Josh Steadmon <steadmon@google.com>
Date: Oct 30, 2024, 21:22 UTC
Message-ID: <ikraw375qal5ioopai5wk3i5vtpnyurxxd2ixsdujehpypuova@in5idqkd6wee>
In-Reply-To: <cover.1730235646.git.jonathantanmy@google.com>

On 2024.10.29 14:11, Jonathan Tan wrote:

Show 8 quoted lines

> I mentioned previously [1] the possibility of not running maintenance
> steps (commit graph writing and "git maintenance") if no packs were
> fetched, but looking at things again, I think that we shouldn't do
> that - in particular, if I ran "git fetch --refetch", I would fully
> expect the objects to be repacked, even if Git wasn't able to detect
> conclusively whether a pack was transmitted.
> 
> [1] https://lore.kernel.org/git/20241028225504.4151804-1-jonathantanmy@google.com/

A note for upstream, because I'm not sure it was ever explicitly mentioned: at $DAYJOB, we saw this fetch recursion error as a side-effect of the erroneous GC of local commits discussed at [2].

[2] https://lore.kernel.org/git/cover.1729792911.git.jonathantanmy@google.com/

Show 13 quoted lines

> So I went back to my original idea of detecting when an object is
> missing. In trying to balance the concerns of both doing something as
> reasonable as possible in such a repo corruption case, and not slowing
> down and/or unnecessarily complicating the main code flow, I decided
> to detect when an object is present in the commit graph but not in the
> object DB, and to limit this detection for objects specified in the
> fetch refspec.
> 
> Upon detection, we can't fix it due to reasons mentioned in the commit
> message, so I decided to print a warning. An alternate option is to make
> it a fatal error (instead of a warning) if an object is detected to be
> in the commit graph but not the object DB. I haven't thought through the
> ramifications of that, though.

At first glance, I lean towards making this a fatal error, but I'll try thinking out loud a bit:

First, we believe that [2] above should fix the root cause of the particular case we saw at $DAYJOB (hopefully this type of error doesn't have multiple root causes). So we expect to basically never encounter this error again after [2] is merged and rolled out, and all existing cases of repo corruption have been repaired. However, interacting with a broken repo even with a client that includes [2] would still hit this condition and issue a warning.

With the current implementation, fetching in a corrupt repo would still cause git-fetch to infinitely recurse, and therefore would repeatedly print the same error message to the console, until either the user noticed, or we fail to launch a new git-fetch process due to resource exhaustion.

I don't see any reason why the above situation is more friendly or desirable than exiting (with the same error message) as soon as we detect this type of corruption. However, I don't feel super strongly about it. If the rest of the list is OK with repeated error messages, then I can live with it.

Show 12 quoted lines

> Jonathan Tan (2):
>   Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
>   fetch-pack: warn if in commit graph but not obj db
> 
>  fetch-pack.c | 45 +++++++++++++++++++++++++--------------------
>  object.h     |  2 +-
>  2 files changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.47.0.163.g1226f6d8fa-goog
> 
>