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:
> > On Tue, Aug 24, 2021 at 10:10:12PM -0400, Taylor Blau wrote:
> >
> > > > 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
