list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Eric Wong <>
Cc: "Nguyễn Thái Ngọc Duy" <>,
	"Antonio Ospite" <>,
	"Junio C Hamano" <>,
Subject: Re: new segfault in master (6a6c0f10a70a6eb1)
Date: Sat, 11 May 2019 19:02:05 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Sat, May 11, 2019 at 06:31:20PM -0400, Jeff King wrote:

> On Sat, May 11, 2019 at 08:57:11PM +0000, Eric Wong wrote:
> > This test-tool submodule segfault seems new.  Noticed it while
> > checking dmesg for other things.
> Yeah, I hadn't seen it before. It's almost certainly the expect_failure
> added in 2b1257e463 (t/helper: add test-submodule-nested-repo-config,
> 2018-10-25), since otherwise we'd be complaining of a test failure.
> I know we don't expect this to do the right thing yet, but it seems like
> there's still a bug, since the test seems to think we should output a
> nice message (and it's possible that the segfault can be triggered from
> non-test-tool code, too).
> +cc the author.

Actually, the plot thickens. That test _used to_ correctly expect
failure (well, sort of -- it greps for the string with %s, which is
wrong!). But then more recently in d9b8b8f896 (submodule-config.c: use
repo_get_oid for reading .gitmodules, 2019-04-16), it started actually
doing the lookup in the correct repo. And that started the segfault,
because nobody has actually loaded the index for the submodule.

I don't think this can be triggered outside of test-tool. There are
four ways to get to config_from_gitmodules():

  - via repo_read_gitmodules(), which explicitly loads the index

  - via print_config_from_gitmodules(). This is called from
    submodule--helper, but only with the_repository as the argument (and
    I _think_ that the_repository->index is never NULL, because we point
    it at the_index).

  - via fetch_config_from_gitmodules(), which always passes

  - via update_clone_config_from_gitmodules(), likewise

But regardless, I think it makes sense to load the index on demand when
we need it here, which makes Antonio's original test pass (like the
patch below).

The segfault ultimately comes from repo_get_oid(); we feed it
":.gitmodules" and it blindly looks at repo->index. It's probably worth
it being a bit more defensive and just returning "no such entry" if
there's no index to look at (it could also load on demand, I guess, but
it seems like too low a level to be making that kind of decision).

I'm out of time for now, but I'll look into cleaning this up and writing
a real commit message later.

diff --git a/submodule-config.c b/submodule-config.c
index 4264ee216f..ad2444bcec 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -630,7 +630,8 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void
 		file = repo_worktree_path(repo, GITMODULES_FILE);
 		if (file_exists(file)) {
 			config_source.file = file;
-		} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||
+		} else if ((repo_read_index(repo) >= 0 &&
+			    repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0) ||
 			   repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {
 			config_source.blob = oidstr = xstrdup(oid_to_hex(&oid));
 			if (repo != the_repository)
diff --git a/t/ b/t/
index fcc0fb82d8..ad28e93880 100755
--- a/t/
+++ b/t/
@@ -243,18 +243,14 @@ test_expect_success 'reading nested submodules config' '
-# When this test eventually passes, before turning it into
-# test_expect_success, remember to replace the test_i18ngrep below with
-# a "test_must_be_empty warning" to be sure that the warning is actually
-# removed from the code.
-test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '
+test_expect_success 'reading nested submodules config when .gitmodules is not in the working tree' '
 	test_when_finished "git -C super/submodule checkout .gitmodules" &&
 	(cd super &&
 		echo "./nested_submodule" >expect &&
 		rm submodule/.gitmodules &&
 		test-tool submodule-nested-repo-config \
 			submodule submodule.nested_submodule.url >actual 2>warning &&
-		test_i18ngrep "nested submodules without %s in the working tree are not supported yet" warning &&
+		test_must_be_empty warning &&
 		test_cmp expect actual

  reply	other threads:[~2019-05-11 23:02 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-11 20:57 new segfault in master (6a6c0f10a70a6eb1) Eric Wong
2019-05-11 22:31 ` Jeff King
2019-05-11 23:02   ` Jeff King [this message]
2019-05-12  4:26     ` Duy Nguyen
2019-05-14 13:54     ` [PATCH] get_oid: handle NULL repo->index Jeff King
2019-05-14 23:38       ` Eric Wong
2019-05-15  1:24       ` Duy Nguyen
2019-05-15  1:46         ` Jeff King
2019-05-15  5:16           ` Junio C Hamano
2019-05-15  9:29             ` Duy Nguyen
2019-05-16  1:43               ` Junio C Hamano
2019-05-19  2:56                 ` [PATCH] repository.c: always allocate 'index' at repo init time Nguyễn Thái Ngọc Duy
2019-05-20 13:17                   ` Jeff King
2019-05-21 10:34                     ` Duy Nguyen
2019-05-21 20:58                       ` Jeff King
2019-05-28 16:07                       ` Junio C Hamano

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:

  List information:

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

  git send-email \ \ \ \ \ \ \ \

* 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

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