git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 1/2] prepare_commit_graft: treat non-repository as a noop
Date: Thu, 31 May 2018 18:42:53 -0400	[thread overview]
Message-ID: <20180531224252.GA13127@sigill.intra.peff.net> (raw)
In-Reply-To: <20180531223901.GA18999@sigill.intra.peff.net>

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 <peff@peff.net>
---
 commit.c               | 3 +++
 t/t5300-pack-object.sh | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/commit.c b/commit.c
index b0e57cc440..0030e79940 100644
--- a/commit.c
+++ b/commit.c
@@ -207,6 +207,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 87a590c4a9..2336d09dcc 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -421,6 +421,12 @@ test_expect_success 'index-pack <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
+test_expect_success 'index-pack --strict <pack> 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 &&
-- 
2.17.1.1443.gd58a3f03b7


  reply	other threads:[~2018-05-31 22:42 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-31 22:39 [PATCH 0/2] index-pack out-of-repo fixups Jeff King
2018-05-31 22:42 ` Jeff King [this message]
2018-05-31 22:45 ` [PATCH 2/2] index-pack: handle --strict checks of non-repo packs Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180531224252.GA13127@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).