{"thread":{"id":"48137","subject":"Re: Null pointer dereference in git-submodule","startedAt":"2018-03-25T09:50:15Z","lastAt":"2018-03-28T22:35:58Z","messageCount":14,"participants":["Jeremy Feusi","René Scharfe","Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"342880","messageId":"20180325095046.GA687@feusi.co","threadId":"48137","inReplyTo":null,"subject":"Re: Null pointer dereference in git-submodule","fromName":"Jeremy Feusi","fromEmail":"jeremy@feusi.co","sentAt":"2018-03-25T09:50:46Z","receivedAt":"2018-03-25T09:50:15Z","isPatch":false,"sender":{"key":"jeremy@feusi.co","avatar":null},"body":"\nReply-To: \n\nHmm... That's weird. I can reproduce it on 3 independant systems with\nversions 2.16.2 up, although it does not work with version 2.11.0.\nAnyway, I figured out how to reproduce this bug. It is caused when a\nsubmodule is added and then the directory it resides in is moved or\ndeleted without commiting. For example:\n\ngit init\ngit submodule add https://github.com/git/git git\nmv git git.BAK\ngit submodule status #this command segfaults\n\ncheers\nJeremy\n\nIn-Reply-To: \n\nOn Sat, Mar 24, 2018 at 09:42:57PM +0100, René Scharfe wrote:\n> Am 24.03.2018 um 18:42 schrieb Jeremy Feusi:\n> > Hi,\n> > While bootstrapping a gnu repository I noticed that git segfaulted when\n> > called as \"git submodule status\". After compiling git with address\n> > sanitizer and minimizing the directory I finally narrowed it down to the\n> > files which I have attached as a tar archive. Here is a detailed backtrace:\n> > \n> > AddressSanitizer:DEADLYSIGNAL\n> > =================================================================\n> > ==63400==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000c27a93 bp 0x7ffdcb4eec10 sp 0x7ffdcb4eeb80 T0)\n> > ==63400==The signal is caused by a READ memory access.\n> > ==63400==Hint: address points to the zero page.\n> > #0 0xc27a92 in refs_read_raw_ref /home/jfe/git/refs.c:1451:20\n> > #1 0xc174a6 in refs_resolve_ref_unsafe /home/jfe/git/refs.c:1493:7\n> > #2 0xc1826a in refs_read_ref_full /home/jfe/git/refs.c:224:6\n> > #3 0xc26d53 in refs_head_ref /home/jfe/git/refs.c:1314:7\n> > #4 0x8071e6 in status_submodule /home/jfe/git/builtin/submodule--helper.c:658:7\n> > #5 0x806a89 in status_submodule_cb /home/jfe/git/builtin/submodule--helper.c:699:2\n> > #6 0x80523e in for_each_listed_submodule /home/jfe/git/builtin/submodule--helper.c:438:3\n> > #7 0x7f7e9a in module_status /home/jfe/git/builtin/submodule--helper.c:732:2\n> > #8 0x7efd69 in cmd_submodule__helper /home/jfe/git/builtin/submodule--helper.c:1859:11\n> > #9 0x51e024 in run_builtin /home/jfe/git/git.c:346:11\n> > #10 0x5192c2 in handle_builtin /home/jfe/git/git.c:554:8\n> > #11 0x51d0f0 in run_argv /home/jfe/git/git.c:606:4\n> > #12 0x518600 in cmd_main /home/jfe/git/git.c:683:19\n> > #13 0x8501d6 in main /home/jfe/git/common-main.c:43:9\n> > #14 0x7f49fdaf2f29 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20f29)\n> > #15 0x41f4b9 in _start (/home/jfe/git/inst/libexec/git-core/git+0x41f4b9)\n> > \n> > AddressSanitizer can not provide additional info.\n> > SUMMARY: AddressSanitizer: SEGV /home/jfe/git/refs.c:1451:20 in refs_read_raw_ref\n> > ==63400==ABORTING\n> > \n> > As mentioned above, this bug is triggered by issuing the command\n> > \"git submodule status\" while in the attached directory.\n> > \n> > This bug was confirmed on Debian with version 2.16.1 and\n> > 2.17.0.rc1.35.g90bbd502d as well as on Arch Linux with version 2.16.2\n> > where further output is given by git:\n> > \n> > /usr/lib/git-core/git-submodule: line 979: 8119 Segmentation fault (core dumped) git ${wt_prefix:+-C \"$wt_prefix\"} ${prefix:+--super-prefix \"$prefix\"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} \"$@\"\n> > \n> \n> You may have minimized too much. With the patch below I get:\n> \n> \tfatal: no ref store in submodule 'gnulib'\n> \n> I guess you'll get a different one in your original repo.\n> \n> The patch seems like a good idea in any case, though.\n> \n> -- >8 --\n> Subject: [PATCH] submodule: check for NULL return of get_submodule_ref_store()\n> \n> refs_head_ref() requires a valid ref_store pointer to be given as its\n> first argument. get_submodule_ref_store() can return NULL. Exit and\n> report the failure to find a ref store in that case instead of\n> segfaulting.\n> \n> Reported-by: Jeremy Feusi \n> Signed-off-by: Rene Scharfe \n> ---\n> builtin/submodule--helper.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ee020d4749..0f74e81005 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -654,9 +654,11 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n> \t\t\t displaypath);\n> \t} else if (!(flags & OPT_CACHED)) {\n> \t\tstruct object_id oid;\n> +\t\tstruct ref_store *refs = get_submodule_ref_store(path);\n> \n> -\t\tif (refs_head_ref(get_submodule_ref_store(path),\n> -\t\t\t\t handle_submodule_head_ref, &oid))\n> +\t\tif (!refs)\n> +\t\t\tdie(_(\"no ref store in submodule '%s'\"), path);\n> +\t\tif (refs_head_ref(refs, handle_submodule_head_ref, &oid))\n> \t\t\tdie(_(\"could not resolve HEAD ref inside the \"\n> \t\t\t \"submodule '%s'\"), path);\n> \n> -- \n> 2.16.3\n\n"},{"id":"342881","messageId":"9c3c0161-f894-3368-ece2-500d0bb6f475@web.de","threadId":"48137","inReplyTo":"20180325095046.GA687@feusi.co","subject":"Re: Null pointer dereference in git-submodule","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-03-25T10:57:28Z","receivedAt":"2018-03-25T10:57:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:\n> \n> Hmm... That's weird. I can reproduce it on 3 independant systems with\n> versions 2.16.2 up, although it does not work with version 2.11.0.\n> Anyway, I figured out how to reproduce this bug. It is caused when a\n> submodule is added and then the directory it resides in is moved or\n> deleted without commiting. For example:\n> \n> git init\n> git submodule add https://github.com/git/git git\n> mv git git.BAK\n> git submodule status #this command segfaults\n\nWith the patch I sent in my first reply the last command reports:\n\n\tfatal: no ref store in submodule 'git'\n\nThat may not be the most helpful message -- not just the ref store is\nmissing, the whole submodule is gone!\n\nCome to think about it, this removal may be intended. How about\nshowing the submodule as not being initialized at that point?\n\n-- >8 --\nSubject: [PATCH v2] submodule: check for NULL return of get_submodule_ref_store()\n\nIf we can't find a ref store for a submodule then assume it the latter\nis not initialized (or was removed). Print a status line accordingly\ninstead of causing a segmentation fault by passing NULL as the first\nparameter of refs_head_ref().\n\nReported-by: Jeremy Feusi \nSigned-off-by: Rene Scharfe \n---\nTest missing..\n\n builtin/submodule--helper.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ee020d4749..ae3014ac5a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t displaypath);\n \t} else if (!(flags & OPT_CACHED)) {\n \t\tstruct object_id oid;\n+\t\tstruct ref_store *refs = get_submodule_ref_store(path);\n \n-\t\tif (refs_head_ref(get_submodule_ref_store(path),\n-\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\tif (!refs) {\n+\t\t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (refs_head_ref(refs, handle_submodule_head_ref, &oid))\n \t\t\tdie(_(\"could not resolve HEAD ref inside the \"\n \t\t\t \"submodule '%s'\"), path);\n \n-- \n2.17.0.rc1.38.g7c51fd80b8\n"},{"id":"343190","messageId":"CAGZ79kYagdvpOcZykF4JPQc9vpVb8_xyFiQkE9yznBQTD1PWJw@mail.gmail.com","threadId":"48137","inReplyTo":"9c3c0161-f894-3368-ece2-500d0bb6f475@web.de","subject":"Re: Null pointer dereference in git-submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-27T23:50:44Z","receivedAt":"2018-03-27T23:51:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Mar 25, 2018 at 3:58 AM René Scharfe wrote:\n\n> Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:\n> >\n> > Hmm... That's weird. I can reproduce it on 3 independant systems with\n> > versions 2.16.2 up, although it does not work with version 2.11.0.\n> > Anyway, I figured out how to reproduce this bug. It is caused when a\n> > submodule is added and then the directory it resides in is moved or\n> > deleted without commiting. For example:\n> >\n> > git init\n> > git submodule add https://github.com/git/git git\n> > mv git git.BAK\n> > git submodule status #this command segfaults\n\n> With the patch I sent in my first reply the last command reports:\n\n> fatal: no ref store in submodule 'git'\n\n> That may not be the most helpful message -- not just the ref store is\n> missing, the whole submodule is gone!\n\n> Come to think about it, this removal may be intended. How about\n> showing the submodule as not being initialized at that point?\n\nAt first I thought we could still retrieve the ref store via a lookup of\npath -> name in .gitmodules and then navigate to\n.git/modules/ (as seen from the superproject)\nand load the ref store. But loading the refstore is a mere detail.\n\n\"not initialized\" is technically correct, the existing git directory\ninside the superproject doesn't matter.\n\n\n> -- >8 --\n> Subject: [PATCH v2] submodule: check for NULL return of\nget_submodule_ref_store()\n\nMaybe more imperative, telling what we actually want\nto achieve instead of what we do?\n\n submodule: report deleted submodules as not initialized\n\n> If we can't find a ref store for a submodule then assume it the latter\n> is not initialized (or was removed). Print a status line accordingly\n> instead of causing a segmentation fault by passing NULL as the first\n> parameter of refs_head_ref().\n\nThanks for the message here. Looks good!\n\n> Reported-by: Jeremy Feusi \n\nPlease also sign off instead of just claiming to report it.\n(The sign off has legal implications, see\nhttps://developercertificate.org/ or our copy in\nDocumentation/SubmittingPatches)\n\n> Signed-off-by: Rene Scharfe \n> ---\n> Test missing..\n\nWhich would be added in t/t7400-submodule-basic.sh\n\nThanks for coming up with a sensible patch!\nStefan\n\n\n> builtin/submodule--helper.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ee020d4749..ae3014ac5a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -654,9 +654,13 @@ static void status_submodule(const char *path, const\nstruct object_id *ce_oid,\n> displaypath);\n> } else if (!(flags & OPT_CACHED)) {\n> struct object_id oid;\n> + struct ref_store *refs = get_submodule_ref_store(path);\n\n> - if (refs_head_ref(get_submodule_ref_store(path),\n> - handle_submodule_head_ref, &oid))\n> + if (!refs) {\n> + print_status(flags, '-', path, ce_oid,\ndisplaypath);\n> + goto cleanup;\n> + }\n> + if (refs_head_ref(refs, handle_submodule_head_ref, &oid))\n> die(_(\"could not resolve HEAD ref inside the \"\n> \"submodule '%s'\"), path);\n\n> --\n> 2.17.0.rc1.38.g7c51fd80b8\n"},{"id":"343225","messageId":"xmqqlgec87ns.fsf@gitster-ct.c.googlers.com","threadId":"48137","inReplyTo":"CAGZ79kYagdvpOcZykF4JPQc9vpVb8_xyFiQkE9yznBQTD1PWJw@mail.gmail.com","subject":"Re: Null pointer dereference in git-submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-28T16:52:07Z","receivedAt":"2018-03-28T16:52:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Subject: [PATCH v2] submodule: check for NULL return of\n> get_submodule_ref_store()\n>\n> Maybe more imperative, telling what we actually want\n> to achieve instead of what we do?\n>\n> submodule: report deleted submodules as not initialized\n>\n>> If we can't find a ref store for a submodule then assume it the latter\n>> is not initialized (or was removed). Print a status line accordingly\n>> instead of causing a segmentation fault by passing NULL as the first\n>> parameter of refs_head_ref().\n>\n> Thanks for the message here. Looks good!\n> ...\n> Which would be added in t/t7400-submodule-basic.sh\n>\n> Thanks for coming up with a sensible patch!\n\nI take the above to mean that you as a contributor active in this\narea like the general idea in the patch but not volunteering to take\nthis topic over and instead trust René to tie the loose ends with a\nreroll, taking hints from your suggestions?\n\nI just wanted to make sure that we won't be confused whose turn it\nis next (e.g. me waiting for update to t7400 from you or René doing\nthe same).\n\nThanks.\n\n\n\n"},{"id":"343227","messageId":"CAGZ79kYveU=TK-zvxNimSYcQqhOf4OFgynuNDu08bvbDtwfEEA@mail.gmail.com","threadId":"48137","inReplyTo":"xmqqlgec87ns.fsf@gitster-ct.c.googlers.com","subject":"Re: Null pointer dereference in git-submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T17:03:09Z","receivedAt":"2018-03-28T17:03:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 28, 2018 at 9:52 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> Subject: [PATCH v2] submodule: check for NULL return of\n>> get_submodule_ref_store()\n>>\n>> Maybe more imperative, telling what we actually want\n>> to achieve instead of what we do?\n>>\n>> submodule: report deleted submodules as not initialized\n>>\n>>> If we can't find a ref store for a submodule then assume it the latter\n>>> is not initialized (or was removed). Print a status line accordingly\n>>> instead of causing a segmentation fault by passing NULL as the first\n>>> parameter of refs_head_ref().\n>>\n>> Thanks for the message here. Looks good!\n>> ...\n>> Which would be added in t/t7400-submodule-basic.sh\n>>\n>> Thanks for coming up with a sensible patch!\n>\n> I take the above to mean that you as a contributor active in this\n> area like the general idea in the patch but not volunteering to take\n> this topic over\n\nRereading the discussion, I overlooked the author of the second patch\nto be Rene (for some reason I thought someone else would have\nwritten that patch. Sorry about that!)\n\n> and instead trust René to tie the loose ends with a\n> reroll, taking hints from your suggestions?\n\nAs Rene likes. I can reroll that patch with a test, too.\nI'd pick it up after rerolling the series from yesterday\n(moving nested submodules).\n\n> I just wanted to make sure that we won't be confused whose turn it\n> is next (e.g. me waiting for update to t7400 from you or René doing\n> the same).\n\nThanks,\nStefan\n"},{"id":"343261","messageId":"20180328183832.243036-1-sbeller@google.com","threadId":"48137","inReplyTo":"9c3c0161-f894-3368-ece2-500d0bb6f475@web.de","subject":"[PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T18:38:32Z","receivedAt":"2018-03-28T18:38:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: René Scharfe \n\nIf we can't find a ref store for a submodule then assume it the latter\nis not initialized (or was removed). Print a status line accordingly\ninstead of causing a segmentation fault by passing NULL as the first\nparameter of refs_head_ref().\n\nReported-by: Jeremy Feusi \nSigned-off-by: Rene Scharfe \nSigned-off-by: Stefan Beller \n---\n\nI added a test for you.\n\nThanks,\nStefan\n\n builtin/submodule--helper.c | 8 ++++++--\n t/t7400-submodule-basic.sh | 12 ++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ee020d4749..ae3014ac5a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t displaypath);\n \t} else if (!(flags & OPT_CACHED)) {\n \t\tstruct object_id oid;\n+\t\tstruct ref_store *refs = get_submodule_ref_store(path);\n \n-\t\tif (refs_head_ref(get_submodule_ref_store(path),\n-\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\tif (!refs) {\n+\t\t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (refs_head_ref(refs, handle_submodule_head_ref, &oid))\n \t\t\tdie(_(\"could not resolve HEAD ref inside the \"\n \t\t\t \"submodule '%s'\"), path);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a39e69a3eb..d8aee51603 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '\n \t)\n '\n \n+test_expect_success 'moving the submodule does not break the superproject' '\n+\t(\n+\t\tcd addtest2 &&\n+\n+\t\tmv repo repo.bak &&\n+\t\tgit submodule status >actual &&\n+\t\tgrep -e \"^-\" -e repo actual &&\n+\n+\t\tmv repo.bak repo\n+\t)\n+'\n+\n test_expect_success 'submodule add --name allows to replace a submodule with another at the same path' '\n \t(\n \t\tcd addtest2 &&\n-- \n2.17.0.rc1.321.gba9d0f2565-goog\n\n"},{"id":"343264","messageId":"CAPig+cT1Yp8J8E0VDKdxvPdVD-UBsyDfQS+a83zOAW11ayfhnA@mail.gmail.com","threadId":"48137","inReplyTo":"20180328183832.243036-1-sbeller@google.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-28T18:57:42Z","receivedAt":"2018-03-28T18:57:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller wrote:\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '\n> +test_expect_success 'moving the submodule does not break the superproject' '\n> + (\n> + cd addtest2 &&\n> +\n> + mv repo repo.bak &&\n> + git submodule status >actual &&\n> + grep -e \"^-\" -e repo actual &&\n> +\n> + mv repo.bak repo\n\nShould this \"move back\" be encapsulated in a test_when_finished?\n\n> + )\n> +'\n"},{"id":"343269","messageId":"CAGZ79kYD+RC_Z7AH_uH9tZSDHhkikML4HkxUNGxkW8voHJ1i3A@mail.gmail.com","threadId":"48137","inReplyTo":"CAPig+cT1Yp8J8E0VDKdxvPdVD-UBsyDfQS+a83zOAW11ayfhnA@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T20:08:57Z","receivedAt":"2018-03-28T20:09:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine wrote:\n> On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller wrote:\n>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n>> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '\n>> +test_expect_success 'moving the submodule does not break the superproject' '\n>> + (\n>> + cd addtest2 &&\n>> +\n>> + mv repo repo.bak &&\n>> + git submodule status >actual &&\n>> + grep -e \"^-\" -e repo actual &&\n>> +\n>> + mv repo.bak repo\n>\n> Should this \"move back\" be encapsulated in a test_when_finished?\n\nI thought about that, but decided against it for some reason as I was debating\nwhere to put the test_when_finished. I mostly saw those at the very beginning\nof a test and wondered if it can be called from within a subshell.\n(I'd not want to put it at the beginning but rather adjacent to the move.)\n"},{"id":"343270","messageId":"CAPig+cQ_j4OyBjsZHE8ZPBojqD7HhSEb14-CFY9qYfXX+dafpQ@mail.gmail.com","threadId":"48137","inReplyTo":"CAGZ79kYD+RC_Z7AH_uH9tZSDHhkikML4HkxUNGxkW8voHJ1i3A@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-28T20:21:36Z","receivedAt":"2018-03-28T20:21:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller wrote:\n> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine wrote:\n>>> +test_expect_success 'moving the submodule does not break the superproject' '\n>>> + (\n>>> + cd addtest2 &&\n>>> +\n>>> + mv repo repo.bak &&\n>>> + git submodule status >actual &&\n>>> + grep -e \"^-\" -e repo actual &&\n>>> +\n>>> + mv repo.bak repo\n>>\n>> Should this \"move back\" be encapsulated in a test_when_finished?\n>\n> I thought about that, but decided against it for some reason as I was debating\n> where to put the test_when_finished. I mostly saw those at the very beginning\n> of a test and wondered if it can be called from within a subshell.\n> (I'd not want to put it at the beginning but rather adjacent to the move.)\n\nIt looks like test_when_finished() shouldn't be used in a subshell.\nHowever, wouldn't the following be reasonable?\n\n mv addtest2/repo addtest2/repo.bak &&\n test_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n (\n cd addtest2 &&\n git submodule status >actual &&\n grep -e \"^-\" -e repo actual\n )\n\nOr, even simpler:\n\n mv addtest2/repo addtest2/repo.bak &&\n test_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n git -C addtest2 submodule status >actual &&\n grep -e \"^-\" -e repo actual\n"},{"id":"343275","messageId":"CAGZ79kYs1YFQav8ux0nHcdDoaXcrBrnLM7K5=+9+4sYjHR4yJA@mail.gmail.com","threadId":"48137","inReplyTo":"CAPig+cQ_j4OyBjsZHE8ZPBojqD7HhSEb14-CFY9qYfXX+dafpQ@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T20:54:41Z","receivedAt":"2018-03-28T20:54:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 28, 2018 at 1:21 PM, Eric Sunshine wrote:\n\n> Or, even simpler:\n>\n> mv addtest2/repo addtest2/repo.bak &&\n> test_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n> git -C addtest2 submodule status >actual &&\n> grep -e \"^-\" -e repo actual\n\nI like this!\nTime permitting I'll update the patch (I'll first have to take\ncare of some other series, so feel free to make it into a patch)\n\nThanks,\nStefan\n"},{"id":"343276","messageId":"38570708-e166-0004-878a-2d8442c12b65@web.de","threadId":"48137","inReplyTo":"CAPig+cQ_j4OyBjsZHE8ZPBojqD7HhSEb14-CFY9qYfXX+dafpQ@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-03-28T21:14:08Z","receivedAt":"2018-03-28T21:14:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.03.2018 um 22:21 schrieb Eric Sunshine:\n> On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller wrote:\n>> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine wrote:\n>>>> +test_expect_success 'moving the submodule does not break the superproject' '\n>>>> + (\n>>>> + cd addtest2 &&\n>>>> +\n>>>> + mv repo repo.bak &&\n>>>> + git submodule status >actual &&\n>>>> + grep -e \"^-\" -e repo actual &&\n>>>> +\n>>>> + mv repo.bak repo\n>>>\n>>> Should this \"move back\" be encapsulated in a test_when_finished?\n>>\n>> I thought about that, but decided against it for some reason as I was debating\n>> where to put the test_when_finished. I mostly saw those at the very beginning\n>> of a test and wondered if it can be called from within a subshell.\n>> (I'd not want to put it at the beginning but rather adjacent to the move.)\n> \n> It looks like test_when_finished() shouldn't be used in a subshell.\n> However, wouldn't the following be reasonable?\n> \n> mv addtest2/repo addtest2/repo.bak &&\n> test_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n> (\n> cd addtest2 &&\n> git submodule status >actual &&\n> grep -e \"^-\" -e repo actual\n> )\n\nI like this version, except for the grep call -- it accepts either lines\nstarting with a dash or containing \"repo\". So if status would report\njust \"-\" and nothing else then the test would pass. \n\n> Or, even simpler:\n> \n> mv addtest2/repo addtest2/repo.bak &&\n> test_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n> git -C addtest2 submodule status >actual &&\n> grep -e \"^-\" -e repo actual\n> \n\nThis looks nicer here in the script, but doesn't test exactly what users\ntype most of the time, I suppose.\n\nSo how about this?\n\n-- >8 --\nSubject: [PATCH v3] submodule: check for NULL return of get_submodule_ref_store()\n\nIf we can't find a ref store for a submodule then assume the latter\nis not initialized (or was removed). Print a status line accordingly\ninstead of causing a segmentation fault by passing NULL as the first\nparameter of refs_head_ref().\n\nReported-by: Jeremy Feusi \nReviewed-by: Stefan Beller \nInitial-Test-By: Stefan Beller \nHelped-by: Eric Sunshine \nSigned-off-by: Rene Scharfe \n---\n builtin/submodule--helper.c | 8 ++++++--\n t/t7400-submodule-basic.sh | 15 +++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6ba8587b6d..9a0fb5e784 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t displaypath);\n \t} else if (!(flags & OPT_CACHED)) {\n \t\tstruct object_id oid;\n+\t\tstruct ref_store *refs = get_submodule_ref_store(path);\n \n-\t\tif (refs_head_ref(get_submodule_ref_store(path),\n-\t\t\t\t handle_submodule_head_ref, &oid))\n+\t\tif (!refs) {\n+\t\t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tif (refs_head_ref(refs, handle_submodule_head_ref, &oid))\n \t\t\tdie(_(\"could not resolve HEAD ref inside the \"\n \t\t\t \"submodule '%s'\"), path);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a39e69a3eb..ef1ea8d6b0 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -821,6 +821,21 @@ test_expect_success 'moving the superproject does not break submodules' '\n \t)\n '\n \n+test_expect_success 'moving the submodule does not break the superproject' '\n+\t(\n+\t\tcd addtest2 &&\n+\t\tgit submodule status\n+\t) >actual &&\n+\tsed -e \"s/^ \\([^ ]* repo\\) .*/-\\1/\" expect &&\n+\tmv addtest2/repo addtest2/repo.bak &&\n+\ttest_when_finished \"mv addtest2/repo.bak addtest2/repo\" &&\n+\t(\n+\t\tcd addtest2 &&\n+\t\tgit submodule status\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'submodule add --name allows to replace a submodule with another at the same path' '\n \t(\n \t\tcd addtest2 &&\n-- \n2.16.3\n"},{"id":"343278","messageId":"CAGZ79kYL55+=9UbW7H-c7OUg6CHbnzdkxs0ZMsFnEAaoVXRwLw@mail.gmail.com","threadId":"48137","inReplyTo":"38570708-e166-0004-878a-2d8442c12b65@web.de","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-03-28T21:37:31Z","receivedAt":"2018-03-28T21:37:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> This looks nicer here in the script, but doesn't test exactly what users\n> type most of the time, I suppose.\n>\n> So how about this?\n\nLooks good to me, though I had a nagging feeling at first that the\nregex could be made more concise.\nWhy do we need the optional \"[^ ]\" inside \\1 ?\n\n> + sed -e \"s/^ \\([^ ]* repo\\) .*/-\\1/\" expect &&\n"},{"id":"343288","messageId":"096248bf-0a33-7360-e82b-b7a36e362604@web.de","threadId":"48137","inReplyTo":"CAGZ79kYL55+=9UbW7H-c7OUg6CHbnzdkxs0ZMsFnEAaoVXRwLw@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-03-28T22:24:49Z","receivedAt":"2018-03-28T22:25:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.03.2018 um 23:37 schrieb Stefan Beller:\n>> This looks nicer here in the script, but doesn't test exactly what users\n>> type most of the time, I suppose.\n>>\n>> So how about this?\n> \n> Looks good to me, though I had a nagging feeling at first that the\n> regex could be made more concise.\n> Why do we need the optional \"[^ ]\" inside \\1 ?\n> \n>> + sed -e \"s/^ \\([^ ]* repo\\) .*/-\\1/\" expect &&\n\n[:xdigit:] would match the hash more precisely, but I don't know how\nwidely this character class is supported among sed implementations.\nAnd being so strict may require changes once SHA1 is replaced -- new\nhashes might be marked with a special character. And it's longer\nanyway.\n\n\\S could be used instead, but I also don't know how widely this is\nsupported.\n\n. could be used as well, of course, but that feels a bit sloppy to\nme.\n\nUsing separate s commands for beginning and end of the line would\nbe shorter. This here is slightly longer, anyway:\n\n\t/ repo / {s/^ /-/; s/ (.*//}\n\nWhat do you have in mind? How to transform this:\n\n b2613938d15a18d9d4a504cacd9654fd1c879197 repo (heads/master)\n\n... into this:\n\n-b2613938d15a18d9d4a504cacd9654fd1c879197 repo\n\n... while keeping other lines unchanged?\n\nRené\n"},{"id":"343295","messageId":"xmqqd0zn7rqx.fsf@gitster-ct.c.googlers.com","threadId":"48137","inReplyTo":"CAGZ79kYL55+=9UbW7H-c7OUg6CHbnzdkxs0ZMsFnEAaoVXRwLw@mail.gmail.com","subject":"Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-28T22:35:50Z","receivedAt":"2018-03-28T22:35:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> This looks nicer here in the script, but doesn't test exactly what users\n>> type most of the time, I suppose.\n>>\n>> So how about this?\n>\n> Looks good to me, though I had a nagging feeling at first that the\n> regex could be made more concise.\n> Why do we need the optional \"[^ ]\" inside \\1 ?\n>\n>> + sed -e \"s/^ \\([^ ]* repo\\) .*/-\\1/\" expect &&\n\nAt that position there's 40-hex object name. If we want to go\nlooser, you could say\n\n\t\"s/^ \\(.* repo\\) .*/-\\1/\"\n\nand if you want to go more strict, you could say\n\n\t\"s/^ \\($_x40 repo\\) (heads\\/master)$/-\\1/\"\n\nI think \"Here between the leading SP and SP before the pathname\n'repo', we expect an object name which should be a run of non SP\nbytes\" is a reasonable mid-point that is stricter than \"anything\ngoes\" and is still concise.\n\n"}]}