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

[PATCH] 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.

For now just improve the user visible error 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

Signed-off-by: Stefan Beller <sbeller@google.com>
---

If you were following the mailing list closely today, you may sense
that I am cleaning up stalled branches. :)

I think such a hot fix is warranted given how often we had reports
on the mailing list.

Thanks,
Stefan

 pathspec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..d522f43331 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,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 (_("Path leads inside submodule '%s', but the submodule "
+		       "was not recognized, i.e. not initialized or deleted"),
+		       ce->name);
 	return magic;
 }
 
-- 
2.11.0.196.gee862f456e.dirty

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

[Jeff King]

On Tue, Dec 27, 2016 at 04:05:59PM -0800, Stefan Beller wrote:

> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1].

If people are running into it, it definitely should not be an assert,
nor a die("BUG"). It should be a regular die(), which your patch does.
So this is definitely a good step, even if the ultimate goal may be to
handle the case more gracefully (I say that without having even read the
background, or knowing what the right handling would be).

But...

> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..d522f43331 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,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 (_("Path leads inside submodule '%s', but the submodule "
> +		       "was not recognized, i.e. not initialized or deleted"),
> +		       ce->name);

Don't you need to flip the logic here? An assert() triggers when the
condition is not true, but an "if" does the opposite. So "assert(X)"
should always become "if (!X) die(...)".

-Peff

[PATCHv2] 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.

For now just improve the user visible error 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

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Peff wrote:
> Don't you need to flip the logic here? An assert() triggers when the
> condition is not true, but an "if" does the opposite. So "assert(X)"
> should always become "if (!X) die(...)".

Duh! and it should compile as well. 

Thanks,
Stefan

 pathspec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..4724d522f2 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,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 (_("Path leads inside submodule '%s', but the submodule "
+		       "was not recognized, i.e. not initialized or deleted"),
+		       item->original);
 	return magic;
 }
 
-- 
2.11.0.196.gee862f456e.dirty

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

[Brandon Williams]

On 12/28, 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.
> 
> For now just improve the user visible error 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
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Peff wrote:
> > Don't you need to flip the logic here? An assert() triggers when the
> > condition is not true, but an "if" does the opposite. So "assert(X)"
> > should always become "if (!X) die(...)".
> 
> Duh! and it should compile as well. 
> 
> Thanks,
> Stefan
> 
>  pathspec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..4724d522f2 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,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 (_("Path leads inside submodule '%s', but the submodule "
> +		       "was not recognized, i.e. not initialized or deleted"),
> +		       item->original);
>  	return magic;
>  }

Turns out I should comment on the most recent version of the patch :P
This looks better to me. (It resolves the issue with using a variable
not in scope).

-- 
Brandon Williams

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

[Brandon Williams]

On 12/27, 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.
> 
> For now just improve the user visible error 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
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> If you were following the mailing list closely today, you may sense
> that I am cleaning up stalled branches. :)
> 
> I think such a hot fix is warranted given how often we had reports
> on the mailing list.
> 
> Thanks,
> Stefan
> 
>  pathspec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..d522f43331 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,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 (_("Path leads inside submodule '%s', but the submodule "
> +		       "was not recognized, i.e. not initialized or deleted"),
> +		       ce->name);
>  	return magic;

I haven't been following everything on the list these past couple days,
but are we sure this is caused by submodules?  Also variable 'ce'
shouldn't be in scope here.

-- 
Brandon Williams

[PATCH] 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.

For now just improve the user visible error 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

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 * a more defensive check and message
 * with tests!
 
 pathspec.c                       | 19 +++++++++++++++++--
 t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..b446d79615 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,23 @@ 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 ce_len = ce_namelen(ce);
+			int len = ce_len < item->len ? ce_len : item->len;
+			if (!S_ISGITLINK(ce->ce_mode))
+				continue;
+			if (!memcmp(ce->name, item->match, len))
+				die (_("Pathspec '%s' is in submodule '%.*s'"),
+					item->original, ce_len, 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..e62dbb7327
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+	test_commit 1 &&
+	git submodule add ./ 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.259.ga95e92af08.dirty

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.
>
> For now just improve the user visible error message.

Thans. judging from the date: header I take this is meant as v3 that
supersedes v2 done on Wednesday.

It is not clear in the above that what this thing is.  Given that we
are de-asserting it, is the early part of the new code diagnosing an
end-user error (i.e. you gave me a pathspec but that extends into a
submodule which is a no-no)?  The "was a submodule issue" comment
added is "this is an end-user mistake, there is nothing to fix in
the code"?

I take that the new "BUG" thing tells the Git developers that no
sane codepath should throw an pathspec_item that satisfies the
condition of the if() statement for non-submodules?

> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..b446d79615 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,23 @@ 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 ce_len = ce_namelen(ce);
> +			int len = ce_len < item->len ? ce_len : item->len;
> +			if (!S_ISGITLINK(ce->ce_mode))
> +				continue;

Computation of ce_len and len are better done after this check, no?

> +			if (!memcmp(ce->name, item->match, len))
> +				die (_("Pathspec '%s' is in submodule '%.*s'"),
> +					item->original, ce_len, 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..e62dbb7327
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> +	test_commit 1 &&
> +	git submodule add ./ sub &&

Is this adding our own project as its submodule?  

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.

Better yet create a separate repository, have a commit there, and
then pull it in with "git submodule add && git submodule init" into
our repository.  That would be the normal way to borrow somebody
else's project as a part of your own superproject.

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

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

[Stefan Beller]

On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>>
>> For now just improve the user visible error message.
>
> Thans. judging from the date: header I take this is meant as v3 that
> supersedes v2 done on Wednesday.

Yes, that is correct. Sorry for being sloppy not numbering the
patches correctly.

>
> It is not clear in the above that what this thing is.  Given that we
> are de-asserting it, is the early part of the new code diagnosing an
> end-user error (i.e. you gave me a pathspec but that extends into a
> submodule which is a no-no)?  The "was a submodule issue" comment
> added is "this is an end-user mistake, there is nothing to fix in
> the code"?

This is not a fix in the code, but purely improving an error message.
So far anytime someone run into this assert, it was related to submodules.
I do not know the pathspec code well enough to claim this condition
can be produced via submodules *only*, though.

So I proposed a more defensive patch, which diagnoses if it is the
"no-no, pathspec extends into a submodule" first and then throws
a generic error afterwards in case it is not the submodule issue.

> I take that the new "BUG" thing tells the Git developers that no
> sane codepath should throw an pathspec_item that satisfies the
> condition of the if() statement for non-submodules?

If we want to keep the semantics of the assert around, then we
have to have a blank statement if the submodule error message
is not triggered.

I assume if we print this BUG, then there is an actual bug.

>
>> diff --git a/pathspec.c b/pathspec.c
>> index 22ca74a126..b446d79615 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -313,8 +313,23 @@ 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 ce_len = ce_namelen(ce);
>> +                     int len = ce_len < item->len ? ce_len : item->len;
>> +                     if (!S_ISGITLINK(ce->ce_mode))
>> +                             continue;
>
> Computation of ce_len and len are better done after this check, no?

Yes, though I trusted the modern-day-compilers to get it right. Will
fix in a reroll.

>> +test_expect_success 'setup a submodule' '
>> +     test_commit 1 &&
>> +     git submodule add ./ sub &&
>
> Is this adding our own project as its submodule?

Yes it is.

>
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.

I agree that this is not a good idea.

>  No sane project does that in real life, doesn't it?

If such a project was cloned with submodules, it would recurse endlessly. :)

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

I wonder if we want to have a helper function in test-lib.sh to be used
for that. This use case (have a repository and a submodule) happens in
a lot of tests, so we could make life easier by providing a function
in the library so it is even easier than this HACK.

> Better yet create a separate repository, have a commit there, and
> then pull it in with "git submodule add && git submodule init" into
> our repository.  That would be the normal way to borrow somebody
> else's project as a part of your own superproject.

The library function could do that, yes.

Thanks,
Stefan

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

[Jeff King]

On Tue, Jan 03, 2017 at 10:15:38AM -0800, Stefan Beller wrote:

> > I take that the new "BUG" thing tells the Git developers that no
> > sane codepath should throw an pathspec_item that satisfies the
> > condition of the if() statement for non-submodules?
> 
> If we want to keep the semantics of the assert around, then we
> have to have a blank statement if the submodule error message
> is not triggered.
> 
> I assume if we print this BUG, then there is an actual bug.

Right. I think this isn't a new "BUG", but rather a loosening of an
existing one. IOW, two things are going on here:

  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.

If I understand the submodule case correctly, then (2) is reasonable.
The user gave a bogus pathspec that crosses the submodule boundary.

I've no idea if there are other cases that could ever hit the remaining
BUG, but it seems like a good defensive measure to leave it in. If
somebody hits it, then we can investigate their case.

-Peff