{"thread":{"id":"51075","subject":"new segfault in master (6a6c0f10a70a6eb1)","startedAt":"2019-05-11T20:57:15Z","lastAt":"2019-05-28T16:07:50Z","messageCount":16,"participants":["Eric Wong","Jeff King","Duy Nguyen","Junio C Hamano","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"375340","messageId":"20190511205711.tdclwrdixaau75zv@dcvr","threadId":"51075","inReplyTo":null,"subject":"new segfault in master (6a6c0f10a70a6eb1)","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-11T20:57:11Z","receivedAt":"2019-05-11T20:57:15Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"This test-tool submodule segfault seems new. Noticed it while\nchecking dmesg for other things.\nThere's also \"name-rev HEAD~4000\" (bottom), which is old, I think...\n\nCore was generated by `$WT/t/helper/test-tool submodule-nested-repo-config submodule sub'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0x0000559b12746174 in get_oid_with_context_1 (\n repo=repo@entry=0x7ffc5de3cf30, \n name=name@entry=0x559b1280a882 \":.gitmodules\", flags=flags@entry=0, \n prefix=prefix@entry=0x0, oid=oid@entry=0x7ffc5de3ce80, \n oc=oc@entry=0x7ffc5de3ce20) at sha1-name.c:1840\n1840\t\t\tif (!repo->index->cache)\n(gdb) #0 0x0000559b12746174 in get_oid_with_context_1 (\n repo=repo@entry=0x7ffc5de3cf30, \n name=name@entry=0x559b1280a882 \":.gitmodules\", flags=flags@entry=0, \n prefix=prefix@entry=0x0, oid=oid@entry=0x7ffc5de3ce80, \n oc=oc@entry=0x7ffc5de3ce20) at sha1-name.c:1840\n#1 0x0000559b12746dc3 in get_oid_with_context (oc=0x7ffc5de3ce20, \n oid=0x7ffc5de3ce80, flags=0, str=str@entry=0x559b1280a882 \":.gitmodules\", \n repo=repo@entry=0x7ffc5de3cf30) at sha1-name.c:1946\n#2 repo_get_oid (r=r@entry=0x7ffc5de3cf30, \n name=name@entry=0x559b1280a882 \":.gitmodules\", \n oid=oid@entry=0x7ffc5de3ce80) at sha1-name.c:1595\n#3 0x0000559b12753447 in config_from_gitmodules (\n fn=fn@entry=0x559b127534b0 , \n repo=repo@entry=0x7ffc5de3cf30, data=0x559b145802c0)\n at submodule-config.c:633\n#4 0x0000559b12754664 in print_config_from_gitmodules (\n repo=repo@entry=0x7ffc5de3cf30, key=)\n at submodule-config.c:742\n#5 0x0000559b126dca4f in cmd__submodule_nested_repo_config (\n argc=, argv=0x7ffc5de3d290)\n at t/helper/test-submodule-nested-repo-config.c:27\n#6 0x0000559b126d367f in cmd_main (argc=3, argv=0x7ffc5de3d290)\n at t/helper/test-tool.c:109\n#7 0x0000559b126d337a in main (argc=4, argv=0x7ffc5de3d288)\n at common-main.c:50\n(gdb) quit\n\n\nLooks like a stack overflow:\n\nCore was generated by `$WT/git name-rev HEAD~4000'.\nProgram terminated with signal SIGSEGV, Segmentation fault.\n#0 0x00007f4df1896d6a in _int_malloc (\n av=av@entry=0x7f4df1bb7b00 , bytes=bytes@entry=33)\n at malloc.c:3444\n(gdb) #0 0x00007f4df1896d6a in _int_malloc (\n av=av@entry=0x7f4df1bb7b00 , bytes=bytes@entry=33)\n at malloc.c:3444\n#1 0x00007f4df1897b68 in malloc_check (sz=32, caller=)\n at hooks.c:295\n#2 0x00005642d1240f51 in do_xmalloc (size=size@entry=32, \n gentle=gentle@entry=0) at wrapper.c:60\n#3 0x00005642d12410e7 in xmalloc (size=size@entry=32) at wrapper.c:87\n#4 0x00005642d10c75b1 in name_rev (commit=0x5642d2357bb0, \n tip_name=tip_name@entry=0x5642d245e7c0 \"master\", \n taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1044, \n distance=distance@entry=1044, from_tag=from_tag@entry=0, deref=0)\n at builtin/name-rev.c:103\n#5 0x00005642d10c74e3 in name_rev (commit=, \n tip_name=tip_name@entry=0x5642d245e7c0 \"master\", \n taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1043, \n distance=distance@entry=1043, from_tag=from_tag@entry=0, deref=0)\n at builtin/name-rev.c:138\n\n...\n\n#1047 0x00005642d10c74e3 in name_rev (commit=, \n tip_name=tip_name@entry=0x5642d245e7c0 \"master\", \n taggerdate=taggerdate@entry=1000799900, generation=generation@entry=1, \n distance=distance@entry=1, from_tag=from_tag@entry=0, deref=0)\n at builtin/name-rev.c:138\n#1048 0x00005642d10c74e3 in name_rev (commit=commit@entry=0x5642d22e6420, \n tip_name=0x5642d245e7c0 \"master\", taggerdate=taggerdate@entry=1000799900, \n generation=generation@entry=0, distance=distance@entry=0, \n from_tag=from_tag@entry=0, deref=0) at builtin/name-rev.c:138\n#1049 0x00005642d10c7889 in name_ref (path=, \n oid=0x5642d2465bd8, flags=, cb_data=)\n at builtin/name-rev.c:276\n#1050 0x00005642d11d5074 in do_for_each_repo_ref_iterator (\n r=0x5642d1546de0 , iter=0x5642d245da20, \n fn=fn@entry=0x5642d11cab10 , \n cb_data=cb_data@entry=0x7ffc71fffa90) at refs/iterator.c:418\n#1051 0x00005642d11cc7eb in do_for_each_ref (refs=, \n prefix=prefix@entry=0x5642d12a0450 \"\", \n fn=fn@entry=0x5642d10c75f0 , trim=trim@entry=0, \n flags=flags@entry=0, cb_data=cb_data@entry=0x7ffc71fffb40) at refs.c:1496\n#1052 0x00005642d11cd488 in refs_for_each_ref (cb_data=0x7ffc71fffb40, \n fn=0x5642d10c75f0 , refs=) at refs.c:1502\n#1053 for_each_ref (fn=fn@entry=0x5642d10c75f0 , \n cb_data=cb_data@entry=0x7ffc71fffb40) at refs.c:1507\n#1054 0x00005642d10c7ec5 in cmd_name_rev (argc=, \n argv=0x7ffc71fffb40, prefix=) at builtin/name-rev.c:490\n#1055 0x00005642d1070d68 in run_builtin (argv=, \n argc=, p=) at git.c:444\n#1056 handle_builtin (argc=2, argv=0x7ffc72000ac0) at git.c:675\n#1057 0x00005642d1071d1e in run_argv (argv=0x7ffc72000840, \n argcp=0x7ffc7200084c) at git.c:742\n#1058 cmd_main (argc=, argv=) at git.c:876\n#1059 0x00005642d107094a in main (argc=3, argv=0x7ffc72000ab8)\n at common-main.c:50\n(gdb) quit\n"},{"id":"375344","messageId":"20190511223120.GA25224@sigill.intra.peff.net","threadId":"51075","inReplyTo":"20190511205711.tdclwrdixaau75zv@dcvr","subject":"Re: new segfault in master (6a6c0f10a70a6eb1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-11T22:31:20Z","receivedAt":"2019-05-11T22:34:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2019 at 08:57:11PM +0000, Eric Wong wrote:\n\n> This test-tool submodule segfault seems new. Noticed it while\n> checking dmesg for other things.\n\nYeah, I hadn't seen it before. It's almost certainly the expect_failure\nadded in 2b1257e463 (t/helper: add test-submodule-nested-repo-config,\n2018-10-25), since otherwise we'd be complaining of a test failure.\n\nI know we don't expect this to do the right thing yet, but it seems like\nthere's still a bug, since the test seems to think we should output a\nnice message (and it's possible that the segfault can be triggered from\nnon-test-tool code, too).\n\n+cc the author.\n\n> There's also \"name-rev HEAD~4000\" (bottom), which is old, I think...\n> [...]\n> Looks like a stack overflow:\n\nYeah, this one is old and expected. It's also in an expect_failure. The\nCI we run things through at GitHub complains if there are any segfaults,\nand I hacked around it with the patch below.\n\nI sort of assumed nobody else cared, since they hadn't mentioned it. But\nwe could do something similar. Though note in my version the default is\n\"do not run the test\", and we'd maybe want to flip it the other way (and\nalso break up the setup step so that the succeeding test actually runs).\n\n-- >8 --\nSubject: [PATCH] t6120: mark a failing test with SEGFAULT_OK prereq\n\nUpstream recently added a test of name-rev on a deep repository, which\nshows that its recursive algorithm can blow out the stack and segfault.\nWe have several such tests already, but the twist here is that it's\nexpect_failure. So we _know_ it's going to segfault and Git's test suite\nis OK with that, until the problem is fixed.\n\nBut our CI is not so forgiving. If it sees any segfault at all, it\ninterrupts the test run and declares the whole thing a failure.\n\nLet's just skip this test by adding a prerequisite that isn't filled.\nIt's not telling us anything interesting. And if it ever gets fixed\nupstream, that will cause a conflict and we can start running it.\n\nNote that we also have to skip the test after it, which relies on the\nstate set up by the first one. This isn't a big deal, as it's not\ntesting code that we're likely to change ourselves.\n\nSigned-off-by: Jeff King \n---\n t/t6120-describe.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 1c0e8659d9..9d98b95ba6 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -310,7 +310,7 @@ test_expect_success 'describe ignoring a borken submodule' '\n \tgrep broken out\n '\n \n-test_expect_failure ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '\n+test_expect_failure ULIMIT_STACK_SIZE,SEGFAULT_OK 'name-rev works in a deep repo' '\n \ti=1 &&\n \twhile test $i -lt 8000\n \tdo\n@@ -331,7 +331,7 @@ EOF\"\n \ttest_cmp expect actual\n '\n \n-test_expect_success ULIMIT_STACK_SIZE 'describe works in a deep repo' '\n+test_expect_success ULIMIT_STACK_SIZE,SEGFAULT_OK 'describe works in a deep repo' '\n \tgit tag -f far-far-away HEAD~7999 &&\n \techo \"far-far-away\" >expect &&\n \tgit describe --tags --abbrev=0 HEAD~4000 >actual &&\n-- \n2.21.0.1388.g2b1efd806f\n\n"},{"id":"375345","messageId":"20190511230204.GA18474@sigill.intra.peff.net","threadId":"51075","inReplyTo":"20190511223120.GA25224@sigill.intra.peff.net","subject":"Re: new segfault in master (6a6c0f10a70a6eb1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-11T23:02:05Z","receivedAt":"2019-05-11T23:02:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2019 at 06:31:20PM -0400, Jeff King wrote:\n\n> On Sat, May 11, 2019 at 08:57:11PM +0000, Eric Wong wrote:\n> \n> > This test-tool submodule segfault seems new. Noticed it while\n> > checking dmesg for other things.\n> \n> Yeah, I hadn't seen it before. It's almost certainly the expect_failure\n> added in 2b1257e463 (t/helper: add test-submodule-nested-repo-config,\n> 2018-10-25), since otherwise we'd be complaining of a test failure.\n> \n> I know we don't expect this to do the right thing yet, but it seems like\n> there's still a bug, since the test seems to think we should output a\n> nice message (and it's possible that the segfault can be triggered from\n> non-test-tool code, too).\n> \n> +cc the author.\n\nActually, the plot thickens. That test _used to_ correctly expect\nfailure (well, sort of -- it greps for the string with %s, which is\nwrong!). But then more recently in d9b8b8f896 (submodule-config.c: use\nrepo_get_oid for reading .gitmodules, 2019-04-16), it started actually\ndoing the lookup in the correct repo. And that started the segfault,\nbecause nobody has actually loaded the index for the submodule.\n\nI don't think this can be triggered outside of test-tool. There are\nfour ways to get to config_from_gitmodules():\n\n - via repo_read_gitmodules(), which explicitly loads the index\n\n - via print_config_from_gitmodules(). This is called from\n submodule--helper, but only with the_repository as the argument (and\n I _think_ that the_repository->index is never NULL, because we point\n it at the_index).\n\n - via fetch_config_from_gitmodules(), which always passes\n the_repository\n\n - via update_clone_config_from_gitmodules(), likewise\n\nBut regardless, I think it makes sense to load the index on demand when\nwe need it here, which makes Antonio's original test pass (like the\npatch below).\n\nThe segfault ultimately comes from repo_get_oid(); we feed it\n\":.gitmodules\" and it blindly looks at repo->index. It's probably worth\nit being a bit more defensive and just returning \"no such entry\" if\nthere's no index to look at (it could also load on demand, I guess, but\nit seems like too low a level to be making that kind of decision).\n\nI'm out of time for now, but I'll look into cleaning this up and writing\na real commit message later.\n\n---\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4264ee216f..ad2444bcec 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -630,7 +630,8 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\tfile = repo_worktree_path(repo, GITMODULES_FILE);\n \t\tif (file_exists(file)) {\n \t\t\tconfig_source.file = file;\n-\t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n+\t\t} else if ((repo_read_index(repo) >= 0 &&\n+\t\t\t repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0) ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n \t\t\tconfig_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n \t\t\tif (repo != the_repository)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex fcc0fb82d8..ad28e93880 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -243,18 +243,14 @@ test_expect_success 'reading nested submodules config' '\n \t)\n '\n \n-# When this test eventually passes, before turning it into\n-# test_expect_success, remember to replace the test_i18ngrep below with\n-# a \"test_must_be_empty warning\" to be sure that the warning is actually\n-# removed from the code.\n-test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '\n+test_expect_success 'reading nested submodules config when .gitmodules is not in the working tree' '\n \ttest_when_finished \"git -C super/submodule checkout .gitmodules\" &&\n \t(cd super &&\n \t\techo \"./nested_submodule\" >expect &&\n \t\trm submodule/.gitmodules &&\n \t\ttest-tool submodule-nested-repo-config \\\n \t\t\tsubmodule submodule.nested_submodule.url >actual 2>warning &&\n-\t\ttest_i18ngrep \"nested submodules without %s in the working tree are not supported yet\" warning &&\n+\t\ttest_must_be_empty warning &&\n \t\ttest_cmp expect actual\n \t)\n '\n"},{"id":"375349","messageId":"CACsJy8A9kZJVgsxTbsrSt+_CZd6v9fY606V4cds48gMFs_iTxg@mail.gmail.com","threadId":"51075","inReplyTo":"20190511230204.GA18474@sigill.intra.peff.net","subject":"Re: new segfault in master (6a6c0f10a70a6eb1)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-12T04:26:56Z","receivedAt":"2019-05-12T04:29:23Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 12, 2019 at 6:02 AM Jeff King wrote:\n>\n> On Sat, May 11, 2019 at 06:31:20PM -0400, Jeff King wrote:\n>\n> > On Sat, May 11, 2019 at 08:57:11PM +0000, Eric Wong wrote:\n> >\n> > > This test-tool submodule segfault seems new. Noticed it while\n> > > checking dmesg for other things.\n> >\n> > Yeah, I hadn't seen it before. It's almost certainly the expect_failure\n> > added in 2b1257e463 (t/helper: add test-submodule-nested-repo-config,\n> > 2018-10-25), since otherwise we'd be complaining of a test failure.\n> >\n> > I know we don't expect this to do the right thing yet, but it seems like\n> > there's still a bug, since the test seems to think we should output a\n> > nice message (and it's possible that the segfault can be triggered from\n> > non-test-tool code, too).\n> >\n> > +cc the author.\n>\n> Actually, the plot thickens. That test _used to_ correctly expect\n> failure (well, sort of -- it greps for the string with %s, which is\n> wrong!). But then more recently in d9b8b8f896 (submodule-config.c: use\n> repo_get_oid for reading .gitmodules, 2019-04-16), it started actually\n> doing the lookup in the correct repo. And that started the segfault,\n> because nobody has actually loaded the index for the submodule.\n>\n> I don't think this can be triggered outside of test-tool. There are\n> four ways to get to config_from_gitmodules():\n>\n> - via repo_read_gitmodules(), which explicitly loads the index\n>\n> - via print_config_from_gitmodules(). This is called from\n> submodule--helper, but only with the_repository as the argument (and\n> I _think_ that the_repository->index is never NULL, because we point\n> it at the_index).\n>\n> - via fetch_config_from_gitmodules(), which always passes\n> the_repository\n>\n> - via update_clone_config_from_gitmodules(), likewise\n>\n> But regardless, I think it makes sense to load the index on demand when\n> we need it here, which makes Antonio's original test pass (like the\n> patch below).\n>\n> The segfault ultimately comes from repo_get_oid(); we feed it\n> \":.gitmodules\" and it blindly looks at repo->index. It's probably worth\n> it being a bit more defensive and just returning \"no such entry\" if\n> there's no index to look at (it could also load on demand, I guess, but\n> it seems like too low a level to be making that kind of decision).\n>\n> I'm out of time for now, but I'll look into cleaning this up and writing\n> a real commit message later.\n>\n> ---\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 4264ee216f..ad2444bcec 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -630,7 +630,8 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n> file = repo_worktree_path(repo, GITMODULES_FILE);\n> if (file_exists(file)) {\n> config_source.file = file;\n> - } else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n> + } else if ((repo_read_index(repo) >= 0 &&\n> + repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0) ||\n> repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n> config_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n> if (repo != the_repository)\n> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\n> index fcc0fb82d8..ad28e93880 100755\n> --- a/t/t7411-submodule-config.sh\n> +++ b/t/t7411-submodule-config.sh\n> @@ -243,18 +243,14 @@ test_expect_success 'reading nested submodules config' '\n> )\n> '\n>\n> -# When this test eventually passes, before turning it into\n> -# test_expect_success, remember to replace the test_i18ngrep below with\n> -# a \"test_must_be_empty warning\" to be sure that the warning is actually\n> -# removed from the code.\n> -test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '\n> +test_expect_success 'reading nested submodules config when .gitmodules is not in the working tree' '\n\nI did miss this test. Yeah your fix makes sense.\n\n> test_when_finished \"git -C super/submodule checkout .gitmodules\" &&\n> (cd super &&\n> echo \"./nested_submodule\" >expect &&\n> rm submodule/.gitmodules &&\n> test-tool submodule-nested-repo-config \\\n> submodule submodule.nested_submodule.url >actual 2>warning &&\n> - test_i18ngrep \"nested submodules without %s in the working tree are not supported yet\" warning &&\n> + test_must_be_empty warning &&\n> test_cmp expect actual\n> )\n> '\n\n\n\n-- \nDuy\n"},{"id":"375536","messageId":"20190514135455.GA17927@sigill.intra.peff.net","threadId":"51075","inReplyTo":"20190511230204.GA18474@sigill.intra.peff.net","subject":"[PATCH] get_oid: handle NULL repo->index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T13:54:55Z","receivedAt":"2019-05-14T13:54:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2019 at 07:02:05PM -0400, Jeff King wrote:\n\n> But regardless, I think it makes sense to load the index on demand when\n> we need it here, which makes Antonio's original test pass (like the\n> patch below).\n> \n> The segfault ultimately comes from repo_get_oid(); we feed it\n> \":.gitmodules\" and it blindly looks at repo->index. It's probably worth\n> it being a bit more defensive and just returning \"no such entry\" if\n> there's no index to look at (it could also load on demand, I guess, but\n> it seems like too low a level to be making that kind of decision).\n\nThis turned out to be even simpler than the patch I posted earlier.\nAfter polishing up my submodule-config fix, I looked at teaching\nget_oid() to behave differently. And it turns out that it already tries\nto load the index on demand! There's just a silly mistake in the code to\ncheck whether the index is already initialized.\n\nOnce we fix that, we don't need to handle this specifically in the\nsubmodule code. So here's a simpler, revised patch:\n\n-- >8 --\nSubject: [PATCH] get_oid: handle NULL repo->index\n\nWhen get_oid() and its helpers see an index name like \":.gitmodules\",\nthey try to load the index on demand, like:\n\n if (repo->index->cache)\n\trepo_read_index(repo);\n\nHowever, that misses the case when \"repo->index\" itself is NULL; we'll\nsegfault in the conditional.\n\nThis never happens with the_repository; there we always point its index\nfield to &the_index. But a submodule repository may have a NULL index\nfield until somebody calls repo_read_index().\n\nThis bug is triggered by t7411, but it was hard to notice because it's\nin an expect_failure block. That test was added by 2b1257e463 (t/helper:\nadd test-submodule-nested-repo-config, 2018-10-25). Back then we had no\neasy way to access the .gitmodules blob of a submodule repo, so we\nexpected (and got) an error message to that effect. Later, d9b8b8f896\n(submodule-config.c: use repo_get_oid for reading .gitmodules,\n2019-04-16) started looking in the correct repo, which is when we\nstarted triggering the segfault.\n\nWith this fix, the test starts passing (once we clean it up as its\ncomment instructs).\n\nNote that as far as I know, this bug could not be triggered outside of\nthe test suite. It requires resolving an index name in a submodule, and\nall of the code paths (aside from test-tool) which do that either load\nthe index themselves, or always pass the_repository.\n\nUltimately it comes from 3a7a698e93 (sha1-name.c: remove implicit\ndependency on the_index, 2019-01-12), which replaced a check of\n\"the_index.cache\" with \"repo->index->cache\". So even if there is another\nway to trigger it, it wouldn't affect any versions before then.\n\nSigned-off-by: Jeff King \n---\nArguably this code should just unconditionally call repo_read_index(),\nwhich should be a noop if the index is already loaded. But I wanted to\ndo the minimal fix here, without getting into any subtle differences\nbetween what checking index->cache versus index->initialized might mean.\nAnybody who wants to dig into that is welcome to make a patch on top. :)\n\nI also wondered if we should simply allocate an empty index whenever we\nhave a non-toplevel \"struct repository\", which might be less surprising\nto other callers. I don't have a strong opinion either way. I did grep\naround for other callers which might have similar problems, but couldn't\nfind any.\n\n sha1-name.c | 2 +-\n t/t7411-submodule-config.sh | 8 ++------\n 2 files changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 775a73d8ad..455e9fb1ea 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1837,7 +1837,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\tif (flags & GET_OID_RECORD_PATH)\n \t\t\toc->path = xstrdup(cp);\n \n-\t\tif (!repo->index->cache)\n+\t\tif (!repo->index || !repo->index->cache)\n \t\t\trepo_read_index(repo);\n \t\tpos = index_name_pos(repo->index, cp, namelen);\n \t\tif (pos < 0)\ndiff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh\nindex fcc0fb82d8..ad28e93880 100755\n--- a/t/t7411-submodule-config.sh\n+++ b/t/t7411-submodule-config.sh\n@@ -243,18 +243,14 @@ test_expect_success 'reading nested submodules config' '\n \t)\n '\n \n-# When this test eventually passes, before turning it into\n-# test_expect_success, remember to replace the test_i18ngrep below with\n-# a \"test_must_be_empty warning\" to be sure that the warning is actually\n-# removed from the code.\n-test_expect_failure 'reading nested submodules config when .gitmodules is not in the working tree' '\n+test_expect_success 'reading nested submodules config when .gitmodules is not in the working tree' '\n \ttest_when_finished \"git -C super/submodule checkout .gitmodules\" &&\n \t(cd super &&\n \t\techo \"./nested_submodule\" >expect &&\n \t\trm submodule/.gitmodules &&\n \t\ttest-tool submodule-nested-repo-config \\\n \t\t\tsubmodule submodule.nested_submodule.url >actual 2>warning &&\n-\t\ttest_i18ngrep \"nested submodules without %s in the working tree are not supported yet\" warning &&\n+\t\ttest_must_be_empty warning &&\n \t\ttest_cmp expect actual\n \t)\n '\n-- \n2.21.0.1388.g2b1efd806f\n\n"},{"id":"375571","messageId":"20190514233809.7wnlbb4s6cjhjv63@dcvr","threadId":"51075","inReplyTo":"20190514135455.GA17927@sigill.intra.peff.net","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-05-14T23:38:09Z","receivedAt":"2019-05-14T23:38:12Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> +++ b/sha1-name.c\n> @@ -1837,7 +1837,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> \t\tif (flags & GET_OID_RECORD_PATH)\n> \t\t\toc->path = xstrdup(cp);\n> \n> -\t\tif (!repo->index->cache)\n> +\t\tif (!repo->index || !repo->index->cache)\n> \t\t\trepo_read_index(repo);\n\nAwesome, looks obviously correct and can confirm it fixes the\nnew segfault :>\n\nNow I'm kinda wondering if some static checker coulda/shoulda\nspotted that, sooner.\n"},{"id":"375581","messageId":"CACsJy8AvsyOz2G1zjRjpKYVZ0DLKj02-v=hXJHS0BRHnxoeWAw@mail.gmail.com","threadId":"51075","inReplyTo":"20190514135455.GA17927@sigill.intra.peff.net","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-15T01:24:34Z","receivedAt":"2019-05-15T01:25:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 8:54 PM Jeff King wrote:\n> diff --git a/sha1-name.c b/sha1-name.c\n> index 775a73d8ad..455e9fb1ea 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1837,7 +1837,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> if (flags & GET_OID_RECORD_PATH)\n> oc->path = xstrdup(cp);\n>\n> - if (!repo->index->cache)\n> + if (!repo->index || !repo->index->cache)\n> repo_read_index(repo);\n\nWe could even drop the \"if\" and call repo_read_index()\nunconditionally. If the index is already read, it will be no-op\n(forcing a reread has always been discard_index(); read_index();)\n\nThanks for catching this by the way. I'll need to go through all\nthe_index conversion to see if I left similar traps like this.\n-- \nDuy\n"},{"id":"375586","messageId":"20190515014622.GB13255@sigill.intra.peff.net","threadId":"51075","inReplyTo":"CACsJy8AvsyOz2G1zjRjpKYVZ0DLKj02-v=hXJHS0BRHnxoeWAw@mail.gmail.com","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-15T01:46:22Z","receivedAt":"2019-05-15T01:46:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2019 at 08:24:34AM +0700, Duy Nguyen wrote:\n\n> On Tue, May 14, 2019 at 8:54 PM Jeff King wrote:\n> > diff --git a/sha1-name.c b/sha1-name.c\n> > index 775a73d8ad..455e9fb1ea 100644\n> > --- a/sha1-name.c\n> > +++ b/sha1-name.c\n> > @@ -1837,7 +1837,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> > if (flags & GET_OID_RECORD_PATH)\n> > oc->path = xstrdup(cp);\n> >\n> > - if (!repo->index->cache)\n> > + if (!repo->index || !repo->index->cache)\n> > repo_read_index(repo);\n> \n> We could even drop the \"if\" and call repo_read_index()\n> unconditionally. If the index is already read, it will be no-op\n> (forcing a reread has always been discard_index(); read_index();)\n\nI think you missed my bit after the \"---\":\n\n Arguably this code should just unconditionally call repo_read_index(),\n which should be a noop if the index is already loaded. But I wanted to\n do the minimal fix here, without getting into any subtle differences\n between what checking index->cache versus index->initialized might\n mean. Anybody who wants to dig into that is welcome to make a patch\n on top. :)\n\n> Thanks for catching this by the way. I'll need to go through all\n> the_index conversion to see if I left similar traps like this.\n\nYeah. I could not find any others, but it would not hurt to have a\nsecond set of eyes.\n\nAlso from my earlier message, if you missed it:\n\n I also wondered if we should simply allocate an empty index whenever\n we have a non-toplevel \"struct repository\", which might be less\n surprising to other callers. I don't have a strong opinion either way.\n I did grep around for other callers which might have similar problems,\n but couldn't find any.\n\n-Peff\n"},{"id":"375594","messageId":"xmqqh89w70w8.fsf@gitster-ct.c.googlers.com","threadId":"51075","inReplyTo":"20190515014622.GB13255@sigill.intra.peff.net","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-15T05:16:39Z","receivedAt":"2019-05-15T05:16:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Also from my earlier message, if you missed it:\n>\n> I also wondered if we should simply allocate an empty index whenever\n> we have a non-toplevel \"struct repository\", which might be less\n> surprising to other callers. I don't have a strong opinion either way.\n> I did grep around for other callers which might have similar problems,\n> but couldn't find any.\n\nThat is an approach to make it harder to make mistakes by accepting\npossibly a small wasted resource; but at that point, I think calling\nrepo_read_index() unconditionally from here and similar places would\nbe a simpler fix in the same spirit.\n"},{"id":"375615","messageId":"CACsJy8BS8NR6aZR29VLYUrRjBE_oyzH=L6X7CSpCO9G+sPjcbA@mail.gmail.com","threadId":"51075","inReplyTo":"xmqqh89w70w8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-15T09:29:12Z","receivedAt":"2019-05-15T09:29:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 15, 2019 at 12:16 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > Also from my earlier message, if you missed it:\n> >\n> > I also wondered if we should simply allocate an empty index whenever\n> > we have a non-toplevel \"struct repository\", which might be less\n> > surprising to other callers. I don't have a strong opinion either way.\n> > I did grep around for other callers which might have similar problems,\n> > but couldn't find any.\n>\n> That is an approach to make it harder to make mistakes by accepting\n> possibly a small wasted resource; but at that point, I think calling\n> repo_read_index() unconditionally from here and similar places would\n> be a simpler fix in the same spirit.\n\nFor repo_read_index() case, maybe. But we have a lot of\n\"r(epo)->index->something\". All or most of these references\ntraditionally are the_index.something, which is always safe to\ndereference. Submodule repos with the optionally NULL repo->index\nbreak this assumption.\n\nSo either we audit all the code and make sure \"repo->index\" cannot be\nNULL because repo_read_index() has been called before (and may have to\nrepeat auditing), or we just stick to the old assumption and make sure\nrepo->index is not NULL from the beginning. This makes me think the\nsmall extra resource is worth it. Much less time will be spent on\nsimilar issues now and in the future.\n\nPS. Sorry Jeff for the noise. I should have waited until I come home\nand can read your mail more carefully.\n-- \nDuy\n"},{"id":"375683","messageId":"xmqqftpf5g3d.fsf@gitster-ct.c.googlers.com","threadId":"51075","inReplyTo":"CACsJy8BS8NR6aZR29VLYUrRjBE_oyzH=L6X7CSpCO9G+sPjcbA@mail.gmail.com","subject":"Re: [PATCH] get_oid: handle NULL repo->index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-16T01:43:34Z","receivedAt":"2019-05-16T01:50:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> That is an approach to make it harder to make mistakes by accepting\n>> possibly a small wasted resource; but at that point, I think calling\n>> repo_read_index() unconditionally from here and similar places would\n>> be a simpler fix in the same spirit.\n>\n> For repo_read_index() case, maybe. But we have a lot of\n> \"r(epo)->index->something\". All or most of these references\n> traditionally are the_index.something, which is always safe to\n> dereference. Submodule repos with the optionally NULL repo->index\n> break this assumption.\n\nAh, good point. Thanks for a dose of sanity.\n"},{"id":"375884","messageId":"20190519025636.24819-1-pclouds@gmail.com","threadId":"51075","inReplyTo":"xmqqftpf5g3d.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] repository.c: always allocate 'index' at repo init time","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-19T02:56:36Z","receivedAt":"2019-05-19T18:17:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There are two ways a 'struct repository' could be initialized before\nusing: via initialize_the_repository() and repo_init().\n\nThe first way always initializes 'index' field because that's how it is\nbefore the introduction of 'struct repository'. Back then 'the_index' is\nalways available (even if not loaded). The second way however leaves\n'index' NULL and relies on repo_read_index() to allocate it on demand.\n\nThe problem with the second way is that, the majority of our code base\nwas written with 'the_index' (i.e. the first way) in mind, where\ndereferencing 'the_index' (or the 'index' field now) is always\nsafe.\n\nThe second way breaks this assumption. The 'index' field can be NULL\nuntil loading from disk, which could lead to segfaults like\n581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14).\n\nWe have two options to handle this: either we audit the entire code\nbase, adding 'is index NULL' when needed, or we make sure 'index' is\nnever NULL to begin with.\n\nThis patch goes with the second option, making sure that 'index' is\nalways allocated after initialization. It's less effort than the first\none, and also safer because you could still miss things during the code\naudit. The extra allocation cost is not a real concern.\n\nThe 'index' field is still freed and reset to NULL in repo_clear(). But\nafter that call, a lot more is missing in 'repo' and it can never be\nused again without going through reinitialization phase. So it should be\nfine.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n repository.c | 3 ++-\n repository.h | 4 ++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/repository.c b/repository.c\nindex 682c239fe3..ca58692504 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -160,6 +160,7 @@ int repo_init(struct repository *repo,\n \tstruct repository_format format = REPOSITORY_FORMAT_INIT;\n \tmemset(repo, 0, sizeof(*repo));\n \n+\trepo->index = xcalloc(1, sizeof(*repo->index));\n \trepo->objects = raw_object_store_new();\n \trepo->parsed_objects = parsed_object_pool_new();\n \n@@ -262,7 +263,7 @@ void repo_clear(struct repository *repo)\n int repo_read_index(struct repository *repo)\n {\n \tif (!repo->index)\n-\t\trepo->index = xcalloc(1, sizeof(*repo->index));\n+\t\tBUG(\"the repo hasn't been setup\");\n \n \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n }\ndiff --git a/repository.h b/repository.h\nindex 4fb6a5885f..75c4f68b22 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -85,6 +85,7 @@ struct repository {\n \n \t/*\n \t * Repository's in-memory index.\n+\t * Cannot be NULL after initialization.\n \t * 'repo_read_index()' can be used to populate 'index'.\n \t */\n \tstruct index_state *index;\n@@ -132,6 +133,9 @@ struct submodule;\n int repo_submodule_init(struct repository *subrepo,\n \t\t\tstruct repository *superproject,\n \t\t\tconst struct submodule *sub);\n+/*\n+ * Release all resources in 'repo'. 'repo' cannot be used again.\n+ */\n void repo_clear(struct repository *repo);\n \n /*\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"375920","messageId":"20190520131702.GB13474@sigill.intra.peff.net","threadId":"51075","inReplyTo":"20190519025636.24819-1-pclouds@gmail.com","subject":"Re: [PATCH] repository.c: always allocate 'index' at repo init time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T13:17:03Z","receivedAt":"2019-05-20T13:17:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 19, 2019 at 09:56:36AM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> This patch goes with the second option, making sure that 'index' is\n> always allocated after initialization. It's less effort than the first\n> one, and also safer because you could still miss things during the code\n> audit. The extra allocation cost is not a real concern.\n\nI think this direction makes sense.\n\nThe patch looks good, though I wonder if we could simplify even further\nby just embedding an index into the repository object. The purpose of\nhaving it as a pointer, I think, is so that the_repository can point to\nthe_index. But we could possibly hide the latter behind some macro\ntrickery like:\n\n #define the_index (the_repository->index)\n\nI spent a few minutes on a proof of concept patch, but it gets a bit\nhairy:\n\n 1. There are some circular dependencies in the header files. We'd need\n repository.h to depend on cache.h to get the definition of\n index_state, but the latter includes repository.h. We'd need to\n break the index bits out of cache.h into index.h, which in turn\n requires breaking out some other parts. I did a sloppy job of it in\n the patch below.\n\n 2. There are hundreds of spots that need to swap out \"repo->index\" for\n \"&repo->index\". In the patch below I just did enough to compile\n archive-zip.o, to illustrate. :)\n\nSo it's definitely non-trivial to go that way. I'm not sure if it's\nworth the effort to switch at this point, but even if it is, your patch\nseems like a good thing to do in the meantime.\n\nEither way, I think we could probably revert the non-test portion of my\n581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14) after this.\n\n-Peff\n\n---\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 4d66b5be6e..517e203483 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -353,7 +353,7 @@ static int write_zip_entry(struct archiver_args *args,\n \t\t\t\treturn error(_(\"cannot read %s\"),\n \t\t\t\t\t oid_to_hex(oid));\n \t\t\tcrc = crc32(crc, buffer, size);\n-\t\t\tis_binary = entry_is_binary(args->repo->index,\n+\t\t\tis_binary = entry_is_binary(&args->repo->index,\n \t\t\t\t\t\t path_without_prefix,\n \t\t\t\t\t\t buffer, size);\n \t\t\tout = buffer;\n@@ -430,7 +430,7 @@ static int write_zip_entry(struct archiver_args *args,\n \t\t\t\tbreak;\n \t\t\tcrc = crc32(crc, buf, readlen);\n \t\t\tif (is_binary == -1)\n-\t\t\t\tis_binary = entry_is_binary(args->repo->index,\n+\t\t\t\tis_binary = entry_is_binary(&args->repo->index,\n \t\t\t\t\t\t\t path_without_prefix,\n \t\t\t\t\t\t\t buf, readlen);\n \t\t\twrite_or_die(1, buf, readlen);\n@@ -463,7 +463,7 @@ static int write_zip_entry(struct archiver_args *args,\n \t\t\t\tbreak;\n \t\t\tcrc = crc32(crc, buf, readlen);\n \t\t\tif (is_binary == -1)\n-\t\t\t\tis_binary = entry_is_binary(args->repo->index,\n+\t\t\t\tis_binary = entry_is_binary(&args->repo->index,\n \t\t\t\t\t\t\t path_without_prefix,\n \t\t\t\t\t\t\t buf, readlen);\n \ndiff --git a/cache.h b/cache.h\nindex b4bb2e2c11..d0450025e1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -17,6 +17,7 @@\n #include \"sha1-array.h\"\n #include \"repository.h\"\n #include \"mem-pool.h\"\n+#include \"oid.h\"\n \n #include \n typedef struct git_zstream {\n@@ -43,28 +44,6 @@ int git_deflate_end_gently(git_zstream *);\n int git_deflate(git_zstream *, int flush);\n unsigned long git_deflate_bound(git_zstream *, unsigned long);\n \n-/* The length in bytes and in hex digits of an object name (SHA-1 value). */\n-#define GIT_SHA1_RAWSZ 20\n-#define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)\n-/* The block size of SHA-1. */\n-#define GIT_SHA1_BLKSZ 64\n-\n-/* The length in bytes and in hex digits of an object name (SHA-256 value). */\n-#define GIT_SHA256_RAWSZ 32\n-#define GIT_SHA256_HEXSZ (2 * GIT_SHA256_RAWSZ)\n-/* The block size of SHA-256. */\n-#define GIT_SHA256_BLKSZ 64\n-\n-/* The length in byte and in hex digits of the largest possible hash value. */\n-#define GIT_MAX_RAWSZ GIT_SHA256_RAWSZ\n-#define GIT_MAX_HEXSZ GIT_SHA256_HEXSZ\n-/* The largest possible block size for any supported hash. */\n-#define GIT_MAX_BLKSZ GIT_SHA256_BLKSZ\n-\n-struct object_id {\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n-};\n-\n #define the_hash_algo the_repository->hash_algo\n \n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n@@ -143,16 +122,6 @@ struct cache_header {\n #define INDEX_FORMAT_LB 2\n #define INDEX_FORMAT_UB 4\n \n-/*\n- * The \"cache_time\" is just the low 32 bits of the\n- * time. It doesn't matter if it overflows - we only\n- * check it for equality in the 32 bits we save.\n- */\n-struct cache_time {\n-\tuint32_t sec;\n-\tuint32_t nsec;\n-};\n-\n struct stat_data {\n \tstruct cache_time sd_ctime;\n \tstruct cache_time sd_mtime;\n@@ -326,32 +295,6 @@ static inline unsigned int canon_mode(unsigned int mode)\n #define UNTRACKED_CHANGED\t(1 << 7)\n #define FSMONITOR_CHANGED\t(1 << 8)\n \n-struct split_index;\n-struct untracked_cache;\n-\n-struct index_state {\n-\tstruct cache_entry **cache;\n-\tunsigned int version;\n-\tunsigned int cache_nr, cache_alloc, cache_changed;\n-\tstruct string_list *resolve_undo;\n-\tstruct cache_tree *cache_tree;\n-\tstruct split_index *split_index;\n-\tstruct cache_time timestamp;\n-\tunsigned name_hash_initialized : 1,\n-\t\t initialized : 1,\n-\t\t drop_cache_tree : 1,\n-\t\t updated_workdir : 1,\n-\t\t updated_skipworktree : 1,\n-\t\t fsmonitor_has_run_once : 1;\n-\tstruct hashmap name_hash;\n-\tstruct hashmap dir_hash;\n-\tstruct object_id oid;\n-\tstruct untracked_cache *untracked;\n-\tuint64_t fsmonitor_last_update;\n-\tstruct ewah_bitmap *fsmonitor_dirty;\n-\tstruct mem_pool *ce_mem_pool;\n-};\n-\n /* Name hashing */\n int test_lazy_init_name_hash(struct index_state *istate, int try_threaded);\n void add_name_hash(struct index_state *istate, struct cache_entry *ce);\ndiff --git a/repository.h b/repository.h\nindex 4fb6a5885f..3371afceaa 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -1,11 +1,11 @@\n #ifndef REPOSITORY_H\n #define REPOSITORY_H\n \n+#include \"index.h\"\n #include \"path.h\"\n \n struct config_set;\n struct git_hash_algo;\n-struct index_state;\n struct lock_file;\n struct pathspec;\n struct raw_object_store;\n@@ -87,7 +87,7 @@ struct repository {\n \t * Repository's in-memory index.\n \t * 'repo_read_index()' can be used to populate 'index'.\n \t */\n-\tstruct index_state *index;\n+\tstruct index_state index;\n \n \t/* Repository's current hash algorithm, as serialized on disk. */\n \tconst struct git_hash_algo *hash_algo;\n"},{"id":"375986","messageId":"CACsJy8CoauTdJ1huU=w2YNbw53iea5U304yAu2oCUuTvFRaV7w@mail.gmail.com","threadId":"51075","inReplyTo":"20190520131702.GB13474@sigill.intra.peff.net","subject":"Re: [PATCH] repository.c: always allocate 'index' at repo init time","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-21T10:34:02Z","receivedAt":"2019-05-21T10:34:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, May 20, 2019 at 8:17 PM Jeff King wrote:\n> The patch looks good, though I wonder if we could simplify even further\n> by just embedding an index into the repository object. The purpose of\n> having it as a pointer, I think, is so that the_repository can point to\n> the_index. But we could possibly hide the latter behind some macro\n> trickery like:\n>\n> #define the_index (the_repository->index)\n>\n> I spent a few minutes on a proof of concept patch, but it gets a bit\n> hairy:\n>\n> 1. There are some circular dependencies in the header files. We'd need\n> repository.h to depend on cache.h to get the definition of\n> index_state, but the latter includes repository.h. We'd need to\n> break the index bits out of cache.h into index.h, which in turn\n> requires breaking out some other parts. I did a sloppy job of it in\n> the patch below.\n>\n> 2. There are hundreds of spots that need to swap out \"repo->index\" for\n> \"&repo->index\". In the patch below I just did enough to compile\n> archive-zip.o, to illustrate. :)\n\nYou are more thorough than me. I saw #2 first and immediately backed\noff (partly for a selfish reason: I have plenty of the_repo conversion\npatches in queue and anything touching \"repo\" may delay those patches\neven more).\n\nThere's also #3 but this one is minor. So far 'struct repo' is more of\na glue of things. Embedding index_state in it while leaving\nobject_store, ref_store... pointers feels inconsistent and a bit\nweird. It's not a strong reason for making index_state a pointer too,\nbut if we have to deal with pointers anyway...\n\n> So it's definitely non-trivial to go that way. I'm not sure if it's\n> worth the effort to switch at this point, but even if it is, your patch\n> seems like a good thing to do in the meantime.\n>\n> Either way, I think we could probably revert the non-test portion of my\n> 581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14) after this.\n\nYeah. I'm thinking of doing that after, scanning for similar lines\ntoo. But it looks like it's the only one. Will fix in v2.\n-- \nDuy\n"},{"id":"376014","messageId":"20190521205806.GA14807@sigill.intra.peff.net","threadId":"51075","inReplyTo":"CACsJy8CoauTdJ1huU=w2YNbw53iea5U304yAu2oCUuTvFRaV7w@mail.gmail.com","subject":"Re: [PATCH] repository.c: always allocate 'index' at repo init time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-21T20:58:06Z","receivedAt":"2019-05-21T20:58:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 21, 2019 at 05:34:02PM +0700, Duy Nguyen wrote:\n\n> > 2. There are hundreds of spots that need to swap out \"repo->index\" for\n> > \"&repo->index\". In the patch below I just did enough to compile\n> > archive-zip.o, to illustrate. :)\n> \n> You are more thorough than me. I saw #2 first and immediately backed\n> off (partly for a selfish reason: I have plenty of the_repo conversion\n> patches in queue and anything touching \"repo\" may delay those patches\n> even more).\n\nYeah, that's true, it would be disruptive.\n\n> There's also #3 but this one is minor. So far 'struct repo' is more of\n> a glue of things. Embedding index_state in it while leaving\n> object_store, ref_store... pointers feels inconsistent and a bit\n> weird. It's not a strong reason for making index_state a pointer too,\n> but if we have to deal with pointers anyway...\n\nAnd yeah, I agree it would nice for it to all be consistent. Let's leave\nit at your patch for now, and we can think about refactoring this later.\n\n-Peff\n"},{"id":"376312","messageId":"xmqqblzm5zqn.fsf@gitster-ct.c.googlers.com","threadId":"51075","inReplyTo":"CACsJy8CoauTdJ1huU=w2YNbw53iea5U304yAu2oCUuTvFRaV7w@mail.gmail.com","subject":"Re: [PATCH] repository.c: always allocate 'index' at repo init time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T16:07:44Z","receivedAt":"2019-05-28T16:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, May 20, 2019 at 8:17 PM Jeff King wrote:\n>> The patch looks good, though I wonder if we could simplify even further\n>> by just embedding an index into the repository object. The purpose of\n>> having it as a pointer, I think, is so that the_repository can point to\n>> the_index. But we could possibly hide the latter behind some macro\n>> trickery like:\n>>\n>> #define the_index (the_repository->index)\n> ...\n>> So it's definitely non-trivial to go that way. I'm not sure if it's\n>> worth the effort to switch at this point, but even if it is, your patch\n>> seems like a good thing to do in the meantime.\n\nYeah, the fact that the_reopsitory->index is not an embedded\ninstance has bothered me from the very beginning, and I am happy to\nsee others share the same feeling ;-)\n\n>> Either way, I think we could probably revert the non-test portion of my\n>> 581d2fd9f2 (get_oid: handle NULL repo->index, 2019-05-14) after this.\n>\n> Yeah. I'm thinking of doing that after, scanning for similar lines\n> too. But it looks like it's the only one. Will fix in v2.\n\nThanks.\n"}]}