On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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