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

On Wed, Aug 25, 2021 at 03:36:15AM -0400, Jeff King wrote:

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

quoted

quoted

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.

All of those seem reasonable to me, and are what I would suggest if we
were starting from scratch. My only hesitation is whether people are
using the weird behavior of --object-dir in the wild (e.g., are bup
folks relying on it).

Johannes, is this something you're using _now_, and it works, or
something you hoped to use in the future?

I did some research[1] on what parts of `--object-dir` have worked (and not
worked) in the past, and came to the conclusion that although this
behavior is surprising, we do bear the responsibility of continuing to
maintain it.

And in that sense, I agree with your "only call close_object_store() if
the MIDX we are using came from the object store, or otherwise call
close_midx() if it didn't", so that's what I did in the
tb/multi-pack-bitmaps branch of my fork[2].

I think that this is the most reasonable path forward, since it resolves
Johannes' concerns while also not breaking any existing functionality in
the meantime as we add new features on top. It has the added benefit of
closing some holes that were open in the past, so I think that it's
worth doing.

Before I drop 27 patches onto the inboxes of list subscribers, would you
mind taking a look at [1] (and the rest of the patches in [2]) to make
sure that you're OK with the approach too?

Thanks,
Taylor

[1]: https://github.com/ttaylorr/git/commit/a24290489c2b30f3caed7e33fe8f85226a12778f
[2]: https://github.com/ttaylorr/git/compare/tb/multi-pack-bitmaps