Re: bug: sparse config interpretation incorrectness in 2.8.0-rc2

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 17, 2016, 10:17 UTC
Message-ID: <CACsJy8A2D8CqeWZ6CFdTVXzXiWdonBE=3tWFPpe7-ZmDtNGYNw@mail.gmail.com>
In-Reply-To: <xmqq37rp35k1.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 17, 2016 at 2:51 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Mar 16, 2016 at 05:09:23PM -0700, Durham Goode wrote:
>>> Using git 2.8.0-rc2, given a repo with the following files:
>>>
>>> - one/hideme
>>> - one/donthide
>>> - two/foo
>>>
>>> A sparse config of:
>>>
>>> cat > .git/info/sparse-checkout <<EOF
>>> /*
>>> !one/hideme
>>> EOF
>>>
>>> Results in a repository that only has `one/donthide` in it.  I would
>>> expect `two/foo`to be present as well.  This worked in 2.6, and
>>> bisecting it points to d589a67eceacd1cc171bbe94906ca7c9a0edd8c5
>>> "dir.c: don't exclude whole dir prematurely" (author cc'd).
>>
>> Thank you. This should fix it. I think I understand why it goes
>> wrong. I'm going to run some more tests and post a proper patch later.
>
> I admit that I've always considered "sparse checkout" was an
> uninteresting experimental feature that I do not want to pay too
> much attention to, and the only review I did carefully myself was to
> make sure that patches around that area would not change the
> behaviour of the original code in repositories that do not use that
> feature, so please do not laugh too loudly at me if I ask the
> obvious ;-)

Good news for you is there's "sparse checkout v2" in the work, that would not rely on exclude engine and should be both faster and more elegant. That should reduce "sparse checkout v1" usage to really small cases.

Show 18 quoted lines

> So the pattern list in that info/sparse-checkout file
> shares the same logic with the gitignore mechanism, and the paths
> that would have been "ignored" if the pattern list were in the
> .gitignore file are the ones that would be included in the checkout?
>
> If so, among the three paths Durham lists:
>
>  * one/hideme matches !one/hideme the last, i.e. it would not be
>    ignored if the pattern appeared in .gitignore, hence it should not
>    be in the checkout;
>
>  * one/donthide matches * the last, i.e. would be ignored, hence it
>    should be in the checkout;
>
>  * two/foo (or really anything) matches * the last, i.e. would be
>    ignored, hence it should be in the checkout.
>
> Am I reading the bug correctly?

Yes that's how I read it too. The patterns basically say "include everything (/*) except one/hideme (!one/hideme)" so two/foo should be included.

Show 16 quoted lines

>> -- 8< --
>> diff --git a/dir.c b/dir.c
>> index 69e0be6..77f38a5 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1027,7 +1027,6 @@ static struct exclude *last_exclude_matching_from_list(const char *pathname,
>>                               exc = x;
>>                               break;
>>                       }
>> -                     continue;
>>               }
>>
>>               if (x->flags & EXC_FLAG_MUSTBEDIR) {
>> -- 8< --
>> --
>> Duy

-- 
Duy