From 14a9bd2898b38449663644f862542d618f332952 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 May 2018 18:42:53 -0400 Subject: prepare_commit_graft: treat non-repository as a noop The parse_commit_buffer() function consults lookup_commit_graft() to see if we need to rewrite parents. The latter will look at $GIT_DIR/info/grafts. If you're outside of a repository, then this will trigger a BUG() as of b1ef400eec (setup_git_env: avoid blind fall-back to ".git", 2016-10-20). It's probably uncommon to actually parse a commit outside of a repository, but you can see it in action with: cd /not/a/git/repo git index-pack --strict /some/file.pack This works fine without --strict, but the fsck checks will try to parse any commits, triggering the BUG(). We can fix that by teaching the graft code to behave as if there are no grafts when we aren't in a repository. Arguably index-pack (and fsck) are wrong to consider grafts at all. So another solution is to disable grafts entirely for those commands. But given that the graft feature is deprecated anyway, it's not worth even thinking through the ramifications that might have. There is one other corner case I considered here. What should: cd /not/a/git/repo export GIT_GRAFT_FILE=/file/with/grafts git index-pack --strict /some/file.pack do? We don't have a repository, but the user has pointed us directly at a graft file, which we could respect. I believe this case did work that way prior to b1ef400eec. However, fixing it now would be pretty invasive. Back then we would just call into setup_git_env() even without a repository. But these days it actually takes a git_dir argument. So there would be a fair bit of refactoring of the setup code involved. Given the obscurity of this case, plus the fact that grafts are deprecated and probably shouldn't work under index-pack anyway, it's not worth pursuing further. This patch at least un-breaks the common case where you're _not_ using grafts, but we BUG() anyway trying to even find that out. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/commit.c b/commit.c index 00c99c7..9e49c22 100644 --- a/commit.c +++ b/commit.c @@ -196,6 +196,9 @@ static void prepare_commit_graft(void) if (commit_graft_prepared) return; + if (!startup_info->have_repository) + return; + graft_file = get_graft_file(); read_graft_file(graft_file); /* make sure shallows are read */ diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 9c68b99..88ec5d7 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -421,6 +421,12 @@ test_expect_success 'index-pack works in non-repo' ' test_path_is_file foo.idx ' +test_expect_success 'index-pack --strict works in non-repo' ' + rm -f foo.idx && + nongit git index-pack --strict ../foo.pack && + test_path_is_file foo.idx +' + test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'index-pack --threads=N or pack.threads=N warns when no pthreads' ' test_must_fail git index-pack --threads=2 2>err && grep ^warning: err >warnings && -- cgit v0.10.2-6-g49f6 From 368b4e59061e5e6d2136d685f29c1cd01f5e0557 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 31 May 2018 18:45:31 -0400 Subject: index-pack: handle --strict checks of non-repo packs Commit 73c3f0f704 (index-pack: check .gitmodules files with --strict, 2018-05-04) added a call to add_packed_git(), with the intent that the newly-indexed objects would be available to the process when we run fsck_finish(). But that's not what add_packed_git() does. It only allocates the struct, and you must install_packed_git() on the result. So that call was effectively doing nothing (except leaking a struct). But wait, we passed all of the tests! Does that mean we don't need the call at all? For normal cases, no. When we run "index-pack --stdin" inside a repository, we write the new pack into the object directory. If fsck_finish() needs to access one of the new objects, then our initial lookup will fail to find it, but we'll follow up by running reprepare_packed_git() and looking again. That logic was meant to handle somebody else repacking simultaneously, but it ends up working for us here. But there is a case that does need this, that we were not testing. You can run "git index-pack foo.pack" on any file, even when it is not inside the object directory. Or you may not even be in a repository at all! This case fails without doing the proper install_packed_git() call. We can make this work by adding the install call. Note that we should be prepared to handle add_packed_git() failing. We can just silently ignore this case, though. If fsck_finish() later needs the objects and they're not available, it will complain itself. And if it doesn't (because we were able to resolve the whole fsck in the first pass), then it actually isn't an interesting error at all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 7b2f7c0..7b39947 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1480,8 +1480,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name, } else chmod(final_index_name, 0444); - if (do_fsck_object) - add_packed_git(final_index_name, strlen(final_index_name), 0); + if (do_fsck_object) { + struct packed_git *p; + p = add_packed_git(final_index_name, strlen(final_index_name), 0); + if (p) + install_packed_git(the_repository, p); + } if (!from_stdin) { printf("%s\n", sha1_to_hex(hash)); diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index a770d92..4157e1a 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7415-submodule-names.sh @@ -122,6 +122,16 @@ test_expect_success 'transfer.fsckObjects handles odd pack (index)' ' test_must_fail git -C dst.git index-pack --strict --stdin output && + # Make sure we fail due to bad gitmodules content, not because we + # could not read the blob in the first place. + grep gitmodulesName output +' + test_expect_success 'fsck detects symlinked .gitmodules file' ' git init symlink && ( -- cgit v0.10.2-6-g49f6 From 3737746120d4e16b3a2b3431e34eeb3314edfa8c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 11 Jun 2018 15:09:18 -0700 Subject: index-pack: correct install_packed_git() args The function does not start taking the repository object as a parameter before v2.18 track. Make the topic mergeable to v2.17 maintenance track by dropping it. Signed-off-by: Junio C Hamano diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 7b39947..3030c88 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1484,7 +1484,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, struct packed_git *p; p = add_packed_git(final_index_name, strlen(final_index_name), 0); if (p) - install_packed_git(the_repository, p); + install_packed_git(p); } if (!from_stdin) { -- cgit v0.10.2-6-g49f6