threads / discuss / 48137

Re: Null pointer dereference in git-submodule

Subject: Re: Null pointer dereference in git-submodule

## tl;dr

14 messages between Mar 25, 2018 and Mar 28, 2018.

replies: 13people: 5as markdown or json

Jeremy Feusi· Mar 25, 2018, 09:50 UTC · lore
Reply-To:

Hmm... That's weird. I can reproduce it on 3 independant systems with versions 2.16.2 up, although it does not work with version 2.11.0. Anyway, I figured out how to reproduce this bug. It is caused when a submodule is added and then the directory it resides in is moved or deleted without commiting. For example:

git init git submodule add https://github.com/git/git git mv git git.BAK git submodule status #this command segfaults

cheers Jeremy

In-Reply-To: <a7ad9dbf-1b0f-efc6-3a17-51cf25381ce5@web.de>
On Sat, Mar 24, 2018 at 09:42:57PM +0100, René Scharfe wrote:
Show 85 quoted lines
> Am 24.03.2018 um 18:42 schrieb Jeremy Feusi:
> > Hi,
> > While bootstrapping a gnu repository I noticed that git segfaulted when
> > called as "git submodule status". After compiling git with address
> > sanitizer and minimizing the directory I finally narrowed it down to the
> > files which I have attached as a tar archive. Here is a detailed backtrace:
> > 
> > AddressSanitizer:DEADLYSIGNAL
> > =================================================================
> > ==63400==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000c27a93 bp 0x7ffdcb4eec10 sp 0x7ffdcb4eeb80 T0)
> > ==63400==The signal is caused by a READ memory access.
> > ==63400==Hint: address points to the zero page.
> >      #0 0xc27a92 in refs_read_raw_ref /home/jfe/git/refs.c:1451:20
> >      #1 0xc174a6 in refs_resolve_ref_unsafe /home/jfe/git/refs.c:1493:7
> >      #2 0xc1826a in refs_read_ref_full /home/jfe/git/refs.c:224:6
> >      #3 0xc26d53 in refs_head_ref /home/jfe/git/refs.c:1314:7
> >      #4 0x8071e6 in status_submodule /home/jfe/git/builtin/submodule--helper.c:658:7
> >      #5 0x806a89 in status_submodule_cb /home/jfe/git/builtin/submodule--helper.c:699:2
> >      #6 0x80523e in for_each_listed_submodule /home/jfe/git/builtin/submodule--helper.c:438:3
> >      #7 0x7f7e9a in module_status /home/jfe/git/builtin/submodule--helper.c:732:2
> >      #8 0x7efd69 in cmd_submodule__helper /home/jfe/git/builtin/submodule--helper.c:1859:11
> >      #9 0x51e024 in run_builtin /home/jfe/git/git.c:346:11
> >      #10 0x5192c2 in handle_builtin /home/jfe/git/git.c:554:8
> >      #11 0x51d0f0 in run_argv /home/jfe/git/git.c:606:4
> >      #12 0x518600 in cmd_main /home/jfe/git/git.c:683:19
> >      #13 0x8501d6 in main /home/jfe/git/common-main.c:43:9
> >      #14 0x7f49fdaf2f29 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20f29)
> >      #15 0x41f4b9 in _start (/home/jfe/git/inst/libexec/git-core/git+0x41f4b9)
> > 
> > AddressSanitizer can not provide additional info.
> > SUMMARY: AddressSanitizer: SEGV /home/jfe/git/refs.c:1451:20 in refs_read_raw_ref
> > ==63400==ABORTING
> > 
> > As mentioned above, this bug is triggered by issuing the command
> > "git submodule status" while in the attached directory.
> > 
> > This bug was confirmed on Debian with version 2.16.1 and
> > 2.17.0.rc1.35.g90bbd502d as well as on Arch Linux with version 2.16.2
> > where further output is given by git:
> > 
> > /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} "$@"
> > 
> 
> You may have minimized too much.  With the patch below I get:
> 
> 	fatal: no ref store in submodule 'gnulib'
> 
> I guess you'll get a different one in your original repo.
> 
> The patch seems like a good idea in any case, though.
> 
> -- >8 --
> Subject: [PATCH] submodule: check for NULL return of get_submodule_ref_store()
> 
> refs_head_ref() requires a valid ref_store pointer to be given as its
> first argument.  get_submodule_ref_store() can return NULL.  Exit and
> report the failure to find a ref store in that case instead of
> segfaulting.
> 
> Reported-by: Jeremy Feusi <jeremy@feusi.co>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/submodule--helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..0f74e81005 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -654,9 +654,11 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>  			     displaypath);
>  	} else if (!(flags & OPT_CACHED)) {
>  		struct object_id oid;
> +		struct ref_store *refs = get_submodule_ref_store(path);
>  
> -		if (refs_head_ref(get_submodule_ref_store(path),
> -				  handle_submodule_head_ref, &oid))
> +		if (!refs)
> +			die(_("no ref store in submodule '%s'"), path);
> +		if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
>  			die(_("could not resolve HEAD ref inside the "
>  			      "submodule '%s'"), path);
>  
> -- 
> 2.16.3
René Scharfe· Mar 25, 2018, 10:57 UTC · re: Jeremy Feusi · lore
Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:
Show 11 quoted lines
> 
> Hmm... That's weird. I can reproduce it on 3 independant systems with
> versions 2.16.2 up, although it does not work with version 2.11.0.
> Anyway, I figured out how to reproduce this bug. It is caused when a
> submodule is added and then the directory it resides in is moved or
> deleted without commiting. For example:
> 
> git init
> git submodule add https://github.com/git/git git
> mv git git.BAK
> git submodule status #this command segfaults
With the patch I sent in my first reply the last command reports:
	fatal: no ref store in submodule 'git'

That may not be the most helpful message -- not just the ref store is missing, the whole submodule is gone!

Come to think about it, this removal may be intended. How about showing the submodule as not being initialized at that point?

-- >8 --
Subject: [PATCH v2] submodule: check for NULL return of get_submodule_ref_store()

If we can't find a ref store for a submodule then assume it the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Reported-by: Jeremy Feusi <jeremy@feusi.co>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Test missing..
 builtin/submodule--helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..ae3014ac5a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
 		struct object_id oid;
+		struct ref_store *refs = get_submodule_ref_store(path);
 
-		if (refs_head_ref(get_submodule_ref_store(path),
-				  handle_submodule_head_ref, &oid))
+		if (!refs) {
+			print_status(flags, '-', path, ce_oid, displaypath);
+			goto cleanup;
+		}
+		if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
 			die(_("could not resolve HEAD ref inside the "
 			      "submodule '%s'"), path);
 

-- 
2.17.0.rc1.38.g7c51fd80b8
Stefan Beller· Mar 27, 2018, 23:50 UTC · re: René Scharfe · lore
On Sun, Mar 25, 2018 at 3:58 AM René Scharfe <l.s.r@web.de> wrote:
Show 12 quoted lines
> Am 25.03.2018 um 11:50 schrieb Jeremy Feusi:
> >
> > Hmm... That's weird. I can reproduce it on 3 independant systems with
> > versions 2.16.2 up, although it does not work with version 2.11.0.
> > Anyway, I figured out how to reproduce this bug. It is caused when a
> > submodule is added and then the directory it resides in is moved or
> > deleted without commiting. For example:
> >
> > git init
> > git submodule add https://github.com/git/git git
> > mv git git.BAK
> > git submodule status #this command segfaults
> With the patch I sent in my first reply the last command reports:
>          fatal: no ref store in submodule 'git'
> That may not be the most helpful message -- not just the ref store is
> missing, the whole submodule is gone!
> Come to think about it, this removal may be intended.  How about
> showing the submodule as not being initialized at that point?

At first I thought we could still retrieve the ref store via a lookup of path -> name in .gitmodules and then navigate to .git/modules<name>/ (as seen from the superproject) and load the ref store. But loading the refstore is a mere detail.

"not initialized" is technically correct, the existing git directory inside the superproject doesn't matter.

> -- >8 --
> Subject: [PATCH v2] submodule: check for NULL return of
get_submodule_ref_store()

Maybe more imperative, telling what we actually want to achieve instead of what we do?

   submodule: report deleted submodules as not initialized
> If we can't find a ref store for a submodule then assume it the latter
> is not initialized (or was removed).  Print a status line accordingly
> instead of causing a segmentation fault by passing NULL as the first
> parameter of refs_head_ref().
Thanks for the message here. Looks good!
> Reported-by: Jeremy Feusi <jeremy@feusi.co>

Please also sign off instead of just claiming to report it. (The sign off has legal implications, see https://developercertificate.org/ or our copy in Documentation/SubmittingPatches)

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Test missing..
Which would be added in t/t7400-submodule-basic.sh

Thanks for coming up with a sensible patch! Stefan

>   builtin/submodule--helper.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
Show 5 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index ee020d4749..ae3014ac5a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -654,9 +654,13 @@ static void status_submodule(const char *path, const
struct object_id *ce_oid,
>                               displaypath);
>          } else if (!(flags & OPT_CACHED)) {
>                  struct object_id oid;
> +               struct ref_store *refs = get_submodule_ref_store(path);
> -               if (refs_head_ref(get_submodule_ref_store(path),
> -                                 handle_submodule_head_ref, &oid))
> +               if (!refs) {
> +                       print_status(flags, '-', path, ce_oid,
displaypath);
Show 5 quoted lines
> +                       goto cleanup;
> +               }
> +               if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
>                          die(_("could not resolve HEAD ref inside the "
>                                "submodule '%s'"), path);
> --
> 2.17.0.rc1.38.g7c51fd80b8
Junio C Hamano· Mar 28, 2018, 16:52 UTC · re: Stefan Beller · lore
Stefan Beller <sbeller@google.com> writes:
Show 18 quoted lines
>> Subject: [PATCH v2] submodule: check for NULL return of
> get_submodule_ref_store()
>
> Maybe more imperative, telling what we actually want
> to achieve instead of what we do?
>
>    submodule: report deleted submodules as not initialized
>
>> If we can't find a ref store for a submodule then assume it the latter
>> is not initialized (or was removed).  Print a status line accordingly
>> instead of causing a segmentation fault by passing NULL as the first
>> parameter of refs_head_ref().
>
> Thanks for the message here. Looks good!
> ...
> Which would be added in t/t7400-submodule-basic.sh
>
> Thanks for coming up with a sensible patch!

I take the above to mean that you as a contributor active in this area like the general idea in the patch but not volunteering to take this topic over and instead trust René to tie the loose ends with a reroll, taking hints from your suggestions?

I just wanted to make sure that we won't be confused whose turn it is next (e.g. me waiting for update to t7400 from you or René doing the same).

Thanks.
Stefan Beller· Mar 28, 2018, 17:03 UTC · re: Junio C Hamano · lore
On Wed, Mar 28, 2018 at 9:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>>> Subject: [PATCH v2] submodule: check for NULL return of
>> get_submodule_ref_store()
>>
>> Maybe more imperative, telling what we actually want
>> to achieve instead of what we do?
>>
>>    submodule: report deleted submodules as not initialized
>>
>>> If we can't find a ref store for a submodule then assume it the latter
>>> is not initialized (or was removed).  Print a status line accordingly
>>> instead of causing a segmentation fault by passing NULL as the first
>>> parameter of refs_head_ref().
>>
>> Thanks for the message here. Looks good!
>> ...
>> Which would be added in t/t7400-submodule-basic.sh
>>
>> Thanks for coming up with a sensible patch!
>
> I take the above to mean that you as a contributor active in this
> area like the general idea in the patch but not volunteering to take
> this topic over

Rereading the discussion, I overlooked the author of the second patch to be Rene (for some reason I thought someone else would have written that patch. Sorry about that!)

> and instead trust René to tie the loose ends with a
> reroll, taking hints from your suggestions?

As Rene likes. I can reroll that patch with a test, too. I'd pick it up after rerolling the series from yesterday (moving nested submodules).

> I just wanted to make sure that we won't be confused whose turn it
> is next (e.g. me waiting for update to t7400 from you or René doing
> the same).

Thanks, Stefan

Stefan Beller· Mar 28, 2018, 18:38 UTC · re: René Scharfe · lore

[PATCH] submodule: check for NULL return of get_submodule_ref_store()

From: René Scharfe <l.s.r@web.de>

If we can't find a ref store for a submodule then assume it the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Reported-by: Jeremy Feusi <jeremy@feusi.co>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I added a test for you.

Thanks, Stefan

 builtin/submodule--helper.c |  8 ++++++--
 t/t7400-submodule-basic.sh  | 12 ++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ee020d4749..ae3014ac5a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
 		struct object_id oid;
+		struct ref_store *refs = get_submodule_ref_store(path);
 
-		if (refs_head_ref(get_submodule_ref_store(path),
-				  handle_submodule_head_ref, &oid))
+		if (!refs) {
+			print_status(flags, '-', path, ce_oid, displaypath);
+			goto cleanup;
+		}
+		if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
 			die(_("could not resolve HEAD ref inside the "
 			      "submodule '%s'"), path);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a39e69a3eb..d8aee51603 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '
 	)
 '
 
+test_expect_success 'moving the submodule does not break the superproject' '
+	(
+		cd addtest2 &&
+
+		mv repo repo.bak &&
+		git submodule status >actual &&
+		grep -e "^-" -e repo actual &&
+
+		mv repo.bak repo
+	)
+'
+
 test_expect_success 'submodule add --name allows to replace a submodule with another at the same path' '
 	(
 		cd addtest2 &&

-- 
2.17.0.rc1.321.gba9d0f2565-goog
Eric Sunshine· Mar 28, 2018, 18:57 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller <sbeller@google.com> wrote:
Show 11 quoted lines
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '
> +test_expect_success 'moving the submodule does not break the superproject' '
> +       (
> +               cd addtest2 &&
> +
> +               mv repo repo.bak &&
> +               git submodule status >actual &&
> +               grep -e "^-" -e repo actual &&
> +
> +               mv repo.bak repo
Should this "move back" be encapsulated in a test_when_finished?
> +       )
> +'
Stefan Beller· Mar 28, 2018, 20:08 UTC · re: Eric Sunshine · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 14 quoted lines
> On Wed, Mar 28, 2018 at 2:38 PM, Stefan Beller <sbeller@google.com> wrote:
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> @@ -821,6 +821,18 @@ test_expect_success 'moving the superproject does not break submodules' '
>> +test_expect_success 'moving the submodule does not break the superproject' '
>> +       (
>> +               cd addtest2 &&
>> +
>> +               mv repo repo.bak &&
>> +               git submodule status >actual &&
>> +               grep -e "^-" -e repo actual &&
>> +
>> +               mv repo.bak repo
>
> Should this "move back" be encapsulated in a test_when_finished?

I thought about that, but decided against it for some reason as I was debating where to put the test_when_finished. I mostly saw those at the very beginning of a test and wondered if it can be called from within a subshell. (I'd not want to put it at the beginning but rather adjacent to the move.)

Eric Sunshine· Mar 28, 2018, 20:21 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller <sbeller@google.com> wrote:
Show 17 quoted lines
> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +test_expect_success 'moving the submodule does not break the superproject' '
>>> +       (
>>> +               cd addtest2 &&
>>> +
>>> +               mv repo repo.bak &&
>>> +               git submodule status >actual &&
>>> +               grep -e "^-" -e repo actual &&
>>> +
>>> +               mv repo.bak repo
>>
>> Should this "move back" be encapsulated in a test_when_finished?
>
> I thought about that, but decided against it for some reason as I was debating
> where to put the test_when_finished. I mostly saw those at the very beginning
> of a test and wondered if it can be called from within a subshell.
> (I'd not want to put it at the beginning but rather adjacent to the move.)

It looks like test_when_finished() shouldn't be used in a subshell. However, wouldn't the following be reasonable?

    mv addtest2/repo addtest2/repo.bak &&
    test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
    (
        cd addtest2 &&
        git submodule status >actual &&
        grep -e "^-" -e repo actual
    )
Or, even simpler:
    mv addtest2/repo addtest2/repo.bak &&
    test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
    git -C addtest2 submodule status >actual &&
    grep -e "^-" -e repo actual
Stefan Beller· Mar 28, 2018, 20:54 UTC · re: Eric Sunshine · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

On Wed, Mar 28, 2018 at 1:21 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 6 quoted lines
> Or, even simpler:
>
>     mv addtest2/repo addtest2/repo.bak &&
>     test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
>     git -C addtest2 submodule status >actual &&
>     grep -e "^-" -e repo actual

I like this! Time permitting I'll update the patch (I'll first have to take care of some other series, so feel free to make it into a patch)

Thanks, Stefan

René Scharfe· Mar 28, 2018, 21:14 UTC · re: Eric Sunshine · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

Am 28.03.2018 um 22:21 schrieb Eric Sunshine:
Show 29 quoted lines
> On Wed, Mar 28, 2018 at 4:08 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> +test_expect_success 'moving the submodule does not break the superproject' '
>>>> +       (
>>>> +               cd addtest2 &&
>>>> +
>>>> +               mv repo repo.bak &&
>>>> +               git submodule status >actual &&
>>>> +               grep -e "^-" -e repo actual &&
>>>> +
>>>> +               mv repo.bak repo
>>>
>>> Should this "move back" be encapsulated in a test_when_finished?
>>
>> I thought about that, but decided against it for some reason as I was debating
>> where to put the test_when_finished. I mostly saw those at the very beginning
>> of a test and wondered if it can be called from within a subshell.
>> (I'd not want to put it at the beginning but rather adjacent to the move.)
> 
> It looks like test_when_finished() shouldn't be used in a subshell.
> However, wouldn't the following be reasonable?
> 
>      mv addtest2/repo addtest2/repo.bak &&
>      test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
>      (
>          cd addtest2 &&
>          git submodule status >actual &&
>          grep -e "^-" -e repo actual
>      )

I like this version, except for the grep call -- it accepts either lines starting with a dash or containing "repo". So if status would report just "-" and nothing else then the test would pass.

Show 7 quoted lines
> Or, even simpler:
> 
>      mv addtest2/repo addtest2/repo.bak &&
>      test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
>      git -C addtest2 submodule status >actual &&
>      grep -e "^-" -e repo actual
>

This looks nicer here in the script, but doesn't test exactly what users type most of the time, I suppose.

So how about this?
-- >8 --
Subject: [PATCH v3] submodule: check for NULL return of get_submodule_ref_store()

If we can't find a ref store for a submodule then assume the latter is not initialized (or was removed). Print a status line accordingly instead of causing a segmentation fault by passing NULL as the first parameter of refs_head_ref().

Reported-by: Jeremy Feusi <jeremy@feusi.co>
Reviewed-by: Stefan Beller <sbeller@google.com>
Initial-Test-By: Stefan Beller <sbeller@google.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/submodule--helper.c |  8 ++++++--
 t/t7400-submodule-basic.sh  | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6ba8587b6d..9a0fb5e784 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -654,9 +654,13 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
 		struct object_id oid;
+		struct ref_store *refs = get_submodule_ref_store(path);
 
-		if (refs_head_ref(get_submodule_ref_store(path),
-				  handle_submodule_head_ref, &oid))
+		if (!refs) {
+			print_status(flags, '-', path, ce_oid, displaypath);
+			goto cleanup;
+		}
+		if (refs_head_ref(refs, handle_submodule_head_ref, &oid))
 			die(_("could not resolve HEAD ref inside the "
 			      "submodule '%s'"), path);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index a39e69a3eb..ef1ea8d6b0 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -821,6 +821,21 @@ test_expect_success 'moving the superproject does not break submodules' '
 	)
 '
 
+test_expect_success 'moving the submodule does not break the superproject' '
+	(
+		cd addtest2 &&
+		git submodule status
+	) >actual &&
+	sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&
+	mv addtest2/repo addtest2/repo.bak &&
+	test_when_finished "mv addtest2/repo.bak addtest2/repo" &&
+	(
+		cd addtest2 &&
+		git submodule status
+	) >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'submodule add --name allows to replace a submodule with another at the same path' '
 	(
 		cd addtest2 &&

-- 
2.16.3
Stefan Beller· Mar 28, 2018, 21:37 UTC · re: René Scharfe · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

> This looks nicer here in the script, but doesn't test exactly what users
> type most of the time, I suppose.
>
> So how about this?

Looks good to me, though I had a nagging feeling at first that the regex could be made more concise. Why do we need the optional "[^ ]" inside \1 ?

> +       sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&
René Scharfe· Mar 28, 2018, 22:24 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

Am 28.03.2018 um 23:37 schrieb Stefan Beller:
Show 10 quoted lines
>> This looks nicer here in the script, but doesn't test exactly what users
>> type most of the time, I suppose.
>>
>> So how about this?
> 
> Looks good to me, though I had a nagging feeling at first that the
> regex could be made more concise.
> Why do we need the optional "[^ ]" inside \1 ?
> 
>> +       sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&

[:xdigit:] would match the hash more precisely, but I don't know how widely this character class is supported among sed implementations. And being so strict may require changes once SHA1 is replaced -- new hashes might be marked with a special character. And it's longer anyway.

\S could be used instead, but I also don't know how widely this is supported.

. could be used as well, of course, but that feels a bit sloppy to me.

Using separate s commands for beginning and end of the line would be shorter. This here is slightly longer, anyway:

	/ repo / {s/^ /-/; s/ (.*//}
What do you have in mind?  How to transform this:
 b2613938d15a18d9d4a504cacd9654fd1c879197 repo (heads/master)
... into this:
-b2613938d15a18d9d4a504cacd9654fd1c879197 repo
... while keeping other lines unchanged?
René
Junio C Hamano· Mar 28, 2018, 22:35 UTC · re: Stefan Beller · lore

Re: [PATCH] submodule: check for NULL return of get_submodule_ref_store()

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
>> This looks nicer here in the script, but doesn't test exactly what users
>> type most of the time, I suppose.
>>
>> So how about this?
>
> Looks good to me, though I had a nagging feeling at first that the
> regex could be made more concise.
> Why do we need the optional "[^ ]" inside \1 ?
>
>> +       sed -e "s/^ \([^ ]* repo\) .*/-\1/" <actual >expect &&

At that position there's 40-hex object name. If we want to go looser, you could say

	"s/^ \(.* repo\) .*/-\1/"
and if you want to go more strict, you could say
	"s/^ \($_x40 repo\) (heads\/master)$/-\1/"

I think "Here between the leading SP and SP before the pathname 'repo', we expect an object name which should be a run of non SP bytes" is a reasonable mid-point that is stricter than "anything goes" and is still concise.

← back to recent threads