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.

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.

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