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

From: Jeff King <peff@peff.net>
Date: Jan 4, 2017, 07:55 UTC
Message-ID: <20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net>
In-Reply-To: <20170104014835.22377-3-sbeller@google.com>

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

Show 11 quoted lines

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

Show 16 quoted lines

> 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