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

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

[Stefan Beller]

v6:
* rebased on top of origin/bw/pathspec-cleanup, resolving conflicts.
  (Additionally needs merging with origin/sb/submodule-embed-gitdir to have 
  6f94351b0, test-lib-functions.sh: teach test_commit -C <dir>)
* reworded comments and commit message
* do not reuse the strip_submodule_slash_expensive function, but have
  a dedicated die_inside_submodule_path function.

v5:
* was just resending the latest patch, which turns out to be in conflict with
  origin/bw/pathspec-cleanup

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

v3:
more defensive and with tests.


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

 pathspec.c                       | 31 ++++++++++++++++++++++---------
 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(+), 13 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

-- 
2.11.0.31.g919a8d0.dirty

[PATCHv6 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.31.g919a8d0.dirty

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Yuck.  

I find it doubtful that it is a good idea to create two submodule
repositories by merely dot-including the test-lib.sh; I find it
doubly doubtful that it is a good idea to make test_create_repo
pay attention to the special variable to implement that.

I am OK with a solution where callers that set TEST_CREATE_SUBMODULE
variable in this patch to instead have an explicit call

	test_create_repo --submodule pretzel

That would be a lot more obvious.

The primary reason why I hate the implementation in this patch is
that it is very easy for a test that says TEST_CREATE_SUBMODULE
upfront, only to get the initial test repository (which everybody
else gets) with two test submodules, to later gain a test that wants
to use a separate repository and call "test_create_repo".  It will
always get the pretzel submodules, which may or may not match what
the test writer who adds a new test needs.

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

But isn't the point of this change that use of ./. cannot be
mimicking any real-world use, hence pointless for the purpose of
really testing the components of the system?  If "we craft
deliberately for both inside and outside use" indeed _IS_ a good
thing, then perhaps use of ./. has practical real-world use---if
not, wouldn't we want to fix the scripts that include the
lib-submodule-repo helper not to test such an unrealistic layout?

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

[Stefan Beller]

On Sun, Jan 8, 2017 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> Yuck.
>
> I find it doubtful that it is a good idea to create two submodule
> repositories by merely dot-including the test-lib.sh; I find it
> doubly doubtful that it is a good idea to make test_create_repo
> pay attention to the special variable to implement that.
>
> I am OK with a solution where callers that set TEST_CREATE_SUBMODULE
> variable in this patch to instead have an explicit call
>
>         test_create_repo --submodule pretzel
>
> That would be a lot more obvious.

agreed.

>
> The primary reason why I hate the implementation in this patch is
> that it is very easy for a test that says TEST_CREATE_SUBMODULE
> upfront, only to get the initial test repository (which everybody
> else gets) with two test submodules, to later gain a test that wants
> to use a separate repository and call "test_create_repo".  It will
> always get the pretzel submodules, which may or may not match what
> the test writer who adds a new test needs.

I agree. At the time of writing the patch series, I was anticipating writing
way more submodule tests, but now I have these future tests integrated into
lib-submodule-update.sh.

>
>> 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.
>
> But isn't the point of this change that use of ./. cannot be
> mimicking any real-world use, hence pointless for the purpose of
> really testing the components of the system?  If "we craft
> deliberately for both inside and outside use" indeed _IS_ a good
> thing, then perhaps use of ./. has practical real-world use---if
> not, wouldn't we want to fix the scripts that include the
> lib-submodule-repo helper not to test such an unrealistic layout?
>

Makes sense; I tried to fix it up to avoid the ./. clone in
create_lib_submodule_repo, but the issue is there are too many
implicit assumption of these two repos, such that a faithful conversion
would just duplicate code for the submodule, (e.g. create the same
amount of commits, containing the same diffs, etc.), which then
can be argued to just slow down the test suite as the clone from ./.
is actually reducing the needed work by a factor of 2.

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

     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.

Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed

	if (item->nowildcard_len < prefixlen)
		item->nowildcard_len = prefixlen;

and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.

[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                       | 35 +++++++++++++++++++++++++++++++++--
 t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index d4efcf6662..42cd83c235 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
 	}
 }
 
+static void die_inside_submodule_path(struct pathspec_item *item)
+{
+	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 ||
+		    !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
+		    memcmp(ce->name, item->match, ce_len))
+			continue;
+
+		die(_("Pathspec '%s' is in submodule '%.*s'"),
+		    item->original, ce_len, ce->name);
+	}
+}
+
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	}
 
 	/* 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) {
+		/*
+		 * 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.
+		 */
+		die_inside_submodule_path(item);
+		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+	}
 }
 
 static int pathspec_item_cmp(const void *a_, const void *b_)
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.31.g919a8d0.dirty

Re: [PATCHv6 2/2] 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.
>
> 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.
>
> Note: There is a case (e.g. "git -C submodule add .") in which we call
> strip_submodule_slash_expensive, as git-add requests it via the flag
> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
> trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
> such that we executed
>
> 	if (item->nowildcard_len < prefixlen)
> 		item->nowildcard_len = prefixlen;
>
> and prefixlen was not adapted (e.g. it was computed from "submodule/")
> So in the die_inside_submodule_path function we also need handle paths,
> that were stripped before, i.e. are the exact submodule path. This
> is why the conditions in die_inside_submodule_path are slightly
> different than in strip_submodule_slash_expensive.
>
> [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>
> ---

For future reference, do not bury a useful fix behind unproven new
things.  The main purpose of this two-patch series is this change,
and it does not have to wait for the change to allow test_commit to
notice "-C" you have in another series.

Just write it in longhand, and when both topics graduate, send in
another patch to update "(cd <dir> && test_commit <others>)" to use
the new "test_commit -C <dir> <others>".

Thanks.

>  pathspec.c                       | 35 +++++++++++++++++++++++++++++++++--
>  t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 2 deletions(-)
>  create mode 100755 t/t6134-pathspec-in-submodule.sh
>
> diff --git a/pathspec.c b/pathspec.c
> index d4efcf6662..42cd83c235 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
>  	}
>  }
>  
> +static void die_inside_submodule_path(struct pathspec_item *item)
> +{
> +	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 ||
> +		    !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
> +		    memcmp(ce->name, item->match, ce_len))
> +			continue;
> +
> +		die(_("Pathspec '%s' is in submodule '%.*s'"),
> +		    item->original, ce_len, ce->name);
> +	}
> +}
> +
>  /*
>   * Perform the initialization of a pathspec_item based on a pathspec element.
>   */
> @@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
>  	}
>  
>  	/* 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) {
> +		/*
> +		 * 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.
> +		 */
> +		die_inside_submodule_path(item);
> +		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
> +	}
>  }
>  
>  static int pathspec_item_cmp(const void *a_, const void *b_)
> 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

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

[Stefan Beller]

On Sun, Jan 8, 2017 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> For future reference, do not bury a useful fix behind unproven new
> things.  The main purpose of this two-patch series is this change,
> and it does not have to wait for the change to allow test_commit to
> notice "-C" you have in another series.

noted.

The bug fixed here doesn't need to be fast-tracked IMO, because
of its history. (It's well documented on the web already)

I think this series can wait until the
"test_commit -C" is available.

In a reroll I'll drop the dependency then.