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

On Thu, Aug 26, 2021 at 02:49:10PM -0400, Taylor Blau wrote:

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

quoted

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.

Hmm. Upon thinking on in more, here is some evidence to the contrary.
The new test, specifically this snippet:

    git init repo &&
    test_when_finished "rm -fr repo" &&
    (
      cd repo &&
      test_commit base &&
      git repack -d
    ) &&

    nongit git multi-pack-index --object-dir=$(pwd)/repo/.git/objects write

will fail with GIT_TEST_DEFAULT_HASH=sha256, since the MIDX internals
settle on the hash size via `the_hash_algo` which doesn't respect the
hash algorithm used by the target repository.

And that seems like it never could have worked. Try this at your shell
to observe the failure:

    git init --object-format=sha256 repo &&
    git -C repo commit --allow-empty -m initial &&
    git -C repo repack -d &&

    git multi-pack-index write --object-dir=$(pwd)/repo/.git/objects

and get:

    error: wrong index v2 file size in
    /home/ttaylorr/repo/.git/objects/pack/pack-9f08dc78ae6f37407a5acad69e3fdf5a1887eb7da5c043a1ddedc56ea7160814.idx
    warning: failed to open pack-index
    '/home/ttaylorr/repo/.git/objects/pack/pack-9f08dc78ae6f37407a5acad69e3fdf5a1887eb7da5c043a1ddedc56ea7160814.idx'

since we're trying to open a sha256 index with the_hash_algo in
sha1-mode.

The question is do we consider this to be a bug in the existing behavior
that we should patch, or an indication that the feature shouldn't exist
in the first place?

I think that I tend to agree more with the latter, so I'm inclined to
drop support for it (where "it" is running the midx command outside of a
repository) in this series (i.e., by making the midx builtin have the
RUN_SETUP flag instead of RUN_SETUP_GENTLY).

Thoughts?

Thanks,
Taylor