Hi Taylor, > Thanks for CC'ing me; I have definitely been wondering about the > intended behavior of `--object-dir` on the list recently [1]. Oh, hah. Not really being a contributor I'm not following the list, thanks for the pointer. > I think your patch message could use some clarifying, though. Invoking > > cd $REPO/.. > git multi-pack-index write --object-dir=$REPO/.git/objects > > has... different behavior depending on which side of the "write" > argument you put `--object-dir". Wait what?! To be honest, I didn't expect it to even be valid on the right-hand side of "write". > On the left-hand side (i.e., > "--object-dir=... write", you get something like: > > cd $REPO/.. > git multi-pack-index --object-dir=$REPO/.git/objects write > > zsh: segmentation fault git.compile multi-pack-index ... > > because the_repository->objects->odb isn't initialized (so reading > `path` in `clear_midx_files_ext` crashes). Right, this is what I was doing. > But in the opposite order > (i.e., "write --object-dir=...") you get: > > BUG: environment.c:280: git environment hasn't been setup > zsh: abort git.compile multi-pack-index write > > because we catch that case much earlier in get_object_directory(). Why? > Because cmd_multi_pack_index() fills in the value of object_dir with > get_object_directory() if it isn't filled in already, but seeing "write" > causes us to stop parsing and dispatch to the sub-command > cmd_multi_pack_index_write(). Great ... But why do we even support both? What would the semantic difference be? I'd be happy with either one of them working I guess :) > I discussed this a little in [1] also (see the part about using > RUN_SETUP instead). There are definitely different ways to handle that; > you could equally imagine only dying if we were both outside of a Git > repository and didn't point at one via `--object-dir`. > > But that's separate from another issue which is fixed by your patch > which is that we don't respect the value of `--object-dir` when cleaning > up MIDX .rev files via clear_midx_files_ext(). Well, I _mostly_ meant to fix the crash, but yes, this is really the underlying issue, and indeed we should clean up the .rev files in this case as well. > Your fix there (to use the path of an object_dir instead of a repository > struct) makes sense (since we don't ever fill in a repository struct > corresponding to the `--object-dir` parameter from the MIDX code). > > But I think that's a separate issue than the RUN_SETUP thing I mentioned > earlier, so I would probably consider breaking this into two patches, > the first which addresses the RUN_SETUP thing, and the second which is > this fix. I never wanted to fix the other issue though ;-) And honestly, I think I don't understand the discussion at [1] well enough to really submit a patch for it. > > +test_expect_success 'multi-pack-index with --object-dir need not be in repo' ' > > + p="$(pwd)" && > > + rm -f $objdir/multi-pack-index && > > + cd / && > > + git multi-pack-index --object-dir="$p/$objdir" write && > > + cd "$p" > > +' > > + > > I agree with Stolee that there should be a new repo created within the > current working directory, that way you can "cd .." and be both outside > of the repo you just created, but not outside of the test environment. OK, fair enough, I'll resubmit. > But let's make sure that we're not deleting any files that we should be > leaving alone. So it might be good to do something like: > > git init repo && > test_when_finished "rm -fr repo" && > ( > cd repo && > > test_commit base && > git repack -d && > ) && > > rev="$objdir/pack/multi-pack-index-$(midx_checksum $objdir).rev" && > touch $rev && Hah, so you just manually pretend it was there - and meanwhile I was looking for a way to get git to generate one :) > > git multi-pack-index write --object-dir=repo/.git/objects && Now this has the order of arguments the other way around, why? > test_path_is_file repo/.git/objects/pack/multi-pack-index && > test_path_is_file repo/.git/objects/multi-pack-index && > test_path_is_file $objdir/pack/multi-pack-index && > test_path_is_file $rev Why would test_path_is_file? Seems like it should be !test_path_is_file? > > That isn't testing the "invoked from a non-repo, but --object-dir" is > given case, but I think that's fine since they really are separate > things. But that's the one thing I really want to work :) > Note also that midx_checksum doesn't exist, but it is merely a wrapper > over a test-tool that prints out (for a multi_pack_index "m") `m->data + > m->data_len - the_hash_algo->rawsz`. > > So between splitting the patch, clarifying the patch message, and > implementing support for this new test helper, this may be more of a > project than you were bargaining for ;). > Sounds like ;) But actually we don't really care about the midx_checksum here, afaict? MIDX_WRITE_REV_INDEX isn't ever set, so the rev files are not created today? > Let me know if you want any > help. I also don't mind taking care of it myself, since I promised in > [1] that I'd fix this issue anyway. Thanks :) How about I resubmit this patch with some of the edits, especially with test for the case I care about (--object-dir used from a non-git place to point elsewhere) and then you can build on top of that? Thanks, johannes > [1]: https://lore.kernel.org/git/YQMFIljXl7sAAA%2FL@nand.local/ >
