[PATCHv4 0/2] pathspec: give better message for submodule related pathspec error

[PATCHv4 0/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.  No sane project does that in real life, doesn't it?

> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule.  That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.

This comes as an extra patch before the actual fix.

The actual fixing patch was reworded borrowing some words from Jeff.

As this makes use of "test_commit -C", it goes on top of sb/submodule-embed-gitdir

Thanks,
Stefan


Stefan Beller (2):
  submodule tests: don't use itself as a submodule
  pathspec: give better message for submodule related pathspec error

 pathspec.c                       | 24 ++++++++++++++++++++++--
 t/lib-submodule-update.sh        |  2 ++
 t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 t/t7001-mv.sh                    |  5 +++--
 t/t7507-commit-verbose.sh        |  4 +++-
 t/t7800-difftool.sh              |  4 +++-
 t/test-lib-functions.sh          | 16 ++++++++++++++++
 7 files changed, 82 insertions(+), 6 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

-- 
2.11.0.rc2.31.g2cc886f

[PATCH 1/2] submodule tests: don't use itself as a submodule

[Stefan Beller]

In reality nobody would run "git submodule add ./. <submodule-path>"
to add the repository to itself as a submodule as this comes with some
nasty surprises, such as infinite recursion when cloning that repository.
However we do this all the time in the test suite, because most of the
time this was the most convenient way to test a very specific thing
for submodule behavior.

This provides an easier way to have submodules in tests, by just setting
TEST_CREATE_SUBMODULE to a non empty string, similar to
TEST_NO_CREATE_REPO.

Make use of it in those tests that add a submodule from ./. except for
the occurrence in create_lib_submodule_repo as there it seems we craft
a repository deliberately for both inside as well as outside use.

The name "pretzel.[non]bare" was chosen deliberate to not introduce
more strings to the test suite containing "sub[module]" as searching for
"sub" already yields a lot of hits from different contexts. "pretzel"
doesn't occur in the test suite yet, so it is a good candidate for
a potential remote for a submodule.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh |  2 ++
 t/t7001-mv.sh             |  5 +++--
 t/t7507-commit-verbose.sh |  4 +++-
 t/t7800-difftool.sh       |  4 +++-
 t/test-lib-functions.sh   | 16 ++++++++++++++++
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..58d76d9df8 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -44,6 +44,8 @@ create_lib_submodule_repo () {
 		git branch "no_submodule" &&
 
 		git checkout -b "add_sub1" &&
+		# Adding the repo itself as a submodule is a hack.
+		# Do not imitate this.
 		git submodule add ./. sub1 &&
 		git config -f .gitmodules submodule.sub1.ignore all &&
 		git config submodule.sub1.ignore all &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index e365d1ff77..6cb32f3a3a 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='git mv in subdirs'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 test_expect_success \
@@ -288,12 +289,12 @@ rm -f moved symlink
 test_expect_success 'setup submodule' '
 	git commit -m initial &&
 	git reset --hard &&
-	git submodule add ./. sub &&
+	git submodule add ./pretzel.bare sub &&
 	echo content >file &&
 	git add file &&
 	git commit -m "added sub and file" &&
 	mkdir -p deep/directory/hierarchy &&
-	git submodule add ./. deep/directory/hierarchy/sub &&
+	git submodule add ./pretzel.bare deep/directory/hierarchy/sub &&
 	git commit -m "added another submodule" &&
 	git branch submodule
 '
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index ed2653d46f..d269900afa 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='verbose commit template'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 write_script "check-for-diff" <<\EOF &&
@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '
 
 test_expect_success 'submodule log is stripped out too with -v' '
 	git config diff.submodule log &&
-	git submodule add ./. sub &&
+	git submodule add ./pretzel.bare sub &&
 	git commit -m "sub added" &&
 	(
 		cd sub &&
 		echo "more" >>file &&
+		git add file &&
 		git commit -a -m "submodule commit"
 	) &&
 	(
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..d13a5d0453 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -7,6 +7,7 @@ test_description='git-difftool
 
 Testing basic diff tool invocation
 '
+TEST_CREATE_SUBMODULE=Yes
 
 . ./test-lib.sh
 
@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
 '
 
 test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
-	git submodule add ./. submod/ule &&
+	git submodule add ./pretzel.bare submod/ule &&
+	test_commit -C submod/ule second_commit &&
 	test_config -C submod/ule diff.tool checktrees &&
 	test_config -C submod/ule difftool.checktrees.cmd '\''
 		test -d "$LOCAL" && test -d "$REMOTE" && echo good
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 579e812506..aa327a7dff 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -800,6 +800,22 @@ test_create_repo () {
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit
+	if test -n "$TEST_CREATE_SUBMODULE"
+	then
+		(
+			cd "$repo"
+			TEST_CREATE_SUBMODULE=
+			export TEST_CREATE_SUBMODULE
+			test_create_repo "pretzel.nonbare"
+			test_commit -C "pretzel.nonbare" \
+				"create submodule" "submodule-file" \
+				"submodule-content" "submodule-tag" >&3 2>&4 ||
+				error "cannot run test_commit"
+			git clone --bare "pretzel.nonbare" \
+				  "pretzel.bare" >&3 2>&4 ||
+				  error "cannot clone into bare"
+		)
+	fi
 }
 
 # This function helps on symlink challenged file systems when it is not
-- 
2.11.0.rc2.31.g2cc886f

[PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.

This patch accomplishes two things:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
     "there was something wrong with the input" message.


[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
    https://www.spinics.net/lists/git/msg249473.html

Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 pathspec.c                       | 24 ++++++++++++++++++++++--
 t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..574a0bb158 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,28 @@ 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) {
+		/* Historically this always was a submodule issue */
+		for (i = 0; i < active_nr; i++) {
+			struct cache_entry *ce = active_cache[i];
+			int len;
+
+			if (!S_ISGITLINK(ce->ce_mode))
+				continue;
+
+			len = ce_namelen(ce);
+			if (item->len < len)
+				len = item->len;
+
+			if (!memcmp(ce->name, item->match, len))
+				die (_("Pathspec '%s' is in submodule '%.*s'"),
+					item->original, ce_namelen(ce), ce->name);
+		}
+		/* The error is a new unknown bug */
+		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+	}
+
 	return magic;
 }
 
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+	git submodule add ./pretzel.bare sub &&
+	git commit -a -m "add submodule" &&
+	git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+	echo a >sub/a &&
+	test_must_fail git add sub/a 2>actual &&
+	test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+	test_must_fail git -C sub add . 2>actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.rc2.31.g2cc886f

Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Jeff King]

On Tue, Jan 03, 2017 at 05:48:35PM -0800, Stefan Beller wrote:

> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
> 
> This patch accomplishes two things:
> 
>   1. Switch assert() to die("BUG") to give a more readable message.
> 
>   2. Take one of the cases where we hit a BUG and turn it into a normal
>      "there was something wrong with the input" message.

As this last bit is quoted from me, I won't deny that it's brilliant as
usual.

But as this commit message needs to stand on its own, rather than as part of a
larger discussion thread, it might be worth expanding "one of the cases"
here. And talking about what's happening to the other cases.

Like:

  This assertion triggered for cases where there wasn't a programming
  bug, but just bogus input. In particular, if the user asks for a
  pathspec that is inside a submodule, we shouldn't assert() or
  die("BUG"); we should tell the user their request is bogus.

  We'll retain the assertion for non-submodule cases, though. We don't
  know of any cases that would trigger this, but it _would_ be
  indicative of a programming error, and we should catch it here.

or something. Writing the first paragraph made me wonder if a better
solution, though, would be to catch and complain about this case
earlier. IOW, this _is_ a programming bug, because we're violating some
assumption of the pathspec code. And whatever is putting that item into
the pathspec list is what should be fixed.

I haven't looked closely enough to have a real opinion on that, though.

> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..574a0bb158 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,28 @@ 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) {
> +		/* Historically this always was a submodule issue */
> +		for (i = 0; i < active_nr; i++) {
> +			struct cache_entry *ce = active_cache[i];
> +			int len;

Given the discussion, this comment seems funny now. Who cares about
"historically"? It should probably be something like:

  /*
   * This case can be triggered by the user pointing us to a pathspec
   * inside a submodule, which is an input error. Detect that here
   * and complain, but fallback in the non-submodule case to a BUG,
   * as we have no idea what would trigger that.
   */

-Peff

Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:
> But as this commit message needs to stand on its own, rather than as part of a
> larger discussion thread, it might be worth expanding "one of the cases"
> here. And talking about what's happening to the other cases.
>
> Like:
>
>   This assertion triggered for cases where there wasn't a programming
>   bug, but just bogus input. In particular, if the user asks for a
>   pathspec that is inside a submodule, we shouldn't assert() or
>   die("BUG"); we should tell the user their request is bogus.
>
>   We'll retain the assertion for non-submodule cases, though. We don't
>   know of any cases that would trigger this, but it _would_ be
>   indicative of a programming error, and we should catch it here.

makes sense.

>
> or something. Writing the first paragraph made me wonder if a better
> solution, though, would be to catch and complain about this case
> earlier. IOW, this _is_ a programming bug, because we're violating some
> assumption of the pathspec code. And whatever is putting that item into
> the pathspec list is what should be fixed.
>
> I haven't looked closely enough to have a real opinion on that, though.

Well I think you get different behavior with different flags enabled, i.e.
the test provided is a cornercase (as "git add ." in the submodule should
not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
were set, in my understanding of the code, so maybe the test rather adds
a ./file/with/characters inside the submodule directory)

I think a valid long term vision would be to have

    $ git -C submodule add file
    $ echo $?
    0

to behave the same as

    $ git add submodule/file
    advice/hint: adding file inside of a submodule
    $ echo $?
    0
    $ git -c submodule.iKnowWhatIDo add submodule/anotherfile
    $ echo $?
    0

Brandon, who is refactoring the pathspec stuff currently may have
an opinion if we could catch it earlier and still have beautiful code.

Thanks,
Stefan

> Given the discussion, this comment seems funny now. Who cares about
> "historically"? It should probably be something like:
>
>   /*
>    * This case can be triggered by the user pointing us to a pathspec
>    * inside a submodule, which is an input error. Detect that here
>    * and complain, but fallback in the non-submodule case to a BUG,
>    * as we have no idea what would trigger that.
>    */

Makes sense.

>
> -Peff

Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Brandon Williams]

On 01/04, Stefan Beller wrote:
> On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:
> > But as this commit message needs to stand on its own, rather than as part of a
> > larger discussion thread, it might be worth expanding "one of the cases"
> > here. And talking about what's happening to the other cases.
> >
> > Like:
> >
> >   This assertion triggered for cases where there wasn't a programming
> >   bug, but just bogus input. In particular, if the user asks for a
> >   pathspec that is inside a submodule, we shouldn't assert() or
> >   die("BUG"); we should tell the user their request is bogus.
> >
> >   We'll retain the assertion for non-submodule cases, though. We don't
> >   know of any cases that would trigger this, but it _would_ be
> >   indicative of a programming error, and we should catch it here.
> 
> makes sense.
> 
> >
> > or something. Writing the first paragraph made me wonder if a better
> > solution, though, would be to catch and complain about this case
> > earlier. IOW, this _is_ a programming bug, because we're violating some
> > assumption of the pathspec code. And whatever is putting that item into
> > the pathspec list is what should be fixed.
> >
> > I haven't looked closely enough to have a real opinion on that, though.
> 
> Well I think you get different behavior with different flags enabled, i.e.
> the test provided is a cornercase (as "git add ." in the submodule should
> not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
> were set, in my understanding of the code, so maybe the test rather adds
> a ./file/with/characters inside the submodule directory)
> 
> I think a valid long term vision would be to have
> 
>     $ git -C submodule add file
>     $ echo $?
>     0
> 
> to behave the same as
> 
>     $ git add submodule/file
>     advice/hint: adding file inside of a submodule
>     $ echo $?
>     0
>     $ git -c submodule.iKnowWhatIDo add submodule/anotherfile
>     $ echo $?
>     0
> 
> Brandon, who is refactoring the pathspec stuff currently may have
> an opinion if we could catch it earlier and still have beautiful code.
> 
> Thanks,
> Stefan
> 
> > Given the discussion, this comment seems funny now. Who cares about
> > "historically"? It should probably be something like:
> >
> >   /*
> >    * This case can be triggered by the user pointing us to a pathspec
> >    * inside a submodule, which is an input error. Detect that here
> >    * and complain, but fallback in the non-submodule case to a BUG,
> >    * as we have no idea what would trigger that.
> >    */
> 
> Makes sense.
> 
> >
> > -Peff

So there are really two different things going on in the pathspec code
with regards to submodules.

The case that this series is trying to solve is not because the user
provided a pathspec into a submodule, but rather they are executing in
the context of the submodule with bogus state.  Typically this bogus
state has something to do with the submodule's .gitdir being blown away
(like in the last test (3/3) added in this patch).  Because the
submodule doesn't have a .gitdir, it searches backward in the directory
hierarchy for a .gitdir and it happens to find the superproject's gitdir
and uses that as its own .gitdir.  When this happens test 3/3 catches
that assert with the prefix being "sub/" and match being "sub" (since
the submodule slash was removed).  The condition doesn't trigger when
you supply a pathspec of "./b/a" assuming you have a file 'a' in
directory 'b' inside the submodule, since the prefix would still be
"sub/" while the match string would be "sub/b/a".  Coincidentally the
check that PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE does, does in fact
catch it (if using say the 'git add' command).

This leads me into the second case.  If
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set, then any pathspec which
decends into a submodule will indeed be caught and cause and error (as
was happens in test 2/3 in this patch).

So in my opinion, the assert at the end of constructing a
pathspec object probably isn't the best place for determining if the
submodule's gitdir has been destroyed and instead it has fallen back to
its parent's gitdir.  A check for something like this should happen much
sooner.

There are cases where it is advantages to be able to supply a pathspec
into a submodule without it erroring out (git grep --recurse-submodules
is one example).  So right now the current method for not allowing a
pathspec into a submodule is to pass the STRIP_SUBMODULE_SLASH_EXPENSIVE
flag when creating the pathspec object.

-- 
Brandon Williams

Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:

> As this last bit is quoted from me, I won't deny that it's brilliant as
> usual.

obviously. :)

>
> But as this commit message needs to stand on its own, rather than as part of a
> larger discussion thread, it might be worth expanding "one of the cases"
> here. And talking about what's happening to the other cases.
>
> Like:
>
>   This assertion triggered for cases where there wasn't a programming
>   bug, but just bogus input. In particular, if the user asks for a
>   pathspec that is inside a submodule, we shouldn't assert() or
>   die("BUG"); we should tell the user their request is bogus.

alt. cont'd:

We already would do that if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
is set, but we had to ask for this examination via a flag, because
it is expensive. At this point in code we know there is bogus input,
so all we would do is error out. For that case we can assume that the cost
of the expensive search is negligible compared to the users head scratching
that follows.

(This will appear in the patch I am about to send out)

[PATCHv5] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.

This patch accomplishes two things:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
     "there was something wrong with the input" message.

     This assertion triggered for cases where there wasn't a programming
     bug, but just bogus input. In particular, if the user asks for a
     pathspec that is inside a submodule, we shouldn't assert() or
     die("BUG"); we should tell the user their request is bogus.

     The only reason we did not check for it, is the expensive nature
     of such a check, so callers avoid setting the flag
     PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
     to bogus input, the expense of cpu cycles spent outweighs the user
     wondering what went wrong, so run that check unconditionally before
     dying with a more generic error message.

     In case we found out that the path points inside a submodule, but the
     caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we
     should not silently fix the pathspec to just point at the submodule,
     as that would confuse callers.

To make this happen, specifically the second part, move the check for
being inside a submodule into a function and call it either when
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the
buggy case to give a better error message.

Note: There is this one special case ("git -C submodule add .") in which
we call check_inside_submodule_expensive two times, once for checking
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path
handling the buggy user input. For this to work correctly we need to adapt
the conditions in the check for being inside the submodule to account for
the prior run to have taken effect.

[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
    https://www.spinics.net/lists/git/msg249473.html

Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is just rerolling the second patch of that "make the assert go away",
asking for opinions again.

I agree with Brandon that pathspec code is not the ideal place to
check for issues with submodules. However we should give the best error
message possible for the user, so running this diagnosis is fine by me.

Thanks,
Stefan

 pathspec.c                       | 63 +++++++++++++++++++++++++++-------------
 t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++
 2 files changed, 76 insertions(+), 20 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..41e0dac1df 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
 	strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static void check_inside_submodule_expensive(struct pathspec_item *item,
+					     char *match,
+					     const char *elt, int die_inside)
+{
+	int i;
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+
+		if (item->len < ce_len ||
+		    !(match[ce_len] == '/' || match[ce_len] == '\0') ||
+		    memcmp(ce->name, match, ce_len))
+			continue;
+
+		if (item->len != ce_len + 1 || die_inside)
+			die (_("Pathspec '%s' is in submodule '%.*s'"),
+			     elt, ce_len, ce->name);
+
+		/* strip trailing slash */
+		item->len--;
+		match[item->len] = '\0';
+		break;
+	}
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
 	}
 
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-		for (i = 0; i < active_nr; i++) {
-			struct cache_entry *ce = active_cache[i];
-			int ce_len = ce_namelen(ce);
-
-			if (!S_ISGITLINK(ce->ce_mode))
-				continue;
-
-			if (item->len <= ce_len || match[ce_len] != '/' ||
-			    memcmp(ce->name, match, ce_len))
-				continue;
-			if (item->len == ce_len + 1) {
-				/* strip trailing slash */
-				item->len--;
-				match[item->len] = '\0';
-			} else
-				die (_("Pathspec '%s' is in submodule '%.*s'"),
-				     elt, ce_len, ce->name);
-		}
+		check_inside_submodule_expensive(item, match, elt, 0);
 
 	if (magic & PATHSPEC_LITERAL)
 		item->nowildcard_len = item->len;
@@ -313,8 +324,20 @@ 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) {
+		/*
+		 * We know something is fishy and we're going to die
+		 * anyway, so we don't care about following operation
+		 * to be expensive, despite the caller not asking for
+		 * an expensive submodule check. The potential expensive
+		 * operation here reduces the users head scratching
+		 * greatly, though.
+		 */
+		check_inside_submodule_expensive(item, match, elt, 1);
+		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+	}
+
 	return magic;
 }
 
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+	git submodule add ./pretzel.bare sub &&
+	git commit -a -m "add submodule" &&
+	git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+	echo a >sub/a &&
+	test_must_fail git add sub/a 2>actual &&
+	test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+	test_must_fail git -C sub add . 2>actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.rc2.32.gdde9519.dirty

Re: [PATCHv5] pathspec: give better message for submodule related pathspec error

[Brandon Williams]

On 01/04, Stefan Beller wrote:
> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
> 
> This patch accomplishes two things:
> 
>   1. Switch assert() to die("BUG") to give a more readable message.
> 
>   2. Take one of the cases where we hit a BUG and turn it into a normal
>      "there was something wrong with the input" message.
> 
>      This assertion triggered for cases where there wasn't a programming
>      bug, but just bogus input. In particular, if the user asks for a
>      pathspec that is inside a submodule, we shouldn't assert() or
>      die("BUG"); we should tell the user their request is bogus.
> 
>      The only reason we did not check for it, is the expensive nature
>      of such a check, so callers avoid setting the flag
>      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
>      to bogus input, the expense of cpu cycles spent outweighs the user
>      wondering what went wrong, so run that check unconditionally before
>      dying with a more generic error message.
> 
>      In case we found out that the path points inside a submodule, but the
>      caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we
>      should not silently fix the pathspec to just point at the submodule,
>      as that would confuse callers.
> 
> To make this happen, specifically the second part, move the check for
> being inside a submodule into a function and call it either when
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the
> buggy case to give a better error message.
> 
> Note: There is this one special case ("git -C submodule add .") in which
> we call check_inside_submodule_expensive two times, once for checking
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path
> handling the buggy user input. For this to work correctly we need to adapt
> the conditions in the check for being inside the submodule to account for
> the prior run to have taken effect.
> 
> [1] https://www.google.com/search?q=item-%3Enowildcard_len
> [2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
>     https://www.spinics.net/lists/git/msg249473.html
> 
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This is just rerolling the second patch of that "make the assert go away",
> asking for opinions again.
> 
> I agree with Brandon that pathspec code is not the ideal place to
> check for issues with submodules. However we should give the best error
> message possible for the user, so running this diagnosis is fine by me.
> 
> Thanks,
> Stefan
> 
>  pathspec.c                       | 63 +++++++++++++++++++++++++++-------------
>  t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++
>  2 files changed, 76 insertions(+), 20 deletions(-)
>  create mode 100755 t/t6134-pathspec-in-submodule.sh
> 
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..41e0dac1df 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,
>  	strbuf_addf(sb, ",prefix:%d)", prefixlen);
>  }
>  
> +static void check_inside_submodule_expensive(struct pathspec_item *item,
> +					     char *match,
> +					     const char *elt, int die_inside)
> +{
> +	int i;
> +	for (i = 0; i < active_nr; i++) {
> +		struct cache_entry *ce = active_cache[i];
> +		int ce_len = ce_namelen(ce);
> +
> +		if (!S_ISGITLINK(ce->ce_mode))
> +			continue;
> +
> +		if (item->len < ce_len ||
> +		    !(match[ce_len] == '/' || match[ce_len] == '\0') ||
> +		    memcmp(ce->name, match, ce_len))
> +			continue;
> +
> +		if (item->len != ce_len + 1 || die_inside)
> +			die (_("Pathspec '%s' is in submodule '%.*s'"),
> +			     elt, ce_len, ce->name);
> +
> +		/* strip trailing slash */
> +		item->len--;
> +		match[item->len] = '\0';
> +		break;
> +	}
> +}
> +
>  /*
>   * Take an element of a pathspec and check for magic signatures.
>   * Append the result to the prefix. Return the magic bitmap.
> @@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
>  	}
>  
>  	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> -		for (i = 0; i < active_nr; i++) {
> -			struct cache_entry *ce = active_cache[i];
> -			int ce_len = ce_namelen(ce);
> -
> -			if (!S_ISGITLINK(ce->ce_mode))
> -				continue;
> -
> -			if (item->len <= ce_len || match[ce_len] != '/' ||
> -			    memcmp(ce->name, match, ce_len))
> -				continue;
> -			if (item->len == ce_len + 1) {
> -				/* strip trailing slash */
> -				item->len--;
> -				match[item->len] = '\0';
> -			} else
> -				die (_("Pathspec '%s' is in submodule '%.*s'"),
> -				     elt, ce_len, ce->name);
> -		}
> +		check_inside_submodule_expensive(item, match, elt, 0);
>  
>  	if (magic & PATHSPEC_LITERAL)
>  		item->nowildcard_len = item->len;
> @@ -313,8 +324,20 @@ 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) {
> +		/*
> +		 * We know something is fishy and we're going to die
> +		 * anyway, so we don't care about following operation
> +		 * to be expensive, despite the caller not asking for
> +		 * an expensive submodule check. The potential expensive
> +		 * operation here reduces the users head scratching
> +		 * greatly, though.
> +		 */
> +		check_inside_submodule_expensive(item, match, elt, 1);
> +		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
> +	}
> +
>  	return magic;
>  }
>  
> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..2900d8d06e
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +TEST_CREATE_SUBMODULE=yes
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> +	git submodule add ./pretzel.bare sub &&
> +	git commit -a -m "add submodule" &&
> +	git submodule deinit --all
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec 'sub/a' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule' '
> +	echo a >sub/a &&
> +	test_must_fail git add sub/a 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +cat <<EOF >expect
> +fatal: Pathspec '.' is in submodule 'sub'
> +EOF
> +
> +test_expect_success 'error message for path inside submodule from within submodule' '
> +	test_must_fail git -C sub add . 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done

I haven't taken a through look at this patch but I think you may want to
base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this
patch now conflict with that series.

Also I still don't really think this solves the problem of telling the
user what is wrong, which is that the submodule's gitdir is gone.

-- 
Brandon Williams

Re: [PATCHv5] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

> I haven't taken a through look at this patch but I think you may want to
> base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this
> patch now conflict with that series.

eh right, I forgot to mention this in the notes, it requires
sb/submodule-embed-gitdir as well, so I'll have to figure that out.

>
> Also I still don't really think this solves the problem of telling the
> user what is wrong, which is that the submodule's gitdir is gone.
>

The "git dir gone" is not a big deal IMHO as a deinitialized submodule
is perfectly fine (e.g. not initialized). The errors as I tested in Gerrit,
a superproject that contains submodules in plugins/* :

    : gerrit/plugins/cookbook-plugin$ git add .
    fatal: Pathspec '.' is in submodule 'plugins/cookbook-plugin'
    : gerrit/plugins/cookbook-plugin$ cd ..
    : gerrit/plugins$ git add cookbook-plugin/a
    fatal: Pathspec 'cookbook-plugin/a' is in submodule
'plugins/cookbook-plugin'
    : gerrit/plugins$ git add cookbook-plugin/.
    : gerrit/plugins$ git add cookbook-plugin/./.
    : gerrit/plugins$

I think that is perfect behavior for now, as it reliably detects
(a) the submodule being there and (b) if you are in there, no
matter if there is a .git dir or not.

The same error coming up if the submodule is initialized and valid, e.g.

    : gerrit/plugins$ git submodule update --init cookbook-plugin
    : gerrit/plugins$ git add cookbook-plugin/a/.
    fatal: Pathspec 'cookbook-plugin/a/.' is in submodule
'plugins/cookbook-plugin'

So I think this is pretty much exactly what we want for now:
* if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set
   we keep the behavior as is and do the expensive thing
* if the caller wants to use path inside of a submodule no matter
  the git dir of the submodule, then set the CHEAP flag instead
* in case of the assert (that I originally wanted to fix), we fall back to the
  EXPENSIVE thing reporting the error message that we already reported
  in such cases.

TL;DR: I was rather asking about the code being a viable;
by now I am convinced this is the correct behavior. ;)