{"thread":{"id":"48618","subject":"[PATCH 0/2] index-pack out-of-repo fixups","startedAt":"2018-05-31T22:39:06Z","lastAt":"2018-05-31T22:45:35Z","messageCount":3,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"348942","messageId":"20180531223901.GA18999@sigill.intra.peff.net","threadId":"48618","inReplyTo":null,"subject":"[PATCH 0/2] index-pack out-of-repo fixups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T22:39:01Z","receivedAt":"2018-05-31T22:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Coverity reported that the recently-added call to add_packed_git() in\nindex-pack.c actually does nothing. It's right, and it turns out this is\na minor bug in the .gitmodules fsck patches in v2.17.1. I say minor\nbecause it errs on the side of complaining too much (so it's not a\nsecurity problem) and it comes up only in what I can imagine is a pretty\nobscure case.\n\nThe second patch here fixes that. But while looking into it, I notice a\nmuch older breakage in running \"index-pack --strict\" outside of a\nrepository entirely. That's fixed by the first patch.\n\n  [1/2]: prepare_commit_graft: treat non-repository as a noop\n  [2/2]: index-pack: handle --strict checks of non-repo packs\n\n builtin/index-pack.c       |  8 ++++++--\n commit.c                   |  3 +++\n t/t5300-pack-object.sh     |  6 ++++++\n t/t7415-submodule-names.sh | 10 ++++++++++\n 4 files changed, 25 insertions(+), 2 deletions(-)\n\n-Peff\n"},{"id":"348943","messageId":"20180531224252.GA13127@sigill.intra.peff.net","threadId":"48618","inReplyTo":"20180531223901.GA18999@sigill.intra.peff.net","subject":"[PATCH 1/2] prepare_commit_graft: treat non-repository as a noop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T22:42:53Z","receivedAt":"2018-05-31T22:42:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_commit_buffer() function consults lookup_commit_graft()\nto see if we need to rewrite parents. The latter will look\nat $GIT_DIR/info/grafts. If you're outside of a repository,\nthen this will trigger a BUG() as of b1ef400eec (setup_git_env:\navoid blind fall-back to \".git\", 2016-10-20).\n\nIt's probably uncommon to actually parse a commit outside of\na repository, but you can see it in action with:\n\n  cd /not/a/git/repo\n  git index-pack --strict /some/file.pack\n\nThis works fine without --strict, but the fsck checks will\ntry to parse any commits, triggering the BUG(). We can fix\nthat by teaching the graft code to behave as if there are no\ngrafts when we aren't in a repository.\n\nArguably index-pack (and fsck) are wrong to consider grafts\nat all. So another solution is to disable grafts entirely\nfor those commands. But given that the graft feature is\ndeprecated anyway, it's not worth even thinking through the\nramifications that might have.\n\nThere is one other corner case I considered here. What\nshould:\n\n  cd /not/a/git/repo\n  export GIT_GRAFT_FILE=/file/with/grafts\n  git index-pack --strict /some/file.pack\n\ndo? We don't have a repository, but the user has pointed us\ndirectly at a graft file, which we could respect. I believe\nthis case did work that way prior to b1ef400eec. However,\nfixing it now would be pretty invasive. Back then we would\njust call into setup_git_env() even without a repository.\nBut these days it actually takes a git_dir argument. So\nthere would be a fair bit of refactoring of the setup code\ninvolved.\n\nGiven the obscurity of this case, plus the fact that grafts\nare deprecated and probably shouldn't work under index-pack\nanyway, it's not worth pursuing further. This patch at least\nun-breaks the common case where you're _not_ using grafts,\nbut we BUG() anyway trying to even find that out.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c               | 3 +++\n t/t5300-pack-object.sh | 6 ++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex b0e57cc440..0030e79940 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -207,6 +207,9 @@ static void prepare_commit_graft(void)\n \n \tif (commit_graft_prepared)\n \t\treturn;\n+\tif (!startup_info->have_repository)\n+\t\treturn;\n+\n \tgraft_file = get_graft_file();\n \tread_graft_file(graft_file);\n \t/* make sure shallows are read */\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 87a590c4a9..2336d09dcc 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -421,6 +421,12 @@ test_expect_success 'index-pack <pack> works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n+test_expect_success 'index-pack --strict <pack> works in non-repo' '\n+\trm -f foo.idx &&\n+\tnongit git index-pack --strict ../foo.pack &&\n+\ttest_path_is_file foo.idx\n+'\n+\n test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n \ttest_must_fail git index-pack --threads=2 2>err &&\n \tgrep ^warning: err >warnings &&\n-- \n2.17.1.1443.gd58a3f03b7\n\n"},{"id":"348944","messageId":"20180531224531.GB13127@sigill.intra.peff.net","threadId":"48618","inReplyTo":"20180531223901.GA18999@sigill.intra.peff.net","subject":"[PATCH 2/2] index-pack: handle --strict checks of non-repo packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T22:45:31Z","receivedAt":"2018-05-31T22:45:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 73c3f0f704 (index-pack: check .gitmodules files with\n--strict, 2018-05-04) added a call to add_packed_git(), with\nthe intent that the newly-indexed objects would be available\nto the process when we run fsck_finish().  But that's not\nwhat add_packed_git() does. It only allocates the struct,\nand you must install_packed_git() on the result. So that\ncall was effectively doing nothing (except leaking a\nstruct).\n\nBut wait, we passed all of the tests! Does that mean we\ndon't need the call at all?\n\nFor normal cases, no. When we run \"index-pack --stdin\"\ninside a repository, we write the new pack into the object\ndirectory. If fsck_finish() needs to access one of the new\nobjects, then our initial lookup will fail to find it, but\nwe'll follow up by running reprepare_packed_git() and\nlooking again. That logic was meant to handle somebody else\nrepacking simultaneously, but it ends up working for us\nhere.\n\nBut there is a case that does need this, that we were not\ntesting. You can run \"git index-pack foo.pack\" on any file,\neven when it is not inside the object directory. Or you may\nnot even be in a repository at all! This case fails without\ndoing the proper install_packed_git() call.\n\nWe can make this work by adding the install call.\n\nNote that we should be prepared to handle add_packed_git()\nfailing. We can just silently ignore this case, though. If\nfsck_finish() later needs the objects and they're not\navailable, it will complain itself. And if it doesn't\n(because we were able to resolve the whole fsck in the first\npass), then it actually isn't an interesting error at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c       |  8 ++++++--\n t/t7415-submodule-names.sh | 10 ++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 4ab31ed388..74fe2973e1 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1482,8 +1482,12 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t} else\n \t\tchmod(final_index_name, 0444);\n \n-\tif (do_fsck_object)\n-\t\tadd_packed_git(final_index_name, strlen(final_index_name), 0);\n+\tif (do_fsck_object) {\n+\t\tstruct packed_git *p;\n+\t\tp = add_packed_git(final_index_name, strlen(final_index_name), 0);\n+\t\tif (p)\n+\t\t\tinstall_packed_git(the_repository, p);\n+\t}\n \n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(hash));\ndiff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh\nindex a770d92a55..4157e1a134 100755\n--- a/t/t7415-submodule-names.sh\n+++ b/t/t7415-submodule-names.sh\n@@ -122,6 +122,16 @@ test_expect_success 'transfer.fsckObjects handles odd pack (index)' '\n \ttest_must_fail git -C dst.git index-pack --strict --stdin <odd.pack\n '\n \n+test_expect_success 'index-pack --strict works for non-repo pack' '\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\tcp odd.pack dst.git &&\n+\ttest_must_fail git -C dst.git index-pack --strict odd.pack 2>output &&\n+\t# Make sure we fail due to bad gitmodules content, not because we\n+\t# could not read the blob in the first place.\n+\tgrep gitmodulesName output\n+'\n+\n test_expect_success 'fsck detects symlinked .gitmodules file' '\n \tgit init symlink &&\n \t(\n-- \n2.17.1.1443.gd58a3f03b7\n"}]}