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

From: Stefan Beller <sbeller@google.com>
Date: Jan 3, 2017, 18:15 UTC
Message-ID: <CAGZ79kZ5F46UzeNeVYQV1EKEEa6+az-Pe_jrT+wZF9X6b34CGA@mail.gmail.com>
In-Reply-To: <xmqqh95j60uh.fsf@gitster.mtv.corp.google.com>

On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

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

Show 7 quoted lines

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

Show 22 quoted lines

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

Show 5 quoted lines

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