assert failed in submodule edge case

assert failed in submodule edge case

[Dennis Kaarsemaker]

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

Re: assert failed in submodule edge case

[Dennis Kaarsemaker]

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

Re: assert failed in submodule edge case

[Jens Lehmann]

Am 13.04.2015 um 18:55 schrieb Dennis Kaarsemaker:
> 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.

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

[PATCH] pathspec: adjust prefixlen after striping trailing slash

[Nguyễn Thái Ngọc Duy]

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

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

[Jens Lehmann]

Am 18.04.2015 um 03:19 schrieb Nguyễn Thái Ngọc Duy:
> 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.

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

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

[Duy Nguyen]

On Sun, Apr 19, 2015 at 7:53 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> 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

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

[Junio C Hamano]

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?

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

[Duy Nguyen]

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

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

[Junio C Hamano]

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

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.

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

[Jens Lehmann]

Am 21.04.2015 um 23:08 schrieb Junio C Hamano:
> 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.

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

[Junio C Hamano]

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.

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.

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

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

[Jens Lehmann]

Am 22.04.2015 um 21:58 schrieb Junio C Hamano:
> 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.

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

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

[Junio C Hamano]

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.

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

[Duy Nguyen]

On Thu, Apr 23, 2015 at 10:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.