On 8/20/2021 3:35 PM, Johannes Berg wrote:
If using --object-dir to point into a repo, 'write' will
segfault trying to access the object-dir via the repo it
found, but that's not fully initialized. Fix it to use
the object_dir properly.
Thanks for finding this! It's difficult to cover all of
the cases, but I'm glad you found this and added a test.
+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"
Why are you using "cd /" here? Even if you mean to use "cd",
please do so within a sub-shell.
Could you instead init a new repo within the current directory
and point the object-dir to that location?
It could look something like this, (warning: I did not test this)
git init other &&
test_commit -C other first &&
git multi-pack-index --object-dir=other/.git/objects write
And is the only post-condition you are checking that we do not
crash? Or is there a specific result you are looking for? For
instance, we can double check that the MIDX was written:
test_path_is_file other/.git/objects/pack/multi-pack-index
but also you seem to be touching areas that delete files. Could
we 'touch' some of those and then see them get deleted?
Thanks,
-Stolee
Hi Derrick,
quoted
+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"
Why are you using "cd /" here?
I just needed to go outside the current test git directory, the tests
are running in a way that the current working directory is already the
git tree I'm operating in.
Even if you mean to use "cd",
please do so within a sub-shell.
I thought about it, but clearly all the tests are run in a sub-shell, so
it didn't seem necessary? But happy to change, I don't really care
either way.
Could you instead init a new repo within the current directory
and point the object-dir to that location?
I guess I could, but all the other stuff in here is already making a new
repo in the current working dir, and already initializing it with
objects, etc.
Doing it all over again seemed like a waste of time?
It could look something like this, (warning: I did not test this)
git init other &&
test_commit -C other first &&
git multi-pack-index --object-dir=other/.git/objects write
Sure.
Actually, this won't work to test for the crash, I'd have to do
something like
git init other
test_commit -C other first &&
(
mkdir non-git &&
cd non-git &&
git multi-pack-index --object-dir=../other/.git/objects write
)
or so.
And is the only post-condition you are checking that we do not
crash?
Yes, I was assuming that it'd actually work at that point - maybe not
the best assumption, it could (erroneously) exit with a 0 exit status
but have done nothing.
Or is there a specific result you are looking for? For
instance, we can double check that the MIDX was written:
test_path_is_file other/.git/objects/pack/multi-pack-index
So I guess that would be a good idea.
but also you seem to be touching areas that delete files. Could
we 'touch' some of those and then see them get deleted?
Ah, well, that's the underlying issue but I'm not sure we even ever get
to that code? Then again, yes, the *.rev files should get removed, I'll
see - not even sure I know how to get them to be generated in the first
place, is that even supported already?
johannes
On Fri, Aug 20, 2021 at 09:35:04PM +0200, Johannes Berg wrote:
If using --object-dir to point into a repo, 'write' will
segfault trying to access the object-dir via the repo it
found, but that's not fully initialized. Fix it to use
the object_dir properly.
Thanks for CC'ing me; I have definitely been wondering about the
intended behavior of `--object-dir` on the list recently [1].
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.
quoted hunk
static int midx_checksum_valid(struct multi_pack_index *m)
@@ -1086,7 +1086,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
if (flags & MIDX_WRITE_REV_INDEX)
write_midx_reverse_index(midx_name, midx_hash, &ctx);
- clear_midx_files_ext(the_repository, ".rev", midx_hash);
+ clear_midx_files_ext(object_dir, ".rev", midx_hash);
We can rely on this value always being non-NULL, so this is good.
quoted hunk
-static void clear_midx_files_ext(struct repository *r, const char *ext,
+static void clear_midx_files_ext(const char *object_dir, const char *ext,
unsigned char *keep_hash)
{
struct clear_midx_data data;@@ -1146,7 +1146,7 @@ static void clear_midx_files_ext(struct repository *r, const char *ext,
hash_to_hex(keep_hash), ext);
data.ext = ext;
- for_each_file_in_pack_dir(r->objects->odb->path,
+ for_each_file_in_pack_dir(object_dir,
clear_midx_file_ext,
&data);
And here's the most important part of the change, which is obviously
correct. But note to other reviewers that this has nothing to do with
the RUN_SETUP issue I mentioned earlier, since
for_each_file_in_pack_dir() doesn't care about that.
+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 &&
git multi-pack-index write --object-dir=repo/.git/objects &&
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
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.
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 ;). 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,
Taylor
[1]: https://lore.kernel.org/git/YQMFIljXl7sAAA%2FL@nand.local/
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.
quoted
+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/