Re: [PATCH v2] object-file: reprepare alternates when necessary

From: Jeff King <peff@peff.net>
Date: Mar 9, 2023, 07:24 UTC
Message-ID: <ZAmJtnLgwimRBGTb@coredump.intra.peff.net>
In-Reply-To: <pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com>

On Wed, Mar 08, 2023 at 06:47:32PM +0000, Derrick Stolee via GitGitGadget wrote:

> Extend reprepare_packed_git() to also reprepate the alternate odb list

s/reprepate/reprepare/

Unless you were inventing a new word. :)

Show 9 quoted lines

> This change is specifically for concurrent changes to the repository, so
> it is difficult to create a test that guarantees this behavior is
> correct. I manually verified by introducing a reprepare_packed_git() call
> into get_revision() and stepped into that call in a debugger with a
> parent 'git log' process. Multiple runs of prepare_alt_odb() kept
> the_repository->objects->odb as a single-item chain until I added a
> .git/objects/info/alternates file in a different process. The next run
> added the new odb to the chain and subsequent runs did not add to the
> chain.

One thing that test wouldn't cover is loading alternates from $GIT_ALTERNATE_OBJECT_DIRECTORIES. Once upon a time, I think we were pretty inconsistent in finding duplicates. But these days it looks like it's all done centrally in link_alt_odb_entry(), via alt_odb_usable(). So it should be good.

Show 7 quoted lines

>     object-file: reprepare alternates when necessary
>     
>     This subtlety was notice by Michael Haggerty due to how alternates are
>     used server-side at $DAYJOB. Moving pack-files from a repository to the
>     alternate occasionally causes failures because processes that start
>     before the alternate exists don't know how to find that alternate at
>     run-time.

Yeah, I don't think there's any real reason not to do this. It simply doesn't come up in the same way that packfile-repreparing does, because there it is routine for a concurrent gc to remove an existing packfile and add the objects elsewhere.

Whereas modifying alternates was never seen as something that would happen often or automatically. I guess in GitHub's case it is from converting a repository from stand-alone into a fork, migrating its objects to a shared one, and adding an alternate.

The only downside might be performance. For sane cases, I think scanning the new alternates is OK. I know Eric (cc'd) has some crazy 100k-alternate setup (from 407532f82d, etc), but I'd expect a reprepare there is already expensive (we already have to re-scan every one of those directories for packfiles, and throw out any loose object caches).

The patch itself looks good to me (and I agree with the sentiment to just inline the lines rather than adding a function).

-Peff