Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 13, 2024, 18:26 UTC
Message-ID: <20241113182656.2135341-1-jonathantanmy@google.com>
In-Reply-To: <20241113073500.GA587228@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> On Fri, Nov 01, 2024 at 01:11:47PM -0700, Jonathan Tan wrote:
> 
> > A subsequent commit will change the behavior of "git index-pack
> > --promisor", which is exercised in "build pack index for an existing
> > pack", causing the unclamped and clamped versions of the --window
> > test to exhibit different behavior. Move the clamp test closer to the
> > unclamped test that it references.
> 
> Hmm. The change in patch 4 broke another similar --window test I had in
> a topic in flight. I can probably move it to match what you've done
> here, but I feel like this may be papering over a bigger issue.
> 
> The reason these window tests are broken is that the earlier "build pack
> index for an existing pack" is now finding and storing deltas in a new
> pack when it does this:
> 
>   git index-pack --promisor=message test-3.pack &&
> 
> But that command is indexing a pack that is not even in the repository's
> object store at all! Yet it triggers a call to pack-objects that repacks
> within that object store.

As far as I know, index-pack, when run as part of fetch, indexes a pack that's not in the repository's object store; it indexes a packfile in a temp directory. (So I don't think this is a strange thing to do.)

Show 13 quoted lines

> Here's an even more extreme version. You do not need to have a
> repository at all to run index-pack. So doing:
> 
>   mkdir /tmp/foo
>   cd /tmp/foo
>   cp /some/repo/.git/objects/pack/*.pack .
>   for i in *.pack; do
>     git index-pack -v --promisor=foo $i
>   done
> 
> used to work, but with your patches will segfault (because the repo
> pointer is NULL). Granted it's odd to pass --promisor when you are not
> in a repo, but certainly we should never segfault.

Ah, good catch.

Show 13 quoted lines

> So I think at the very least that index-pack should not try to modify
> the repository's object database unless we are indexing a pack that is
> within it, which would fix both of those issues.
> 
> I'd guess in the real world, we'd only pass that option when indexing
> packs that we just fetched. But as a bystander to this feature, it feels
> quite odd to me that index-pack, which I generally consider a "read
> only" operation except for the index it was asked to write, would be
> creating a new pack like this. I didn't follow the topic closely enough
> to comment more intelligently, but would it be possible for the caller
> of index-pack to trigger the repack as an independent step?
> 
> -Peff

I thought of that, but as far as I know, during a fetch, index-pack is the only time in which the objects in the fetched pack are uncompressed in memory. There have been concerns about the performance of various ways of solving the promisor-object-and-GC bug, so I took an approach that minimizes the performance hit as much as possible, by avoiding yet another uncompression (we need to uncompress the objects to find their outgoing links, so that we know what to repack).

We definitely should prevent the segfault, but I think that's better done by making --promisor only work if we run index-pack from within a repo. I don't think we can restrict the repacking to run only if we're indexing a pack within the repo, because in our fetch case, we're indexing a new pack - not one within the repo.

Maybe we could conceptualize "index-pack --promisor" as the pack giving "testimony" about objects that its objects link to, so we can update our own records.