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

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 10, 2016, 00:39 UTC
Message-ID: <CACsJy8CbxyJQ7HYy6GaqRhemeSz_upB6jQ7HMX0Vk+tr7bBz3w@mail.gmail.com>
In-Reply-To: <xmqqoaanilia.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 10, 2016 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Side note, it's probably a bad idea to use basename matching on a
>> negative rule (ie. no slashes in the pattern) because what the pattern
>> says is "re-include _any_ directory named 'dir'", not just the directory
>> "dir" at right below this directory.
>
> I am not sure I agree with this comment.  When we say '.depend/' in
> our .gitignore file, we do want to ignore everything in the
> directory with that name anywhere in that tree.  Perhaps '!include/'
> followed by '*' may be a good way to pick only the header files that
> are found in any directory with that name anywhere in the tree in a
> similar fashion.  It certainly is a disappointing comment if made by
> somebody who wants to make 'dir' and '!dir' behave in a more similar
> way, I'd have to say.

Hmm.. yeah personal view. I'll take this out.

Show 12 quoted lines

>> In both cases, we want to test if the pattern matches a parent
>> directory. match_dir_component() is for this purpose.
>
> I do not follow.  I would understand "In all cases, we want to
> behave as if we are testing the _full_ path against the pattern",
> though.
>
> IOW, "dir/file1" matches '*' (because 'file1' and '*' matches),
> '!dir/' (because the pattern is "everything under dir/ directory),
> and nothing else in the pattern list, and the last match
> wins---which happens to be negative ignore, hence dir/file1 is not
> ignored.

Yep, bad phrasing.

Show 11 quoted lines

>> We do want to be careful not to get back too far. Given the file
>> foo/bar/.gitignore, we should only check as far back as foo/bar because
>> this .gitignore file can never match outside that directory, which is
>> "toplevel" in the above paragraphs, to begin with.
>
> Yes.  But isn't that what exclude_stack mechanism already gives us?
> That is, when you are not looking at a path inside foo/bar/, entries
> stored in foo/bar/.gitignore will not participate the determination
> of the fate of the path.
>
> I am not sure why you have to say this.  Puzzled...

Naively speaking, in order to fix this without reorganizing a lot of code, when given path 'foo/bar/dir/file1' and the pattern '!dir/', we first still test if the full path "foo/bar/dir/file1" is a directory, then we cut "file1" out (pretend that we are in foo/bar instead of in foo/bar/dir) and test again the full path, which is just foo/bar/dir, if it's a directory. Repeat again and again. This trick does not go through the normal prep_exclude(), so patterns are not popping out when .gitignore should not be used any more.

The implementation is a bit different. Instead of going up one level, test, and go up again, I construct the new pattern "foo/bar/**/dir/**" and match it (only once) against the original full path foo/bar/dir/file1. The note was there because I would have just constructed "**/dir/**" instead and matched too much.

-- 
Duy