I saw the same error myself when integrating that patch into my series.
I discussed it more in [1], but the failure is basically caused by the
midx code using the_hash_algo even when operating in a different
repository via --object-dir.
If the_hash_algo doesn't match (as is the case when using `--object-dir`
to point at a SHA-256 repository when invoking the builtin from a
repository using SHA-1 or outside of a repository altogether), then
we'll fail when trying to open the pack indexes.
My recollection is that "--object-dir" is mostly about the alternate
odb usecase---am I correct? It is unfortunate that we didn't start
with "alternate repository" and said "we only care about the objects
in the object store they have, and we do not have to care what refs
they point into their object database or what configuration they
have" instead.
I wonder if it is safe to assume that in practice a directory given
to the "--object-dir" option is always the "objects" subdirectory in
a repository, and it is an error if there is no "config" file next
to the directory. Then, we could check ../config relative to the
given directory and error out if they use different hash.
I do not recall offhand how careful link_alt_odb_entries() is, but I
suspect it isn't at all (back when I invented it, there weren't need
for configuration to switch between hashes, and since then I do not
recall seeing any heavy update to the alternate odb code). Perhaps
we should tighten it so that we check the accompanying "config" file
first and ignore the entry with incompatible "hash" (and we may
later discover other trait on a repository that is incompatible with
the current one)?
Thanks.
From: Taylor Blau <hidden> Date: 2021-08-30 00:08:00
On Sun, Aug 29, 2021 at 03:56:31PM -0700, Junio C Hamano wrote:
My recollection is that "--object-dir" is mostly about the alternate
odb usecase---am I correct?
That matches my understanding. The documentation refers to the value of
this flag as `<alt>`, making me think that supporting non-alternates is
a historical accident.
I wonder if it is safe to assume that in practice a directory given
to the "--object-dir" option is always the "objects" subdirectory in
a repository, and it is an error if there is no "config" file next
to the directory. Then, we could check ../config relative to the
given directory and error out if they use different hash.
Maybe... although I have to admit to not being very excited about it. Is
the idea to read ../config to try and check for any incompatibilities
between the in-core state and the target repository's settings? If so,
this seems like a recipe for catching bugs too late.
For e.g., catching the_hash_algo != target_repository->hash would
definitely squash the bug you saw when integrating, but we would have to
remember to update this spot later on if, say, the target repository
started using a different reference storage backend (since bitmap
generation necessarily iterates the references to figure out which
commits should receive coverage).
I do not recall offhand how careful link_alt_odb_entries() is, but I
suspect it isn't at all (back when I invented it, there weren't need
for configuration to switch between hashes, and since then I do not
recall seeing any heavy update to the alternate odb code). Perhaps
we should tighten it so that we check the accompanying "config" file
first and ignore the entry with incompatible "hash" (and we may
later discover other trait on a repository that is incompatible with
the current one)?
Or are you saying you're concerned about an alternates chain which
don't all use the same object format?
If the former, then I would say:
"Supporting arbitrary --object-dir when invoked from outside a
repository is a bug that happened to not cause any problems, but
is surprising, error-prone, and should fall outside of the burden of
backwards compatibility, so we should get rid of it."
If the latter, then I agree we could and should do better at detecting
it and providing a helpful error message, but I don't see how doing so
now or later would affect this series. Even if we just disallow
--object-dir pointing at a non-alternate repository, we would still have
the issue of having alternate chains which don't all have the same
object format.
So that makes me feel like the latter is a problem outside of this
series that can be dealt with later.
I'm admittedly a little unsure of how to progress here. Given that this
series has received positive review over the complicated parts, it seems
that it is getting stuck on how to deal with `--object-dir`, especially
when invoked outside of a Git repository. My inclination would be to
send a new version that simply requires the MIDX builtin to be run from
within a repository (as well as the cleanups from Johannes).
Does that seem like a good direction forward to you? If not, let me know
if there's another issue that we should deal with first and I'd be happy
to start there.
On Sun, Aug 29, 2021 at 03:56:31PM -0700, Junio C Hamano wrote:
quoted
My recollection is that "--object-dir" is mostly about the alternate
odb usecase---am I correct?
That matches my understanding. The documentation refers to the value of
this flag as `<alt>`, making me think that supporting non-alternates is
a historical accident.
Yes, supporting non-alternates is a historical accident. Supporting
alternates that are not actually the core object database of a full
repository is on purpose.
So, hopefully the remaining discussion that I am seeing can be
solved by a decision such as:
"If we add the restriction that the builtin always runs with a
repository and --object-dir always points to its objects dir
or one of its registered alternates, then we have access to a
local config file to learn how to interpret that object directory."
quoted
I wonder if it is safe to assume that in practice a directory given
to the "--object-dir" option is always the "objects" subdirectory in
a repository, and it is an error if there is no "config" file next
to the directory. Then, we could check ../config relative to the
given directory and error out if they use different hash.
I would say that is not always the case, and we should not error out.
I think taking a look to see if ../config exists to use the data
might be helpful for some cases, but should not be a blocker for
completing the requested operation. The config from the non-alternate
repo should be sufficient for this (somewhat strange) case.
I'm admittedly a little unsure of how to progress here. Given that this
series has received positive review over the complicated parts, it seems
that it is getting stuck on how to deal with `--object-dir`, especially
when invoked outside of a Git repository. My inclination would be to
send a new version that simply requires the MIDX builtin to be run from
within a repository (as well as the cleanups from Johannes).
Does that seem like a good direction forward to you? If not, let me know
if there's another issue that we should deal with first and I'd be happy
to start there.
I think it is sensible to restrict 'git multi-pack-index' to run
inside a repository on its own merits. It happens to also solve
some tricky problems that have come up since its creation.
Sorry I'm so late to this thread. I gave most of the messages in this
chain a quick read and this seemed like the best place to chime in.
Hopefully this isn't too much of a re-tread of things covered elsewhere.
Thanks,
-Stolee
From: Jeff King <hidden> Date: 2021-08-31 05:37:49
On Mon, Aug 30, 2021 at 09:21:31PM -0400, Derrick Stolee wrote:
Yes, supporting non-alternates is a historical accident. Supporting
alternates that are not actually the core object database of a full
repository is on purpose.
So, hopefully the remaining discussion that I am seeing can be
solved by a decision such as:
"If we add the restriction that the builtin always runs with a
repository and --object-dir always points to its objects dir
or one of its registered alternates, then we have access to a
local config file to learn how to interpret that object directory."
I left a similar comment in the other part of the thread. :)
quoted
quoted
I wonder if it is safe to assume that in practice a directory given
to the "--object-dir" option is always the "objects" subdirectory in
a repository, and it is an error if there is no "config" file next
to the directory. Then, we could check ../config relative to the
given directory and error out if they use different hash.
I would say that is not always the case, and we should not error out.
I think taking a look to see if ../config exists to use the data
might be helpful for some cases, but should not be a blocker for
completing the requested operation. The config from the non-alternate
repo should be sufficient for this (somewhat strange) case.
Yes, agreed. We have long supported these kind of "bare" alternates, and
I wouldn't be surprised if they are in wide use (though I do wonder how
folks actually modify them, since most commands that touch objects
really do want to be in a repository).
In other cases where we may benefit from their being a containing repo
(e.g., accessing the ref tips of the alternate), we speculatively look
at ".." and see if there are any refs. See refs_from_alternate_cb()[0].
The natural extension for the hash-format problem would probably be to
call check_repository_format_gently() on the parent directory of the
alternate-objects dir. If it succeeds, then we can pull out the
hash_algo parameter from its repository_format struct. And if not, then
we just assume it matches the main repo.
But I suspect all of this is moot for now, beyond being able to return a
nicer error message. The rest of the code is not at all ready to handle
packs with two different hashes in the same process. And I suspect it
would take a reasonable amount of refactoring to make it so. If somebody
wants to work on that, I won't stop them, but I kind of doubt it is
worth anybody's time.
[0] Looking at refs_from_alternate_cb(), I did wonder if it would work
at all with a reftable alternate, but I suspect it would. I think we
ended up still having a "refs/" directory in that case, so we'd
recognize it as a repo (though really, it ought to be using
is_git_directory() instead of its hacky check). And then we farm out
the actual ref iteration to a separate for-each-ref process, passing
along --git-dir, which will read that alternate repo's config. So it
should Just Work, even with a different ref backend. It's almost
certainly broken if the hash algorithms don't match, though, because
we'd get oddly sized results from for-each-ref's output.
That's all just interesting tangent, though. :)
-Peff