Re: [PATCH v4 00/25] multi-pack reachability bitmaps

On Tue, Aug 24, 2021 at 08:28:36PM -0400, Jeff King wrote:

On Tue, Aug 24, 2021 at 12:15:47PM -0400, Taylor Blau wrote:

quoted

Range-diff against v3:
[...]
 9:  40cff5beb5 !  9:  c9fea31fa8 midx: avoid opening multiple MIDXs when writing
    @@ Commit message
         one and should invalidate the object store's memory of any MIDX that
         might have existed beforehand.

    +    Note that this now forbids passing object directories that don't belong
    +    to alternate repositories over `--object-dir`, since before we would
    +    have happily opened a MIDX in any directory, but now restrict ourselves
    +    to only those reachable by `r->objects->multi_pack_index` (and alternate
    +    MIDXs that we can see by walking the `next` pointer).
    +
    +    As far as I can tell, supporting arbitrary directories with
    +    `--object-dir` was a historical accident, since even the documentation
    +    says `<alt>` when referring to the value passed to this option.
    +
    +    A future patch could clean this up and provide a warning() when a
    +    non-alternate directory was given, since we'll still write a new MIDX
    +    there, we just won't reuse any MIDX that might happen to already exist
    +    in that directory.
    +

So this is definitely fixed as we discussed. But since that discussion,
we've had the thread over in:

  https://lore.kernel.org/git/20210820195558.44275-1-johannes@sipsolutions.net/ (local)

and its siblings:

  https://lore.kernel.org/git/20210823094049.44136-1-johannes@sipsolutions.net/ (local)

  https://lore.kernel.org/git/20210823171011.80588-1-johannes@sipsolutions.net/ (local)

It's not clear to me that we have a resolution on whether calling "cd ..
&& git multi-pack-index write --object-dir repo.git" is supposed to
work.

My recommendation would be to do the following things, all in a reroll
of this series:

  - Fix the bug by which we would delete a .rev or .bitmap file out of a
    different object store than we were working in (when the caller
    passes `--object-dir`).

  - Disallow running `git multi-pack-index` outside of a Git repository.

  - Restrict `--object-dir` to only work with alternates of the
    repository in the current working directory.

To me, that seems like both the least-surprising behavior, and what
would lend itself to the easiest implementation. I would probably argue
that the existing behavior (where `--object-dir` would work against
arbitrary repositories) is a bug, and shouldn't continue to be
supported.

So my plan would be to do that, which would generate something like the
following range-diff. If nobody has any objections, I'd like to send
what I currently have in ttaylorr/git on GitHub in the
tb/multi-pack-bitmaps branch as a reroll of this series, and then merge
that early in the cycle to give it a chance to be tested before we cut
2.34.

Thanks,
Taylor