Re: [PATCH] multi-pack-index: fix --object-dir from outside repo

> 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".

>  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).

>  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().

> 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().

> 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.

> > +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.

> 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 &&

> 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 ;).
>