threads / discuss / 39064

assert failed in submodule edge case

Subject: assert failed in submodule edge case

## tl;dr

15 messages between Apr 13, 2015 and Jun 14, 2015.

replies: 14people: 4as markdown or json

Dennis Kaarsemaker· Apr 13, 2015, 16:55 UTC · lore

Reported by djanos_ in #git: git add segfaults when you manage to confuse it with a submodule in the index that is no longer a submodule.

Here's his script to reproduce the segfault:

mkdir segfault cd segfault

mkdir subrepo cd subrepo

git init . echo a > a git add a git commit -m "a"

cd ..

git init . git add .

cd subrepo rm -rf .git git add .

This last git add will now barf: git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.

These all work as I think they are intended to: $ git -C segfault add subrepo/a fatal: Pathspec 'subrepo/a' is in submodule 'subrepo' $ git -C segfault/subrepo add a fatal: Pathspec 'a' is in submodule 'subrepo'

And this does nothing, it also doesn't crash:
$ git -C segfault add subrepo
GDB backtrace below, this is with next as of e6f3401.

Starting program: /home/dennis/code/git/git -C segfault/subrepo add . [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff702c528 in __GI_abort () at abort.c:89
#2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len", file=file@entry=0x560826 "pathspec.c", line=line@entry=317, 
    function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:92
#3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len", 
    file=file@entry=0x560826 "pathspec.c", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:101
#4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 ".", prefixlen=8, prefix=0x7dd09f "subrepo/", flags=50, raw=0x7fffffffd648, 
    p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316
#5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f "subrepo/", 
    argv=argv@entry=0x7fffffffd648) at pathspec.c:417
#6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f "subrepo/") at builtin/add.c:370
#7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350
#8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532
#9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578
#10 main (argc=2, av=<optimized out>) at git.c:686

I dig a bit into pathspec.c and it looks like the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only stripping out paths inside the submodule, not the submodule itself.

I also guess that it shouldn't as otherwise you can't ever 'git add' a submodule. So I have no idea how to proceed, or even what the correct behaviour of 'git add' should be in this case. I do think that failing an assert() to tell a user he's doing something unexpected/silly/wrong isn't the right thing to do though :)

-- 
Dennis Kaarsemaker
www.kaarsemaker.net
Dennis Kaarsemaker· Apr 13, 2015, 16:57 UTC · re: Dennis Kaarsemaker · lore

Re: assert failed in submodule edge case

Err, ignore the 'segfault' bits. It's an assert() failure. He called it a segfault at first and that got stuck in my head.

On ma, 2015-04-13 at 18:55 +0200, Dennis Kaarsemaker wrote:
Show 76 quoted lines
> Reported by djanos_ in #git: git add segfaults when you manage to
> confuse it with a submodule in the index that is no longer a submodule.
> 
> Here's his script to reproduce the segfault:
> 
> mkdir segfault
> cd segfault
> 
> mkdir subrepo
> cd subrepo
> 
> git init .
> echo a > a
> git add a
> git commit -m "a"
> 
> cd ..
> 
> git init .
> git add .
> 
> cd subrepo
> rm -rf .git
> git add .
> 
> This last git add will now barf:
> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.
> 
> These all work as I think they are intended to:
> $ git -C segfault add subrepo/a
> fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
> $ git -C segfault/subrepo add a
> fatal: Pathspec 'a' is in submodule 'subrepo'
> 
> And this does nothing, it also doesn't crash:
> 
> $ git -C segfault add subrepo
> 
> GDB backtrace below, this is with next as of e6f3401.
> 
> Starting program: /home/dennis/code/git/git -C segfault/subrepo add .
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.
> 
> Program received signal SIGABRT, Aborted.
> 0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> 56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> (gdb) bt
> #0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff702c528 in __GI_abort () at abort.c:89
> #2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
>     assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len", file=file@entry=0x560826 "pathspec.c", line=line@entry=317, 
>     function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:92
> #3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len", 
>     file=file@entry=0x560826 "pathspec.c", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:101
> #4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 ".", prefixlen=8, prefix=0x7dd09f "subrepo/", flags=50, raw=0x7fffffffd648, 
>     p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316
> #5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f "subrepo/", 
>     argv=argv@entry=0x7fffffffd648) at pathspec.c:417
> #6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f "subrepo/") at builtin/add.c:370
> #7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350
> #8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532
> #9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578
> #10 main (argc=2, av=<optimized out>) at git.c:686
> 
> I dig a bit into pathspec.c and it looks like the
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only
> stripping out paths inside the submodule, not the submodule itself.
> 
> I also guess that it shouldn't as otherwise you can't ever 'git add' a
> submodule. So I have no idea how to proceed, or even what the correct
> behaviour of 'git add' should be in this case. I do think that failing
> an assert() to tell a user he's doing something unexpected/silly/wrong
> isn't the right thing to do though :)
> 
-- 
Dennis Kaarsemaker
www.kaarsemaker.net
Jens Lehmann· Apr 16, 2015, 19:27 UTC · re: Dennis Kaarsemaker · lore

Re: assert failed in submodule edge case

Am 13.04.2015 um 18:55 schrieb Dennis Kaarsemaker:
Show 20 quoted lines
> Reported by djanos_ in #git: git add segfaults when you manage to
> confuse it with a submodule in the index that is no longer a submodule.
>
> Here's his script to reproduce the segfault:
>
> mkdir segfault
> cd segfault
>
> mkdir subrepo
> cd subrepo
>
> git init .
> echo a > a
> git add a
> git commit -m "a"
>
> cd ..
>
> git init .
> git add .
If you add a "git rm -f --cached subrepo" here the problem goes away.
Show 54 quoted lines
> cd subrepo
> rm -rf .git
> git add .
>
> This last git add will now barf:
> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.
>
> These all work as I think they are intended to:
> $ git -C segfault add subrepo/a
> fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
> $ git -C segfault/subrepo add a
> fatal: Pathspec 'a' is in submodule 'subrepo'
>
> And this does nothing, it also doesn't crash:
>
> $ git -C segfault add subrepo
>
> GDB backtrace below, this is with next as of e6f3401.
>
> Starting program: /home/dennis/code/git/git -C segfault/subrepo add .
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.
>
> Program received signal SIGABRT, Aborted.
> 0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> 56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> (gdb) bt
> #0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff702c528 in __GI_abort () at abort.c:89
> #2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
>      assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len", file=file@entry=0x560826 "pathspec.c", line=line@entry=317,
>      function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:92
> #3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 "item->nowildcard_len <= item->len && item->prefix <= item->len",
>      file=file@entry=0x560826 "pathspec.c", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> "prefix_pathspec") at assert.c:101
> #4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 ".", prefixlen=8, prefix=0x7dd09f "subrepo/", flags=50, raw=0x7fffffffd648,
>      p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316
> #5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f "subrepo/",
>      argv=argv@entry=0x7fffffffd648) at pathspec.c:417
> #6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f "subrepo/") at builtin/add.c:370
> #7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350
> #8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532
> #9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578
> #10 main (argc=2, av=<optimized out>) at git.c:686
>
> I dig a bit into pathspec.c and it looks like the
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only
> stripping out paths inside the submodule, not the submodule itself.
>
> I also guess that it shouldn't as otherwise you can't ever 'git add' a
> submodule. So I have no idea how to proceed, or even what the correct
> behaviour of 'git add' should be in this case. I do think that failing
> an assert() to tell a user he's doing something unexpected/silly/wrong
> isn't the right thing to do though :)

The problem seems to be that "subrepo" is still registered as a submodule in the index. But we should see a proper error message instead of an assert in that case ... CCed Duy who knows much more about pathspec.c than me.

Nguyễn Thái Ngọc Duy· Apr 18, 2015, 01:19 UTC · re: Jens Lehmann · lore

[PATCH] pathspec: adjust prefixlen after striping trailing slash

A path(spec) from git perspective consists of two parts, the prefix, and the rest. The prefix covers the part of `pwd` after expanding ".." components. The split is to support case-insensitive match in a sane way (see 93d9353, especially the big comment block added in dir.c).

Normally the prefix is found after prefix_path_gently() and that will be it. Unfortunately the *STRIP_SUBMODULE* flags can strip the trailing slash (see 2ce53f9 for the reason) and cut into this prefix part. In the test, the prefix is "submodule/", but the final path is just "submodule". We need to readjust prefix info when this happens.

The assert() that catches this is turned to die() to make sure it's always active. prefix_pathspec() is not in any critical path to be a performance concern because of this change.

93d9353 (parse_pathspec: accept :(icase)path syntax - 2013-07-14) 2ce53f9 (git add: do not add files from a submodule - 2009-01-02)

Noticed-by: djanos_ (via irc)
Helped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, Apr 17, 2015 at 2:27 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
 > The problem seems to be that "subrepo" is still registered as a
 > submodule in the index. But we should see a proper error message
 > instead of an assert in that case ... CCed Duy who knows much
 > more about pathspec.c than me.
 This deals with the bug in pathspec code. I leave it to you to decide
 how git-add should do when a submodule is registered in index, but
 it's no longer a submodule in worktree. Yeah maybe it should error
 out.
 pathspec.c                    | 18 +++++++++++++++---
 t/t3703-add-magic-pathspec.sh |  8 ++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 9304ee3..aa5e2c7 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -262,7 +262,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	} else
 		item->original = elt;
 	item->len = strlen(item->match);
-	item->prefix = prefixlen;
 
 	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
 	    (item->len >= 1 && item->match[item->len - 1] == '/') &&
@@ -292,6 +291,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 				     elt, ce_len, ce->name);
 		}
 
+	/*
+	 * Adjust prefixlen if the above trailing slash stripping cuts
+	 * into the prefix part
+	 */
+	if ((flags & (PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)) &&
+	    prefixlen > item->len)
+		prefixlen = item->len;
+
 	if (magic & PATHSPEC_LITERAL)
 		item->nowildcard_len = item->len;
 	else {
@@ -299,6 +307,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 		if (item->nowildcard_len < prefixlen)
 			item->nowildcard_len = prefixlen;
 	}
+	item->prefix = prefixlen;
 	item->flags = 0;
 	if (magic & PATHSPEC_GLOB) {
 		/*
@@ -313,8 +322,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	}
 
 	/* sanity checks, pathspec matchers assume these are sane */
-	assert(item->nowildcard_len <= item->len &&
-	       item->prefix         <= item->len);
+	if (!(item->nowildcard_len <= item->len &&
+	      item->prefix         <= item->len))
+		die("BUG: item->nowildcard_len (%d) or item->prefix (%d)"
+		    " is longer than item->len (%d)",
+		    item->nowildcard_len, item->prefix, item->len);
 	return magic;
 }
 
diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
index 5115de7..cced8c4 100755
--- a/t/t3703-add-magic-pathspec.sh
+++ b/t/t3703-add-magic-pathspec.sh
@@ -55,4 +55,12 @@ test_expect_success COLON_DIR 'a file with the same (short) magic name exists' '
 	git add -n "./:/bar"
 '
 
+test_expect_success 'prefix is updated after trailing slash is stripped' '
+	git init submodule &&
+	( cd submodule && test_commit test ) &&
+	git add submodule &&
+	mv submodule/.git submodule/dotgit &&
+	( cd submodule && git add . )
+'
+
 test_done

-- 
2.3.0.rc1.137.g477eb31
Jens Lehmann· Apr 19, 2015, 12:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Am 18.04.2015 um 03:19 schrieb Nguyễn Thái Ngọc Duy:
Show 32 quoted lines
> A path(spec) from git perspective consists of two parts, the prefix,
> and the rest. The prefix covers the part of `pwd` after expanding ".."
> components. The split is to support case-insensitive match in a sane
> way (see 93d9353, especially the big comment block added in dir.c).
>
> Normally the prefix is found after prefix_path_gently() and that will
> be it. Unfortunately the *STRIP_SUBMODULE* flags can strip the
> trailing slash (see 2ce53f9 for the reason) and cut into this prefix
> part. In the test, the prefix is "submodule/", but the final path is
> just "submodule". We need to readjust prefix info when this happens.
>
> The assert() that catches this is turned to die() to make sure it's
> always active. prefix_pathspec() is not in any critical path to be a
> performance concern because of this change.
>
> 93d9353 (parse_pathspec: accept :(icase)path syntax - 2013-07-14)
> 2ce53f9 (git add: do not add files from a submodule - 2009-01-02)
>
> Noticed-by: djanos_ (via irc)
> Helped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>   On Fri, Apr 17, 2015 at 2:27 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>   > The problem seems to be that "subrepo" is still registered as a
>   > submodule in the index. But we should see a proper error message
>   > instead of an assert in that case ... CCed Duy who knows much
>   > more about pathspec.c than me.
>
>   This deals with the bug in pathspec code. I leave it to you to decide
>   how git-add should do when a submodule is registered in index, but
>   it's no longer a submodule in worktree. Yeah maybe it should error
>   out.

Thanks for answering with a patch! I tested it and it silences the assert and doesn't add anything below the gitlink to the index.

And me thinks it should error out. We already do that when trying to add a specific file from above the gitlink:

    $ git add subrepo/a
    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
And from below the gitlink:
    $ cd subrepo
    $ git add a
    fatal: Pathspec 'a' is in submodule 'subrepo'

I think we should do the same when using "git add ." below the gitlink.

Show 72 quoted lines
>   pathspec.c                    | 18 +++++++++++++++---
>   t/t3703-add-magic-pathspec.sh |  8 ++++++++
>   2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 9304ee3..aa5e2c7 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -262,7 +262,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>   	} else
>   		item->original = elt;
>   	item->len = strlen(item->match);
> -	item->prefix = prefixlen;
>
>   	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
>   	    (item->len >= 1 && item->match[item->len - 1] == '/') &&
> @@ -292,6 +291,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>   				     elt, ce_len, ce->name);
>   		}
>
> +	/*
> +	 * Adjust prefixlen if the above trailing slash stripping cuts
> +	 * into the prefix part
> +	 */
> +	if ((flags & (PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
> +		      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)) &&
> +	    prefixlen > item->len)
> +		prefixlen = item->len;
> +
>   	if (magic & PATHSPEC_LITERAL)
>   		item->nowildcard_len = item->len;
>   	else {
> @@ -299,6 +307,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>   		if (item->nowildcard_len < prefixlen)
>   			item->nowildcard_len = prefixlen;
>   	}
> +	item->prefix = prefixlen;
>   	item->flags = 0;
>   	if (magic & PATHSPEC_GLOB) {
>   		/*
> @@ -313,8 +322,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>   	}
>
>   	/* sanity checks, pathspec matchers assume these are sane */
> -	assert(item->nowildcard_len <= item->len &&
> -	       item->prefix         <= item->len);
> +	if (!(item->nowildcard_len <= item->len &&
> +	      item->prefix         <= item->len))
> +		die("BUG: item->nowildcard_len (%d) or item->prefix (%d)"
> +		    " is longer than item->len (%d)",
> +		    item->nowildcard_len, item->prefix, item->len);
>   	return magic;
>   }
>
> diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
> index 5115de7..cced8c4 100755
> --- a/t/t3703-add-magic-pathspec.sh
> +++ b/t/t3703-add-magic-pathspec.sh
> @@ -55,4 +55,12 @@ test_expect_success COLON_DIR 'a file with the same (short) magic name exists' '
>   	git add -n "./:/bar"
>   '
>
> +test_expect_success 'prefix is updated after trailing slash is stripped' '
> +	git init submodule &&
> +	( cd submodule && test_commit test ) &&
> +	git add submodule &&
> +	mv submodule/.git submodule/dotgit &&
> +	( cd submodule && git add . )
> +'
> +
>   test_done
>
Duy Nguyen· Apr 20, 2015, 01:34 UTC · re: Jens Lehmann · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

On Sun, Apr 19, 2015 at 7:53 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
Show 14 quoted lines
> And me thinks it should error out. We already do that when trying
> to add a specific file from above the gitlink:
>
>    $ git add subrepo/a
>    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
>
> And from below the gitlink:
>
>    $ cd subrepo
>    $ git add a
>    fatal: Pathspec 'a' is in submodule 'subrepo'
>
> I think we should do the same when using "git add ." below the
> gitlink.

But if you look at it another way, "cd subrepo; git add ." should be the same as "git add subrepo" and the latter does not error out (I think because it hits the "if (!is_directory())" in is_submodule_modified()). If we make the former error out, I suspect the latter will follow the same behavior. Just double checking if this is what we want..

-- 
Duy
Junio C Hamano· Apr 20, 2015, 05:37 UTC · re: Duy Nguyen · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Duy Nguyen <pclouds@gmail.com> writes:
> But if you look at it another way, "cd subrepo; git add ." should be
> the same as "git add subrepo" ...

Once you cd into "subrepo", you are in a different world, a world different from the toplevel project. "git add ." over there should mean "add everything in subproject's working tree to subproject's index", shouldn't it? On the other hand, "git add subrepo" without leavingin the working tree of the superproject is about binding the submodule to the superproject's index.

I do not think these two should be the same.  Where am I mistaken?
Duy Nguyen· Apr 20, 2015, 05:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> But if you look at it another way, "cd subrepo; git add ." should be
>> the same as "git add subrepo" ...
>
> Once you cd into "subrepo", you are in a different world, a world
> different from the toplevel project.  "git add ." over there should
> mean "add everything in subproject's working tree to subproject's
> index", shouldn't it?  On the other hand, "git add subrepo" without
> leavingin the working tree of the superproject is about binding the
> submodule to the superproject's index.
>
> I do not think these two should be the same.  Where am I mistaken?

I think I wrote this sentence and deleted it: I didn't say which way was right. I simply looked at this from the implementation perspective and it looked the same, at least with current impl. But we can differentiate them if we want (prefixlen is the key). A corner case if we go with erroring out in "cd subrepo; git add ." case, do we error out with "cd subrepo; git add ../subrepo"? If we want this case to error out too, I don't think I can use prefixlen to set it apart from "git add subrepo" any more..

-- 
Duy
Junio C Hamano· Apr 21, 2015, 21:08 UTC · re: Duy Nguyen · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Duy Nguyen <pclouds@gmail.com> writes:
Show 14 quoted lines
> On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> But if you look at it another way, "cd subrepo; git add ." should be
>>> the same as "git add subrepo" ...
>>
>> Once you cd into "subrepo", you are in a different world, a world
>> different from the toplevel project.  "git add ." over there should
>> mean "add everything in subproject's working tree to subproject's
>> index", shouldn't it?  On the other hand, "git add subrepo" without
>> leavingin the working tree of the superproject is about binding the
>> submodule to the superproject's index.
>>
>> I do not think these two should be the same.  Where am I mistaken?
> I think I wrote this sentence and deleted it: I didn't say which way
> was right.
OK.

I looked at the test script update. The new test does (I am rephrasing to make it clearer):

    mkdir -p dir/ectory
    git init dir/ectory ;# a new directory inside top-level project
    (
        cd dir/ectory &&
        >test && git add test && git commit -m test
    )
    git add dir/ectory

to set it up. At this point, the top-level index knows dir/ectory is a submodule.

Then the test goes on to turn it a non submodule by
    mv dir/ectory/.git dir/ectory/dotgit
At this point, I think there are two valid things that can happen.
 (1) "git add test" inside dir/ectory removes the submodule entry
     "dir/ectory" and then adds dir/ectory/test as an individual
     entry.  After all that is what happens when you replace a file
     with a directory, e.g.
       >folder && git add folder
       rm folder
       mkdir folder && >folder/file &&
       git -C folder add file
     will first register a regular file "folder" and then replace
     that with paths underneath.
     It would be the same if you did any of the following:
       git -C . add dir/ectory/test
       git -C dir add ectory/test
       git -C dir/ectory add test
 (2) "git add test" inside "dir/ectory" would barf, saying
     "dir/ectory is supposed to be a submodule and you have to first
     remove it".  Again, it would be the same if you did so from any
     directory with relative paths.

I think (2) is less optimal than (1), but it could be the best we could do if the submodule infrastracture around .gitmodules and links to $GIT_DIR/modules/$name may have to get involved in an operation like this (I didn't check).

In my quick test, it appeared that the behaviour with this patch applied was neither of the two and instead to silently do nothing, and if that is the case I think it is quite wrong.

Jens Lehmann· Apr 22, 2015, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Am 21.04.2015 um 23:08 schrieb Junio C Hamano:
Show 70 quoted lines
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>>
>>>> But if you look at it another way, "cd subrepo; git add ." should be
>>>> the same as "git add subrepo" ...
>>>
>>> Once you cd into "subrepo", you are in a different world, a world
>>> different from the toplevel project.  "git add ." over there should
>>> mean "add everything in subproject's working tree to subproject's
>>> index", shouldn't it?  On the other hand, "git add subrepo" without
>>> leavingin the working tree of the superproject is about binding the
>>> submodule to the superproject's index.
>>>
>>> I do not think these two should be the same.  Where am I mistaken?
>
>> I think I wrote this sentence and deleted it: I didn't say which way
>> was right.
>
> OK.
>
> I looked at the test script update.  The new test does (I am
> rephrasing to make it clearer):
>
>      mkdir -p dir/ectory
>      git init dir/ectory ;# a new directory inside top-level project
>      (
>          cd dir/ectory &&
>          >test && git add test && git commit -m test
>      )
>      git add dir/ectory
>
> to set it up.  At this point, the top-level index knows dir/ectory
> is a submodule.
>
> Then the test goes on to turn it a non submodule by
>
>      mv dir/ectory/.git dir/ectory/dotgit
>
> At this point, I think there are two valid things that can happen.
>
>   (1) "git add test" inside dir/ectory removes the submodule entry
>       "dir/ectory" and then adds dir/ectory/test as an individual
>       entry.  After all that is what happens when you replace a file
>       with a directory, e.g.
>
>         >folder && git add folder
>         rm folder
>         mkdir folder && >folder/file &&
>         git -C folder add file
>
>       will first register a regular file "folder" and then replace
>       that with paths underneath.
>
>       It would be the same if you did any of the following:
>
>         git -C . add dir/ectory/test
>         git -C dir add ectory/test
>         git -C dir/ectory add test
>
>   (2) "git add test" inside "dir/ectory" would barf, saying
>       "dir/ectory is supposed to be a submodule and you have to first
>       remove it".  Again, it would be the same if you did so from any
>       directory with relative paths.
>
> I think (2) is less optimal than (1), but it could be the best we
> could do if the submodule infrastracture around .gitmodules and
> links to $GIT_DIR/modules/$name may have to get involved in an
> operation like this (I didn't check).
We already do (2) in the cases you describe:
    $ git add subrepo/a
    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
    $ git -C subrepo add a
    fatal: Pathspec 'a' is in submodule 'subrepo'

And I believe that is better than (1), because when removing a submodule its entry in .gitmodules should also be removed. And I suspect that adding a file in a submodule to the superproject will happen more by accident than on purpose (which cannot happen for files because when you add a new file in a directory for which git still records a file the latter can be safely removed as that entry cannot be a file in the worktree anymore).

So I'd vote to have (2) also for "git -C subrepo add .", which is what started this thread.

> In my quick test, it appeared that the behaviour with this patch
> applied was neither of the two and instead to silently do nothing,
> and if that is the case I think it is quite wrong.
Yep.
Junio C Hamano· Apr 22, 2015, 19:58 UTC · re: Jens Lehmann · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Jens Lehmann <Jens.Lehmann@web.de> writes:
Show 30 quoted lines
> Am 21.04.2015 um 23:08 schrieb Junio C Hamano:
>
>> I looked at the test script update.  The new test does (I am
>> rephrasing to make it clearer):
>>
>>      mkdir -p dir/ectory
>>      git init dir/ectory ;# a new directory inside top-level project
>>      (
>>          cd dir/ectory &&
>>          >test && git add test && git commit -m test
>>      )
>>      git add dir/ectory
>>
>> to set it up.  At this point, the top-level index knows dir/ectory
>> is a submodule.
>>
>> Then the test goes on to turn it a non submodule by
>>
>>      mv dir/ectory/.git dir/ectory/dotgit
>> ...
>
> We already do (2) in the cases you describe:
>
>    $ git add subrepo/a
>    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
>    $ git -C subrepo add a
>    fatal: Pathspec 'a' is in submodule 'subrepo'
> ...
> So I'd vote to have (2) also for "git -C subrepo add .", which
> is what started this thread.

Does having "mv subrepo/.git subrepo/dotgit" before that "git add" change your conclusion?

It is very clear to me that without that "mv" step, (2) is absolutely the right thing to do, and I agree with you.

But it is unclear if we should still do (2) when "subrepo/.git" is no longer there. That has to be done manually and it may be an indication that is clear enough that the end user wants the directory to be a normal directory without any submodule involved, in which case it may match the expectation of the user better to just nuke the corresponding 160000 entry in the index and replace it with files in there. I dunno.

Show 5 quoted lines
>> In my quick test, it appeared that the behaviour with this patch
>> applied was neither of the two and instead to silently do nothing,
>> and if that is the case I think it is quite wrong.
>
> Yep.
Jens Lehmann· Apr 22, 2015, 22:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Am 22.04.2015 um 21:58 schrieb Junio C Hamano:
Show 38 quoted lines
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Am 21.04.2015 um 23:08 schrieb Junio C Hamano:
>>
>>> I looked at the test script update.  The new test does (I am
>>> rephrasing to make it clearer):
>>>
>>>       mkdir -p dir/ectory
>>>       git init dir/ectory ;# a new directory inside top-level project
>>>       (
>>>           cd dir/ectory &&
>>>           >test && git add test && git commit -m test
>>>       )
>>>       git add dir/ectory
>>>
>>> to set it up.  At this point, the top-level index knows dir/ectory
>>> is a submodule.
>>>
>>> Then the test goes on to turn it a non submodule by
>>>
>>>       mv dir/ectory/.git dir/ectory/dotgit
>>> ...
>>
>> We already do (2) in the cases you describe:
>>
>>     $ git add subrepo/a
>>     fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'
>>     $ git -C subrepo add a
>>     fatal: Pathspec 'a' is in submodule 'subrepo'
>> ...
>> So I'd vote to have (2) also for "git -C subrepo add .", which
>> is what started this thread.
>
> Does having "mv subrepo/.git subrepo/dotgit" before that "git add"
> change your conclusion?
>
> It is very clear to me that without that "mv" step, (2) is
> absolutely the right thing to do, and I agree with you.

Huh? Without that mv all files would simply be added to the submodule repo and this would be a non-issue ... what am I missing? I'm just advocating to let "git add ." in a submodule without a .git behave like "git add <file>" already does.

Show 7 quoted lines
> But it is unclear if we should still do (2) when "subrepo/.git" is
> no longer there.  That has to be done manually and it may be an
> indication that is clear enough that the end user wants the
> directory to be a normal directory without any submodule involved,
> in which case it may match the expectation of the user better to
> just nuke the corresponding 160000 entry in the index and replace it
> with files in there.  I dunno.

The user having removed subrepo/.git is just one reason for that. Another is a user adding a file in an unpopulated work tree of a not initialized submodule. I doubt that simply nuking the 160000 entry would be the right thing to do in this case, I expect this to be a pilot error we should barf about ;-)

Junio C Hamano· Apr 23, 2015, 03:47 UTC · re: Jens Lehmann · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

On Wed, Apr 22, 2015 at 3:32 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
Show 14 quoted lines
> ...
>> But it is unclear if we should still do (2) when "subrepo/.git" is
>> no longer there.  That has to be done manually and it may be an
>> indication that is clear enough that the end user wants the
>> directory to be a normal directory without any submodule involved,
>> in which case it may match the expectation of the user better to
>> just nuke the corresponding 160000 entry in the index and replace it
>> with files in there.  I dunno.
>
> The user having removed subrepo/.git is just one reason for that.
> Another is a user adding a file in an unpopulated work tree of a
> not initialized submodule. I doubt that simply nuking the 160000
> entry would be the right thing to do in this case, I expect this
> to be a pilot error we should barf about ;-)
OK, that sounds sensible.
Duy Nguyen· Jun 14, 2015, 13:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

On Thu, Apr 23, 2015 at 10:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> On Wed, Apr 22, 2015 at 3:32 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> ...
>>> But it is unclear if we should still do (2) when "subrepo/.git" is
>>> no longer there.  That has to be done manually and it may be an
>>> indication that is clear enough that the end user wants the
>>> directory to be a normal directory without any submodule involved,
>>> in which case it may match the expectation of the user better to
>>> just nuke the corresponding 160000 entry in the index and replace it
>>> with files in there.  I dunno.
>>
>> The user having removed subrepo/.git is just one reason for that.
>> Another is a user adding a file in an unpopulated work tree of a
>> not initialized submodule. I doubt that simply nuking the 160000
>> entry would be the right thing to do in this case, I expect this
>> to be a pilot error we should barf about ;-)
>
> OK, that sounds sensible.

There are more to this "submodule vs the world". When .git is gone, if you expect to warn instead of deleting 160000. You may want the same for the opposite direction: when b/c is in the index and you add b/.git, you may want "git add b" to complain too (right not it ignores b/.git and updates b/c).

We may need more surgery around this area. It seems to be the "Path %s is in submodule" check (in pathspec.c) is at the wrong place. We have d/f check in add_index_entry_with_check. That should catch it, at the core, not from call sites like add or update-index. But that check is not active...

The first version of "path is in submodule" is in 2ce53f9 (git add: do not add files from a submodule - 2009-01-02). Back then dir.c does not intervene. If you have "b" as 160000 in the index and tries to add b/c, the ok_to_replace feature kicks in and kills 'b'. This is why d/f check is turned off. This ok_to_replace is to deal with symlinks, see 192268c (Add git-update-cache --replace option. - 2005-05-07).

I think we should stop the ok-to-replace feature when submodules are involved, we consider submdules much more valuable than symlinks. If we do this, I think we can delete those "Path is in submodule" trick becaue the index core can handle it well.

-- 
Duy
Junio C Hamano· Jun 14, 2015, 21:34 UTC · re: Duy Nguyen · lore

Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash

Duy Nguyen <pclouds@gmail.com> writes:
> I think we should stop the ok-to-replace feature when submodules are
> involved, we consider submdules much more valuable than symlinks.

Hmm, I am not sure "valuable" is a good criterion to decide what should happen, though.

The push to use ".git" that is a file pointing at a repository that is safely stored away is a move to make valuable submodule more easily removable from the working tree without losing information, so that we can remove it from the working tree when the user instructs us to, just like we can remove a symlink safely without losing information. The only thing we need to be careful is that that the path that corresponds to the index entry is not "dirty". That is, for a symlink, if you make it point at something different without doing "git add" it, you would lose that working-tree-only change when we "kill" that symbolic link in order to replace it with a regular directory. For a submodule, if you have uncommitted changes in the submodule working tree, you would lose that the same way when we "kill" that submodule in order to replace that directory as part of the superproject's working tree.

There may need some more safety implemented (i.e. how we detect the "dirty"-ness and when we stop the operation based on that), but I would imagine there is nothing fundamentally special about submodule that does not apply to a symlink or a normal blob.

← back to recent threads