Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error
From: Jeff King <hidden>
Date: 2017-01-04 07:55:13
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.
quoted hunk ↗ jump to hunk
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