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

[Brandon Williams <bmwill@google.com>]

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