Re: [PATCH 3/2] dir.c: fix dir re-inclusion rules with "NODIR" and "MUSTBEDIR"

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 18, 2016, 05:40 UTC
Message-ID: <CACsJy8A2FKn-8nWtK4QPMHDCDYvTZBrQs1RVMApnuejXQis19g@mail.gmail.com>
In-Reply-To: <56EB8961.70302@fb.com>

On Fri, Mar 18, 2016 at 11:51 AM, Durham Goode <durham@fb.com> wrote:

Show 30 quoted lines

> On 3/17/16 4:49 PM, Junio C Hamano wrote:
>>
>> Thanks for these 5 patches, two of which need to be discarded ;-).
>> I think you can pick either one of 1/2, pick the one that says
>> "non-NULL" (as opposed to "something") in the log message for 2/2.
>>
>> Durham, does it fix your issues if you apply the 1/2 and 2/2 (but
>> not 3/2) on top of 2.8-rc?
>>
>> Duy, how comfortable are you with the idea of including this two in
>> 2.8 final?  We have long passed the final -rc, and while it is
>> probably OK to prolong the cycle and do another -rc, we cannot keep
>> going like "oops, there is another thing discovered by somebod new"
>> forever.
>>
>> Thanks.
>
> Patches 1+2 fix the repro steps in the report, yes.  But I've found another
> case that produces different results in 2.8 than in 2.7:
>
> Given a repo with files:
>
> dir1/dir2/show/file
> dir1/dir2/hide/file
>
> and a sparse-checkout of
>
> /*
> /dir1/dir2/show
> !/dir1/dir2/

I would say this is "undefined behavior" patterns. The intention of "!" pattern is to revert a subset of a matched pattern, e.g. !/dir1/dir2/show/something. Combining lines 2 and 3 together, "!dir1/dir2/" is not only supposed to revert dir1/dir2/show entirely, but extend the reversion outside of it. At least to me that's not intended.

Skipping that tricky pair, the pairs "/*" and "!/dir1/dir2/" means "exclude everything except dir1/dir2" (in .gitignore sense) or "include everything except dir1/dir2" in sparse checkout sense. Which results in empty worktree. 1+2 trips when the trailing slash in the last rule exists and includes both files in show/hide. Patch 3/2 fixes the tripping and exclude both. If the last rule is "!/dir1/dir2" then 1+2 results in empty worktree as well.

If I swap the last two rules, then it behaves the way we expect, dir1/dir2/show stays, dir1/dir2/hide is gone (again, the trailing "/" in !dir1/dir2/ requires patch 3/2).

v2.7.3 differs in the way "!" is handled. It does extend reversion outside dir1/dir2/show, back to dir1/dir2. While 2.8+1+2 recognizes and follows the "/*" and "!dir1/dir2/" pair.

The way I interpreted the rules above, though, may be because I'm just trying to defend the current code. Junio, your call on whether to revert this whole patch series.

Show 5 quoted lines

> the working copy still contains dir1/dir2/hide/file when run from 2.8.0-rc2.
> In git 2.6 and 2.7.3 it does not show up (which is the expected behavior,
> from what I understand of the docs).  Repro script is below.  Notice, the
> 'dir2/' part of the paths is important.  If I drop that directory, the issue
> doesn't repro.

-- 
Duy