{"thread":{"id":"39064","subject":"assert failed in submodule edge case","startedAt":"2015-04-13T16:55:57Z","lastAt":"2015-06-14T21:34:45Z","messageCount":15,"participants":["Dennis Kaarsemaker","Jens Lehmann","Nguyễn Thái Ngọc Duy","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259305","messageId":"1428944157.13660.8.camel@kaarsemaker.net","threadId":"39064","inReplyTo":null,"subject":"assert failed in submodule edge case","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-04-13T16:55:57Z","receivedAt":"2015-04-13T16:55:57Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Reported by djanos_ in #git: git add segfaults when you manage to\nconfuse it with a submodule in the index that is no longer a submodule.\n\nHere's his script to reproduce the segfault:\n\nmkdir segfault\ncd segfault\n\nmkdir subrepo\ncd subrepo\n\ngit init .\necho a > a\ngit add a\ngit commit -m \"a\"\n\ncd ..\n\ngit init .\ngit add .\n\ncd subrepo\nrm -rf .git\ngit add .\n\nThis last git add will now barf:\ngit: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n\nThese all work as I think they are intended to:\n$ git -C segfault add subrepo/a\nfatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n$ git -C segfault/subrepo add a\nfatal: Pathspec 'a' is in submodule 'subrepo'\n\nAnd this does nothing, it also doesn't crash:\n\n$ git -C segfault add subrepo\n\nGDB backtrace below, this is with next as of e6f3401.\n\nStarting program: /home/dennis/code/git/git -C segfault/subrepo add .\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\ngit: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n\nProgram received signal SIGABRT, Aborted.\n0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n56\t../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.\n(gdb) bt\n#0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n#1  0x00007ffff702c528 in __GI_abort () at abort.c:89\n#2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 \"%s%s%s:%u: %s%sAssertion `%s' failed.\\n%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, \n    function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:92\n#3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 \"item->nowildcard_len <= item->len && item->prefix <= item->len\", \n    file=file@entry=0x560826 \"pathspec.c\", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:101\n#4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 \".\", prefixlen=8, prefix=0x7dd09f \"subrepo/\", flags=50, raw=0x7fffffffd648, \n    p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316\n#5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f \"subrepo/\", \n    argv=argv@entry=0x7fffffffd648) at pathspec.c:417\n#6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f \"subrepo/\") at builtin/add.c:370\n#7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350\n#8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532\n#9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578\n#10 main (argc=2, av=<optimized out>) at git.c:686\n\nI dig a bit into pathspec.c and it looks like the\nPATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only\nstripping out paths inside the submodule, not the submodule itself.\n\nI also guess that it shouldn't as otherwise you can't ever 'git add' a\nsubmodule. So I have no idea how to proceed, or even what the correct\nbehaviour of 'git add' should be in this case. I do think that failing\nan assert() to tell a user he's doing something unexpected/silly/wrong\nisn't the right thing to do though :)\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"259306","messageId":"1428944256.13660.9.camel@kaarsemaker.net","threadId":"39064","inReplyTo":"1428944157.13660.8.camel@kaarsemaker.net","subject":"Re: assert failed in submodule edge case","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-04-13T16:57:36Z","receivedAt":"2015-04-13T16:57:36Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Err, ignore the 'segfault' bits. It's an assert() failure. He called it\na segfault at first and that got stuck in my head.\n\nOn ma, 2015-04-13 at 18:55 +0200, Dennis Kaarsemaker wrote:\n> Reported by djanos_ in #git: git add segfaults when you manage to\n> confuse it with a submodule in the index that is no longer a submodule.\n> \n> Here's his script to reproduce the segfault:\n> \n> mkdir segfault\n> cd segfault\n> \n> mkdir subrepo\n> cd subrepo\n> \n> git init .\n> echo a > a\n> git add a\n> git commit -m \"a\"\n> \n> cd ..\n> \n> git init .\n> git add .\n> \n> cd subrepo\n> rm -rf .git\n> git add .\n> \n> This last git add will now barf:\n> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n> \n> These all work as I think they are intended to:\n> $ git -C segfault add subrepo/a\n> fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n> $ git -C segfault/subrepo add a\n> fatal: Pathspec 'a' is in submodule 'subrepo'\n> \n> And this does nothing, it also doesn't crash:\n> \n> $ git -C segfault add subrepo\n> \n> GDB backtrace below, this is with next as of e6f3401.\n> \n> Starting program: /home/dennis/code/git/git -C segfault/subrepo add .\n> [Thread debugging using libthread_db enabled]\n> Using host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n> \n> Program received signal SIGABRT, Aborted.\n> 0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n> 56\t../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.\n> (gdb) bt\n> #0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n> #1  0x00007ffff702c528 in __GI_abort () at abort.c:89\n> #2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 \"%s%s%s:%u: %s%sAssertion `%s' failed.\\n%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, \n>     function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:92\n> #3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 \"item->nowildcard_len <= item->len && item->prefix <= item->len\", \n>     file=file@entry=0x560826 \"pathspec.c\", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:101\n> #4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 \".\", prefixlen=8, prefix=0x7dd09f \"subrepo/\", flags=50, raw=0x7fffffffd648, \n>     p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316\n> #5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f \"subrepo/\", \n>     argv=argv@entry=0x7fffffffd648) at pathspec.c:417\n> #6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f \"subrepo/\") at builtin/add.c:370\n> #7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350\n> #8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532\n> #9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578\n> #10 main (argc=2, av=<optimized out>) at git.c:686\n> \n> I dig a bit into pathspec.c and it looks like the\n> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only\n> stripping out paths inside the submodule, not the submodule itself.\n> \n> I also guess that it shouldn't as otherwise you can't ever 'git add' a\n> submodule. So I have no idea how to proceed, or even what the correct\n> behaviour of 'git add' should be in this case. I do think that failing\n> an assert() to tell a user he's doing something unexpected/silly/wrong\n> isn't the right thing to do though :)\n> \n\n\n\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"259537","messageId":"55300D2C.9030903@web.de","threadId":"39064","inReplyTo":"1428944157.13660.8.camel@kaarsemaker.net","subject":"Re: assert failed in submodule edge case","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-04-16T19:27:40Z","receivedAt":"2015-04-16T19:27:40Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.04.2015 um 18:55 schrieb Dennis Kaarsemaker:\n> Reported by djanos_ in #git: git add segfaults when you manage to\n> confuse it with a submodule in the index that is no longer a submodule.\n>\n> Here's his script to reproduce the segfault:\n>\n> mkdir segfault\n> cd segfault\n>\n> mkdir subrepo\n> cd subrepo\n>\n> git init .\n> echo a > a\n> git add a\n> git commit -m \"a\"\n>\n> cd ..\n>\n> git init .\n> git add .\n\nIf you add a \"git rm -f --cached subrepo\" here the problem goes away.\n\n> cd subrepo\n> rm -rf .git\n> git add .\n>\n> This last git add will now barf:\n> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n>\n> These all work as I think they are intended to:\n> $ git -C segfault add subrepo/a\n> fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n> $ git -C segfault/subrepo add a\n> fatal: Pathspec 'a' is in submodule 'subrepo'\n>\n> And this does nothing, it also doesn't crash:\n>\n> $ git -C segfault add subrepo\n>\n> GDB backtrace below, this is with next as of e6f3401.\n>\n> Starting program: /home/dennis/code/git/git -C segfault/subrepo add .\n> [Thread debugging using libthread_db enabled]\n> Using host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n>\n> Program received signal SIGABRT, Aborted.\n> 0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n> 56\t../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.\n> (gdb) bt\n> #0  0x00007ffff702ae37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56\n> #1  0x00007ffff702c528 in __GI_abort () at abort.c:89\n> #2  0x00007ffff7023ce6 in __assert_fail_base (fmt=0x7ffff7173c08 \"%s%s%s:%u: %s%sAssertion `%s' failed.\\n%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,\n>      function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:92\n> #3  0x00007ffff7023d92 in __GI___assert_fail (assertion=assertion@entry=0x560660 \"item->nowildcard_len <= item->len && item->prefix <= item->len\",\n>      file=file@entry=0x560826 \"pathspec.c\", line=line@entry=317, function=function@entry=0x560850 <__PRETTY_FUNCTION__.22237> \"prefix_pathspec\") at assert.c:101\n> #4  0x00000000004d6a37 in prefix_pathspec (elt=0x7fffffffdaf1 \".\", prefixlen=8, prefix=0x7dd09f \"subrepo/\", flags=50, raw=0x7fffffffd648,\n>      p_short_magic=<synthetic pointer>, item=<optimized out>) at pathspec.c:316\n> #5  parse_pathspec (pathspec=pathspec@entry=0x7fffffffd240, magic_mask=magic_mask@entry=0, flags=flags@entry=50, prefix=prefix@entry=0x7dd09f \"subrepo/\",\n>      argv=argv@entry=0x7fffffffd648) at pathspec.c:417\n> #6  0x000000000040698c in cmd_add (argc=<optimized out>, argv=0x7fffffffd648, prefix=0x7dd09f \"subrepo/\") at builtin/add.c:370\n> #7  0x0000000000406001 in run_builtin (argv=0x7fffffffd640, argc=2, p=0x79d740 <commands>) at git.c:350\n> #8  handle_builtin (argc=2, argv=0x7fffffffd640) at git.c:532\n> #9  0x0000000000405151 in run_argv (argv=0x7fffffffd458, argcp=0x7fffffffd43c) at git.c:578\n> #10 main (argc=2, av=<optimized out>) at git.c:686\n>\n> I dig a bit into pathspec.c and it looks like the\n> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE code in prefix_pathspec is only\n> stripping out paths inside the submodule, not the submodule itself.\n>\n> I also guess that it shouldn't as otherwise you can't ever 'git add' a\n> submodule. So I have no idea how to proceed, or even what the correct\n> behaviour of 'git add' should be in this case. I do think that failing\n> an assert() to tell a user he's doing something unexpected/silly/wrong\n> isn't the right thing to do though :)\n\nThe problem seems to be that \"subrepo\" is still registered as a\nsubmodule in the index. But we should see a proper error message\ninstead of an assert in that case ... CCed Duy who knows much\nmore about pathspec.c than me.\n"},{"id":"259611","messageId":"1429319946-19890-1-git-send-email-pclouds@gmail.com","threadId":"39064","inReplyTo":"55300D2C.9030903@web.de","subject":"[PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-04-18T01:19:06Z","receivedAt":"2015-04-18T01:19:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"A path(spec) from git perspective consists of two parts, the prefix,\nand the rest. The prefix covers the part of `pwd` after expanding \"..\"\ncomponents. The split is to support case-insensitive match in a sane\nway (see 93d9353, especially the big comment block added in dir.c).\n\nNormally the prefix is found after prefix_path_gently() and that will\nbe it. Unfortunately the *STRIP_SUBMODULE* flags can strip the\ntrailing slash (see 2ce53f9 for the reason) and cut into this prefix\npart. In the test, the prefix is \"submodule/\", but the final path is\njust \"submodule\". We need to readjust prefix info when this happens.\n\nThe assert() that catches this is turned to die() to make sure it's\nalways active. prefix_pathspec() is not in any critical path to be a\nperformance concern because of this change.\n\n93d9353 (parse_pathspec: accept :(icase)path syntax - 2013-07-14)\n2ce53f9 (git add: do not add files from a submodule - 2009-01-02)\n\nNoticed-by: djanos_ (via irc)\nHelped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Fri, Apr 17, 2015 at 2:27 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n > The problem seems to be that \"subrepo\" is still registered as a\n > submodule in the index. But we should see a proper error message\n > instead of an assert in that case ... CCed Duy who knows much\n > more about pathspec.c than me.\n\n This deals with the bug in pathspec code. I leave it to you to decide\n how git-add should do when a submodule is registered in index, but\n it's no longer a submodule in worktree. Yeah maybe it should error\n out.\n\n pathspec.c                    | 18 +++++++++++++++---\n t/t3703-add-magic-pathspec.sh |  8 ++++++++\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 9304ee3..aa5e2c7 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -262,7 +262,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t} else\n \t\titem->original = elt;\n \titem->len = strlen(item->match);\n-\titem->prefix = prefixlen;\n \n \tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n \t    (item->len >= 1 && item->match[item->len - 1] == '/') &&\n@@ -292,6 +291,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\t\t\t     elt, ce_len, ce->name);\n \t\t}\n \n+\t/*\n+\t * Adjust prefixlen if the above trailing slash stripping cuts\n+\t * into the prefix part\n+\t */\n+\tif ((flags & (PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n+\t\t      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)) &&\n+\t    prefixlen > item->len)\n+\t\tprefixlen = item->len;\n+\n \tif (magic & PATHSPEC_LITERAL)\n \t\titem->nowildcard_len = item->len;\n \telse {\n@@ -299,6 +307,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\tif (item->nowildcard_len < prefixlen)\n \t\t\titem->nowildcard_len = prefixlen;\n \t}\n+\titem->prefix = prefixlen;\n \titem->flags = 0;\n \tif (magic & PATHSPEC_GLOB) {\n \t\t/*\n@@ -313,8 +322,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t}\n \n \t/* sanity checks, pathspec matchers assume these are sane */\n-\tassert(item->nowildcard_len <= item->len &&\n-\t       item->prefix         <= item->len);\n+\tif (!(item->nowildcard_len <= item->len &&\n+\t      item->prefix         <= item->len))\n+\t\tdie(\"BUG: item->nowildcard_len (%d) or item->prefix (%d)\"\n+\t\t    \" is longer than item->len (%d)\",\n+\t\t    item->nowildcard_len, item->prefix, item->len);\n \treturn magic;\n }\n \ndiff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh\nindex 5115de7..cced8c4 100755\n--- a/t/t3703-add-magic-pathspec.sh\n+++ b/t/t3703-add-magic-pathspec.sh\n@@ -55,4 +55,12 @@ test_expect_success COLON_DIR 'a file with the same (short) magic name exists' '\n \tgit add -n \"./:/bar\"\n '\n \n+test_expect_success 'prefix is updated after trailing slash is stripped' '\n+\tgit init submodule &&\n+\t( cd submodule && test_commit test ) &&\n+\tgit add submodule &&\n+\tmv submodule/.git submodule/dotgit &&\n+\t( cd submodule && git add . )\n+'\n+\n test_done\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"259646","messageId":"5533A567.7070301@web.de","threadId":"39064","inReplyTo":"1429319946-19890-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-04-19T12:53:59Z","receivedAt":"2015-04-19T12:53:59Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.04.2015 um 03:19 schrieb Nguyễn Thái Ngọc Duy:\n> A path(spec) from git perspective consists of two parts, the prefix,\n> and the rest. The prefix covers the part of `pwd` after expanding \"..\"\n> components. The split is to support case-insensitive match in a sane\n> way (see 93d9353, especially the big comment block added in dir.c).\n>\n> Normally the prefix is found after prefix_path_gently() and that will\n> be it. Unfortunately the *STRIP_SUBMODULE* flags can strip the\n> trailing slash (see 2ce53f9 for the reason) and cut into this prefix\n> part. In the test, the prefix is \"submodule/\", but the final path is\n> just \"submodule\". We need to readjust prefix info when this happens.\n>\n> The assert() that catches this is turned to die() to make sure it's\n> always active. prefix_pathspec() is not in any critical path to be a\n> performance concern because of this change.\n>\n> 93d9353 (parse_pathspec: accept :(icase)path syntax - 2013-07-14)\n> 2ce53f9 (git add: do not add files from a submodule - 2009-01-02)\n>\n> Noticed-by: djanos_ (via irc)\n> Helped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>   On Fri, Apr 17, 2015 at 2:27 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n>   > The problem seems to be that \"subrepo\" is still registered as a\n>   > submodule in the index. But we should see a proper error message\n>   > instead of an assert in that case ... CCed Duy who knows much\n>   > more about pathspec.c than me.\n>\n>   This deals with the bug in pathspec code. I leave it to you to decide\n>   how git-add should do when a submodule is registered in index, but\n>   it's no longer a submodule in worktree. Yeah maybe it should error\n>   out.\n\nThanks for answering with a patch! I tested it and it silences the\nassert and doesn't add anything below the gitlink to the index.\n\nAnd me thinks it should error out. We already do that when trying\nto add a specific file from above the gitlink:\n\n    $ git add subrepo/a\n    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n\nAnd from below the gitlink:\n\n    $ cd subrepo\n    $ git add a\n    fatal: Pathspec 'a' is in submodule 'subrepo'\n\nI think we should do the same when using \"git add .\" below the\ngitlink.\n\n>   pathspec.c                    | 18 +++++++++++++++---\n>   t/t3703-add-magic-pathspec.sh |  8 ++++++++\n>   2 files changed, 23 insertions(+), 3 deletions(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 9304ee3..aa5e2c7 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -262,7 +262,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>   \t} else\n>   \t\titem->original = elt;\n>   \titem->len = strlen(item->match);\n> -\titem->prefix = prefixlen;\n>\n>   \tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n>   \t    (item->len >= 1 && item->match[item->len - 1] == '/') &&\n> @@ -292,6 +291,15 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>   \t\t\t\t     elt, ce_len, ce->name);\n>   \t\t}\n>\n> +\t/*\n> +\t * Adjust prefixlen if the above trailing slash stripping cuts\n> +\t * into the prefix part\n> +\t */\n> +\tif ((flags & (PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n> +\t\t      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)) &&\n> +\t    prefixlen > item->len)\n> +\t\tprefixlen = item->len;\n> +\n>   \tif (magic & PATHSPEC_LITERAL)\n>   \t\titem->nowildcard_len = item->len;\n>   \telse {\n> @@ -299,6 +307,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>   \t\tif (item->nowildcard_len < prefixlen)\n>   \t\t\titem->nowildcard_len = prefixlen;\n>   \t}\n> +\titem->prefix = prefixlen;\n>   \titem->flags = 0;\n>   \tif (magic & PATHSPEC_GLOB) {\n>   \t\t/*\n> @@ -313,8 +322,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>   \t}\n>\n>   \t/* sanity checks, pathspec matchers assume these are sane */\n> -\tassert(item->nowildcard_len <= item->len &&\n> -\t       item->prefix         <= item->len);\n> +\tif (!(item->nowildcard_len <= item->len &&\n> +\t      item->prefix         <= item->len))\n> +\t\tdie(\"BUG: item->nowildcard_len (%d) or item->prefix (%d)\"\n> +\t\t    \" is longer than item->len (%d)\",\n> +\t\t    item->nowildcard_len, item->prefix, item->len);\n>   \treturn magic;\n>   }\n>\n> diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh\n> index 5115de7..cced8c4 100755\n> --- a/t/t3703-add-magic-pathspec.sh\n> +++ b/t/t3703-add-magic-pathspec.sh\n> @@ -55,4 +55,12 @@ test_expect_success COLON_DIR 'a file with the same (short) magic name exists' '\n>   \tgit add -n \"./:/bar\"\n>   '\n>\n> +test_expect_success 'prefix is updated after trailing slash is stripped' '\n> +\tgit init submodule &&\n> +\t( cd submodule && test_commit test ) &&\n> +\tgit add submodule &&\n> +\tmv submodule/.git submodule/dotgit &&\n> +\t( cd submodule && git add . )\n> +'\n> +\n>   test_done\n>\n"},{"id":"259653","messageId":"CACsJy8A7+rQhn5ry6Z86SbeYHoDw=w7tB73Ls2V05E8F-JMnhA@mail.gmail.com","threadId":"39064","inReplyTo":"5533A567.7070301@web.de","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-04-20T01:34:33Z","receivedAt":"2015-04-20T01:34:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Apr 19, 2015 at 7:53 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> And me thinks it should error out. We already do that when trying\n> to add a specific file from above the gitlink:\n>\n>    $ git add subrepo/a\n>    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n>\n> And from below the gitlink:\n>\n>    $ cd subrepo\n>    $ git add a\n>    fatal: Pathspec 'a' is in submodule 'subrepo'\n>\n> I think we should do the same when using \"git add .\" below the\n> gitlink.\n\nBut if you look at it another way, \"cd subrepo; git add .\" should be\nthe same as \"git add subrepo\" and the latter does not error out (I\nthink because it hits the \"if (!is_directory())\" in\nis_submodule_modified()). If we make the former error out, I suspect\nthe latter will follow the same behavior. Just double checking if this\nis what we want..\n-- \nDuy\n"},{"id":"259658","messageId":"xmqqy4lnmkbv.fsf@gitster.dls.corp.google.com","threadId":"39064","inReplyTo":"CACsJy8A7+rQhn5ry6Z86SbeYHoDw=w7tB73Ls2V05E8F-JMnhA@mail.gmail.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-20T05:37:24Z","receivedAt":"2015-04-20T05:37:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> But if you look at it another way, \"cd subrepo; git add .\" should be\n> the same as \"git add subrepo\" ...\n\nOnce you cd into \"subrepo\", you are in a different world, a world\ndifferent from the toplevel project.  \"git add .\" over there should\nmean \"add everything in subproject's working tree to subproject's\nindex\", shouldn't it?  On the other hand, \"git add subrepo\" without\nleavingin the working tree of the superproject is about binding the\nsubmodule to the superproject's index.\n\nI do not think these two should be the same.  Where am I mistaken?\n"},{"id":"259660","messageId":"CACsJy8B1QWzehAEtjnQeRJ8uehcFFDbdHSViwk_JtrtYehV8ZQ@mail.gmail.com","threadId":"39064","inReplyTo":"xmqqy4lnmkbv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-04-20T05:52:02Z","receivedAt":"2015-04-20T05:52:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> But if you look at it another way, \"cd subrepo; git add .\" should be\n>> the same as \"git add subrepo\" ...\n>\n> Once you cd into \"subrepo\", you are in a different world, a world\n> different from the toplevel project.  \"git add .\" over there should\n> mean \"add everything in subproject's working tree to subproject's\n> index\", shouldn't it?  On the other hand, \"git add subrepo\" without\n> leavingin the working tree of the superproject is about binding the\n> submodule to the superproject's index.\n>\n> I do not think these two should be the same.  Where am I mistaken?\n\nI think I wrote this sentence and deleted it: I didn't say which way\nwas right. I simply looked at this from the implementation perspective\nand it looked the same, at least with current impl. But we can\ndifferentiate them if we want (prefixlen is the key). A corner case if\nwe go with erroring out in \"cd subrepo; git add .\" case, do we error\nout with \"cd subrepo; git add ../subrepo\"? If we want this case to\nerror out too, I don't think I can use prefixlen to set it apart from\n\"git add subrepo\" any more..\n-- \nDuy\n"},{"id":"259760","messageId":"xmqq8udlgpey.fsf@gitster.dls.corp.google.com","threadId":"39064","inReplyTo":"CACsJy8B1QWzehAEtjnQeRJ8uehcFFDbdHSViwk_JtrtYehV8ZQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-21T21:08:37Z","receivedAt":"2015-04-21T21:08:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Duy Nguyen <pclouds@gmail.com> writes:\n>>\n>>> But if you look at it another way, \"cd subrepo; git add .\" should be\n>>> the same as \"git add subrepo\" ...\n>>\n>> Once you cd into \"subrepo\", you are in a different world, a world\n>> different from the toplevel project.  \"git add .\" over there should\n>> mean \"add everything in subproject's working tree to subproject's\n>> index\", shouldn't it?  On the other hand, \"git add subrepo\" without\n>> leavingin the working tree of the superproject is about binding the\n>> submodule to the superproject's index.\n>>\n>> I do not think these two should be the same.  Where am I mistaken?\n\n> I think I wrote this sentence and deleted it: I didn't say which way\n> was right.\n\nOK.\n\nI looked at the test script update.  The new test does (I am\nrephrasing to make it clearer):\n\n    mkdir -p dir/ectory\n    git init dir/ectory ;# a new directory inside top-level project\n    (\n        cd dir/ectory &&\n        >test && git add test && git commit -m test\n    )\n    git add dir/ectory\n\nto set it up.  At this point, the top-level index knows dir/ectory\nis a submodule.\n\nThen the test goes on to turn it a non submodule by\n\n    mv dir/ectory/.git dir/ectory/dotgit\n\nAt this point, I think there are two valid things that can happen.\n\n (1) \"git add test\" inside dir/ectory removes the submodule entry\n     \"dir/ectory\" and then adds dir/ectory/test as an individual\n     entry.  After all that is what happens when you replace a file\n     with a directory, e.g.\n\n       >folder && git add folder\n       rm folder\n       mkdir folder && >folder/file &&\n       git -C folder add file\n\n     will first register a regular file \"folder\" and then replace\n     that with paths underneath.\n\n     It would be the same if you did any of the following:\n\n       git -C . add dir/ectory/test\n       git -C dir add ectory/test\n       git -C dir/ectory add test\n\n (2) \"git add test\" inside \"dir/ectory\" would barf, saying\n     \"dir/ectory is supposed to be a submodule and you have to first\n     remove it\".  Again, it would be the same if you did so from any\n     directory with relative paths.\n\nI think (2) is less optimal than (1), but it could be the best we\ncould do if the submodule infrastracture around .gitmodules and\nlinks to $GIT_DIR/modules/$name may have to get involved in an\noperation like this (I didn't check).\n\nIn my quick test, it appeared that the behaviour with this patch\napplied was neither of the two and instead to silently do nothing,\nand if that is the case I think it is quite wrong.\n"},{"id":"259821","messageId":"5537F31D.4090704@web.de","threadId":"39064","inReplyTo":"xmqq8udlgpey.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-04-22T19:14:37Z","receivedAt":"2015-04-22T19:14:37Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 21.04.2015 um 23:08 schrieb Junio C Hamano:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Mon, Apr 20, 2015 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Duy Nguyen <pclouds@gmail.com> writes:\n>>>\n>>>> But if you look at it another way, \"cd subrepo; git add .\" should be\n>>>> the same as \"git add subrepo\" ...\n>>>\n>>> Once you cd into \"subrepo\", you are in a different world, a world\n>>> different from the toplevel project.  \"git add .\" over there should\n>>> mean \"add everything in subproject's working tree to subproject's\n>>> index\", shouldn't it?  On the other hand, \"git add subrepo\" without\n>>> leavingin the working tree of the superproject is about binding the\n>>> submodule to the superproject's index.\n>>>\n>>> I do not think these two should be the same.  Where am I mistaken?\n>\n>> I think I wrote this sentence and deleted it: I didn't say which way\n>> was right.\n>\n> OK.\n>\n> I looked at the test script update.  The new test does (I am\n> rephrasing to make it clearer):\n>\n>      mkdir -p dir/ectory\n>      git init dir/ectory ;# a new directory inside top-level project\n>      (\n>          cd dir/ectory &&\n>          >test && git add test && git commit -m test\n>      )\n>      git add dir/ectory\n>\n> to set it up.  At this point, the top-level index knows dir/ectory\n> is a submodule.\n>\n> Then the test goes on to turn it a non submodule by\n>\n>      mv dir/ectory/.git dir/ectory/dotgit\n>\n> At this point, I think there are two valid things that can happen.\n>\n>   (1) \"git add test\" inside dir/ectory removes the submodule entry\n>       \"dir/ectory\" and then adds dir/ectory/test as an individual\n>       entry.  After all that is what happens when you replace a file\n>       with a directory, e.g.\n>\n>         >folder && git add folder\n>         rm folder\n>         mkdir folder && >folder/file &&\n>         git -C folder add file\n>\n>       will first register a regular file \"folder\" and then replace\n>       that with paths underneath.\n>\n>       It would be the same if you did any of the following:\n>\n>         git -C . add dir/ectory/test\n>         git -C dir add ectory/test\n>         git -C dir/ectory add test\n>\n>   (2) \"git add test\" inside \"dir/ectory\" would barf, saying\n>       \"dir/ectory is supposed to be a submodule and you have to first\n>       remove it\".  Again, it would be the same if you did so from any\n>       directory with relative paths.\n>\n> I think (2) is less optimal than (1), but it could be the best we\n> could do if the submodule infrastracture around .gitmodules and\n> links to $GIT_DIR/modules/$name may have to get involved in an\n> operation like this (I didn't check).\n\nWe already do (2) in the cases you describe:\n\n    $ git add subrepo/a\n    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n    $ git -C subrepo add a\n    fatal: Pathspec 'a' is in submodule 'subrepo'\n\nAnd I believe that is better than (1), because when removing a\nsubmodule its entry in .gitmodules should also be removed. And\nI suspect that adding a file in a submodule to the superproject\nwill happen more by accident than on purpose (which cannot\nhappen for files because when you add a new file in a directory\nfor which git still records a file the latter can be safely\nremoved as that entry cannot be a file in the worktree anymore).\n\nSo I'd vote to have (2) also for \"git -C subrepo add .\", which\nis what started this thread.\n\n> In my quick test, it appeared that the behaviour with this patch\n> applied was neither of the two and instead to silently do nothing,\n> and if that is the case I think it is quite wrong.\n\nYep.\n"},{"id":"259837","messageId":"xmqq1tjcdjfa.fsf@gitster.dls.corp.google.com","threadId":"39064","inReplyTo":"5537F31D.4090704@web.de","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-22T19:58:33Z","receivedAt":"2015-04-22T19:58:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Am 21.04.2015 um 23:08 schrieb Junio C Hamano:\n>\n>> I looked at the test script update.  The new test does (I am\n>> rephrasing to make it clearer):\n>>\n>>      mkdir -p dir/ectory\n>>      git init dir/ectory ;# a new directory inside top-level project\n>>      (\n>>          cd dir/ectory &&\n>>          >test && git add test && git commit -m test\n>>      )\n>>      git add dir/ectory\n>>\n>> to set it up.  At this point, the top-level index knows dir/ectory\n>> is a submodule.\n>>\n>> Then the test goes on to turn it a non submodule by\n>>\n>>      mv dir/ectory/.git dir/ectory/dotgit\n>> ...\n>\n> We already do (2) in the cases you describe:\n>\n>    $ git add subrepo/a\n>    fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n>    $ git -C subrepo add a\n>    fatal: Pathspec 'a' is in submodule 'subrepo'\n> ...\n> So I'd vote to have (2) also for \"git -C subrepo add .\", which\n> is what started this thread.\n\nDoes having \"mv subrepo/.git subrepo/dotgit\" before that \"git add\"\nchange your conclusion?\n\nIt is very clear to me that without that \"mv\" step, (2) is\nabsolutely the right thing to do, and I agree with you.\n\nBut it is unclear if we should still do (2) when \"subrepo/.git\" is\nno longer there.  That has to be done manually and it may be an\nindication that is clear enough that the end user wants the\ndirectory to be a normal directory without any submodule involved,\nin which case it may match the expectation of the user better to\njust nuke the corresponding 160000 entry in the index and replace it\nwith files in there.  I dunno.\n\n>> In my quick test, it appeared that the behaviour with this patch\n>> applied was neither of the two and instead to silently do nothing,\n>> and if that is the case I think it is quite wrong.\n>\n> Yep.\n"},{"id":"259857","messageId":"5538218B.4010400@web.de","threadId":"39064","inReplyTo":"xmqq1tjcdjfa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-04-22T22:32:43Z","receivedAt":"2015-04-22T22:32:43Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 22.04.2015 um 21:58 schrieb Junio C Hamano:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>\n>> Am 21.04.2015 um 23:08 schrieb Junio C Hamano:\n>>\n>>> I looked at the test script update.  The new test does (I am\n>>> rephrasing to make it clearer):\n>>>\n>>>       mkdir -p dir/ectory\n>>>       git init dir/ectory ;# a new directory inside top-level project\n>>>       (\n>>>           cd dir/ectory &&\n>>>           >test && git add test && git commit -m test\n>>>       )\n>>>       git add dir/ectory\n>>>\n>>> to set it up.  At this point, the top-level index knows dir/ectory\n>>> is a submodule.\n>>>\n>>> Then the test goes on to turn it a non submodule by\n>>>\n>>>       mv dir/ectory/.git dir/ectory/dotgit\n>>> ...\n>>\n>> We already do (2) in the cases you describe:\n>>\n>>     $ git add subrepo/a\n>>     fatal: Pathspec 'subrepo/a' is in submodule 'subrepo'\n>>     $ git -C subrepo add a\n>>     fatal: Pathspec 'a' is in submodule 'subrepo'\n>> ...\n>> So I'd vote to have (2) also for \"git -C subrepo add .\", which\n>> is what started this thread.\n>\n> Does having \"mv subrepo/.git subrepo/dotgit\" before that \"git add\"\n> change your conclusion?\n>\n> It is very clear to me that without that \"mv\" step, (2) is\n> absolutely the right thing to do, and I agree with you.\n\nHuh? Without that mv all files would simply be added to the\nsubmodule repo and this would be a non-issue ... what am I\nmissing? I'm just advocating to let \"git add .\" in a submodule\nwithout a .git behave like \"git add <file>\" already does.\n\n> But it is unclear if we should still do (2) when \"subrepo/.git\" is\n> no longer there.  That has to be done manually and it may be an\n> indication that is clear enough that the end user wants the\n> directory to be a normal directory without any submodule involved,\n> in which case it may match the expectation of the user better to\n> just nuke the corresponding 160000 entry in the index and replace it\n> with files in there.  I dunno.\n\nThe user having removed subrepo/.git is just one reason for that.\nAnother is a user adding a file in an unpopulated work tree of a\nnot initialized submodule. I doubt that simply nuking the 160000\nentry would be the right thing to do in this case, I expect this\nto be a pilot error we should barf about ;-)\n"},{"id":"259881","messageId":"CAPc5daUA30ZbbJVK9S_FM9Z8qyyEXUez9hGD2JNhTm78QYb5YQ@mail.gmail.com","threadId":"39064","inReplyTo":"5538218B.4010400@web.de","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-23T03:47:16Z","receivedAt":"2015-04-23T03:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Apr 22, 2015 at 3:32 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> ...\n>> But it is unclear if we should still do (2) when \"subrepo/.git\" is\n>> no longer there.  That has to be done manually and it may be an\n>> indication that is clear enough that the end user wants the\n>> directory to be a normal directory without any submodule involved,\n>> in which case it may match the expectation of the user better to\n>> just nuke the corresponding 160000 entry in the index and replace it\n>> with files in there.  I dunno.\n>\n> The user having removed subrepo/.git is just one reason for that.\n> Another is a user adding a file in an unpopulated work tree of a\n> not initialized submodule. I doubt that simply nuking the 160000\n> entry would be the right thing to do in this case, I expect this\n> to be a pilot error we should barf about ;-)\n\nOK, that sounds sensible.\n"},{"id":"263814","messageId":"CACsJy8A5ikjGmj+ysTUt9FFi9X0WzVXvnFSVbmFoe8rzHdNNoA@mail.gmail.com","threadId":"39064","inReplyTo":"CAPc5daUA30ZbbJVK9S_FM9Z8qyyEXUez9hGD2JNhTm78QYb5YQ@mail.gmail.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-14T13:16:46Z","receivedAt":"2015-06-14T13:16:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 23, 2015 at 10:47 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Wed, Apr 22, 2015 at 3:32 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n>> ...\n>>> But it is unclear if we should still do (2) when \"subrepo/.git\" is\n>>> no longer there.  That has to be done manually and it may be an\n>>> indication that is clear enough that the end user wants the\n>>> directory to be a normal directory without any submodule involved,\n>>> in which case it may match the expectation of the user better to\n>>> just nuke the corresponding 160000 entry in the index and replace it\n>>> with files in there.  I dunno.\n>>\n>> The user having removed subrepo/.git is just one reason for that.\n>> Another is a user adding a file in an unpopulated work tree of a\n>> not initialized submodule. I doubt that simply nuking the 160000\n>> entry would be the right thing to do in this case, I expect this\n>> to be a pilot error we should barf about ;-)\n>\n> OK, that sounds sensible.\n\nThere are more to this \"submodule vs the world\". When .git is gone, if\nyou expect to warn instead of deleting 160000. You may want the same\nfor the opposite direction: when b/c is in the index and you add\nb/.git, you may want \"git add b\" to complain too (right not it ignores\nb/.git and updates b/c).\n\nWe may need more surgery around this area. It seems to be the \"Path %s\nis in submodule\" check (in pathspec.c) is at the wrong place. We have\nd/f check in add_index_entry_with_check. That should catch it, at the\ncore, not from call sites like add or update-index. But that check is\nnot active...\n\nThe first version of \"path is in submodule\" is in 2ce53f9 (git add: do\nnot add files from a submodule - 2009-01-02). Back then dir.c does not\nintervene. If you have \"b\" as 160000 in the index and tries to add\nb/c, the ok_to_replace feature kicks in and kills 'b'. This is why d/f\ncheck is turned off. This ok_to_replace is to deal with symlinks, see\n192268c (Add git-update-cache --replace option. - 2005-05-07).\n\nI think we should stop the ok-to-replace feature when submodules are\ninvolved, we consider submdules much more valuable than symlinks. If\nwe do this, I think we can delete those \"Path is in submodule\" trick\nbecaue the index core can handle it well.\n-- \nDuy\n"},{"id":"263828","messageId":"xmqq4mmam0sq.fsf@gitster.dls.corp.google.com","threadId":"39064","inReplyTo":"CACsJy8A5ikjGmj+ysTUt9FFi9X0WzVXvnFSVbmFoe8rzHdNNoA@mail.gmail.com","subject":"Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-14T21:34:45Z","receivedAt":"2015-06-14T21:34:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I think we should stop the ok-to-replace feature when submodules are\n> involved, we consider submdules much more valuable than symlinks.\n\nHmm, I am not sure \"valuable\" is a good criterion to decide what\nshould happen, though.\n\nThe push to use \".git\" that is a file pointing at a repository that\nis safely stored away is a move to make valuable submodule more\neasily removable from the working tree without losing information,\nso that we can remove it from the working tree when the user\ninstructs us to, just like we can remove a symlink safely without\nlosing information.  The only thing we need to be careful is that\nthat the path that corresponds to the index entry is not \"dirty\".\nThat is, for a symlink, if you make it point at something different\nwithout doing \"git add\" it, you would lose that working-tree-only\nchange when we \"kill\" that symbolic link in order to replace it with\na regular directory.  For a submodule, if you have uncommitted\nchanges in the submodule working tree, you would lose that the same\nway when we \"kill\" that submodule in order to replace that directory\nas part of the superproject's working tree.\n\nThere may need some more safety implemented (i.e. how we detect the\n\"dirty\"-ness and when we stop the operation based on that), but I\nwould imagine there is nothing fundamentally special about submodule\nthat does not apply to a symlink or a normal blob.\n"}]}