Re: Null pointer dereference in git-submodule

Re: Null pointer dereference in git-submodule

[Jeremy Feusi]

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:
> 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

Re: Null pointer dereference in git-submodule

[René Scharfe]

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?

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

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Sun, Mar 25, 2018 at 3:58 AM René Scharfe <l.s.r@web.de> wrote:

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

> 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

Re: Null pointer dereference in git-submodule

[Junio C Hamano]

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

Re: Null pointer dereference in git-submodule

[Stefan Beller]

On Wed, Mar 28, 2018 at 9:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Stefan Beller]

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

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

[Eric Sunshine]

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?

> +       )
> +'

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

[Stefan Beller]

On Wed, Mar 28, 2018 at 11:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.)

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

[Eric Sunshine]

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
    )

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

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

[Stefan Beller]

On Wed, Mar 28, 2018 at 1:21 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

> 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

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

[René Scharfe]

Am 28.03.2018 um 22:21 schrieb Eric Sunshine:
> 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. 

> 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

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

[Stefan Beller]

> 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 &&

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

[René Scharfe]

Am 28.03.2018 um 23:37 schrieb Stefan Beller:
>> 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é

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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