Re: [PATCH v4 05/25] midx: clear auxiliary .rev after replacing the MIDX

"brian m. carlson" [off-list ref] writes:

Yeah, this is a possible problem.  You can also see it when using git
index-pack outside of a repository with an incorrect --object-format
option.

I'm not sure how folks want to deal with that; I'm just fine saying,
"Well, don't do that," but other folks may have different opinions.

OK, so if we go back to the original breakage of the test script
that triggered this discussion, the right solution would be to make
sure both test repositories/object stores are prepared with the
algorithm specified with GIT_TEST_DEFAULT_HASH?

Thanks.

On Mon, Aug 30, 2021 at 03:28:47PM -0700, Junio C Hamano wrote:

"brian m. carlson" [off-list ref] writes:

quoted

Yeah, this is a possible problem.  You can also see it when using git
index-pack outside of a repository with an incorrect --object-format
option.

I'm not sure how folks want to deal with that; I'm just fine saying,
"Well, don't do that," but other folks may have different opinions.

OK, so if we go back to the original breakage of the test script
that triggered this discussion, the right solution would be to make
sure both test repositories/object stores are prepared with the
algorithm specified with GIT_TEST_DEFAULT_HASH?

Just to make sure do you still see this as a separate issue from running
the midx builtin outside of a repository?

I.e., if we require the midx builtin to be run in a repository, it
side-steps this issue (but presumably not completely, and so we should
deal with both eventually). I want to make sure that I'm on the same
page before I drop 25+ emails on the list.

Thanks,
Taylor

On Mon, Aug 30, 2021 at 06:33:18PM -0400, Taylor Blau wrote:

On Mon, Aug 30, 2021 at 03:28:47PM -0700, Junio C Hamano wrote:

quoted

"brian m. carlson" [off-list ref] writes:

quoted

Yeah, this is a possible problem.  You can also see it when using git
index-pack outside of a repository with an incorrect --object-format
option.

I'm not sure how folks want to deal with that; I'm just fine saying,
"Well, don't do that," but other folks may have different opinions.

OK, so if we go back to the original breakage of the test script
that triggered this discussion, the right solution would be to make
sure both test repositories/object stores are prepared with the
algorithm specified with GIT_TEST_DEFAULT_HASH?

Just to make sure do you still see this as a separate issue from running
the midx builtin outside of a repository?

Adding my two cents: yes, I think it most definitely should be a
separate issue. As you demonstrated, differing config between alternates
and repos that point to them is not specific to the midx code. I agree
with brian's "well, don't do that". But _if_ we want to try to behave
better in such a case, whatever we changes we make would then naturally
apply to the midx code as well.

The two midx-specific things we have to care about are:

  - is it OK for the midx command to refuse to operate when we are not
    in a repository at all? I think yes; we can't even know which hash
    is being used, along with who knows what other lurking
    complications.

  - is it OK to restrict the midx command's --object-dir to only operate
    on a directory which is an alternate of the current repo? I think
    yes again. If it _isn't_ related, we have all the lurking problems
    from the first point, but even worse (because we use config, refs,
    and other information from our current repo with the _totally
    unrelated_ object dir).

So I'm all in favor of locking those down now before things get any more
complicated. If we later want to make the object store more aware of of
differences between alternates and the main store (like say, the object
hash in use), then we could consider loosening using the same mechanism.

-Peff

Taylor Blau [off-list ref] writes:

On Mon, Aug 30, 2021 at 03:28:47PM -0700, Junio C Hamano wrote:

quoted

"brian m. carlson" [off-list ref] writes:

quoted

Yeah, this is a possible problem.  You can also see it when using git
index-pack outside of a repository with an incorrect --object-format
option.

I'm not sure how folks want to deal with that; I'm just fine saying,
"Well, don't do that," but other folks may have different opinions.

OK, so if we go back to the original breakage of the test script
that triggered this discussion, the right solution would be to make
sure both test repositories/object stores are prepared with the
algorithm specified with GIT_TEST_DEFAULT_HASH?

Just to make sure do you still see this as a separate issue from running
the midx builtin outside of a repository?

They are separate issues, but the .midx issue has a small overlap
with the much bigger "do not mix repositories and object stores with
different hashes" issue.

The users of raw object stores (e.g. $GIT_OBJECT_DIRECTORIES,
"--object-dir", there may be others) need to be updated so that the
code paths involved can reliably learn what hash algorithm is used
and other traits that may not be available in the object store alone
(e.g. refs might be relevant if the using code needs to learn which
objects are still reachable) for the latter.  It would need a couple
of things that are fairly isolated to solve, I would imagine:

 (1) convention to either tie a raw object store with its repository
     or declare a raw object store is unusable because "other
     traits" are not found for it.

 (2) given a repository, inspect it and decide if it is "compatible"
     with the current repository.

 (3) update code paths involved in prepare_alt_odb() to use (1) and
     (2) to inspect and reject incompatible object store as
     alternate.

And once we have that, "git multi-pack-index --object-dir=X" can use
(1) and (2) for the same "Is this other object store compatible with
the current repository?" check, no?

The other side of the coin is that midx needs to do equivalents of
(1) and (2) anyway, and the required amount of the work for (3)
smells a lot smaller than work for (1) and (2).  (3) may be just a
matter of "add a call to is_odb_compatible(dir) for the directory
being added as an alt odb", and the same single validation call may
be all it needs on the --object-dir argument on the midx side.

I think it makes sense for the midx command to require being in a
repository to run (to establish what "the current repository" is)
and insist on the other object store given with --object-dir to be
"compatible" with the current repository (i.e. the same hash
algorithm, there may be others).  I am a bit fuzzy why we want it
to be already our alternate.

Thanks.

On Tue, Aug 31, 2021 at 09:29:46AM -0700, Junio C Hamano wrote:

Taylor Blau [off-list ref] writes:

quoted

On Mon, Aug 30, 2021 at 03:28:47PM -0700, Junio C Hamano wrote:

quoted

"brian m. carlson" [off-list ref] writes:

quoted

Yeah, this is a possible problem.  You can also see it when using git
index-pack outside of a repository with an incorrect --object-format
option.

I'm not sure how folks want to deal with that; I'm just fine saying,
"Well, don't do that," but other folks may have different opinions.

OK, so if we go back to the original breakage of the test script
that triggered this discussion, the right solution would be to make
sure both test repositories/object stores are prepared with the
algorithm specified with GIT_TEST_DEFAULT_HASH?

Just to make sure do you still see this as a separate issue from running
the midx builtin outside of a repository?

They are separate issues, but the .midx issue has a small overlap
with the much bigger "do not mix repositories and object stores with
different hashes" issue.

OK, good. Everything you wrote below (which I snipped off in my reply)
makes sense to me, and seems like a worthwhile direction to pursue
outside of this series, especially as more users start using sha256
repositories.

I think it makes sense for the midx command to require being in a
repository to run (to establish what "the current repository" is)
and insist on the other object store given with --object-dir to be
"compatible" with the current repository (i.e. the same hash
algorithm, there may be others).  I am a bit fuzzy why we want it
to be already our alternate.

I don't think there's any strict requirement to the other repository
being our alternate, other than touching arbitrary repositories is a
surprising behavior that appears (to me, at least) to be inconsistent
with the rest of Git.

After (the rerolled version of) this series, we'll be in a state where:

  - `git multi-pack-index` will not run when outside of a Git
    repository.
  - The `--object-dir` argument will only recognize object directories
    belonging to an alternate of the current repository.
  - Using `--object-dir` to point to a repository which uses a
    different hash than the repository in the current working directory
    will continue to not work (as was the case before this series).

I think(?) that there is consensus for that approach, so patches
incoming...

Thanks.

(Thank you, by the way, for clarifying this all in so much detail. I
would much rather just have code to talk about, but it feels
particularly important to be on the same page beforehand in this
instance, since there is *so much* code, and this discussion is centered
around so little of it).

Thanks,
Taylor