patch, 3 partst3700: note a .gitignore matching fault

> Sparse-setting code follows closely how files are excluded in
> read_directory(), every entry (including directories) are fed to
> excluded_from_list() to decide if the entry is suitable. Directories
> are treated no different than files. If a directory is matched (or
> not), the whole directory is considered matched (or not) and the
> process moves on.
>
> This generally works as long as there are no patterns to exclude parts
> of the directory. In case of sparse checkout code, the following patterns
>
>  t
>  !t/t0000-basic.sh
>
> will produce a worktree with full directory "t" even if t0000-basic.sh
> is requested to stay out.
>
> By the same reasoning, if a directory is to be excluded, any rules to
> re-include certain files within that directory will be ignored.
>
> Fix it by always checking files against patterns. If no pattern can
> decide (ie. excluded_from_list() returns -1), those files will be

> included/excluded as same as their parent directory.
>
> Noticed-by: <skillzero@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  The TODO better be solved once read_directory() fixes the same fault.
>  I have a feeling that some code can be shared..
>
>  t/t1011-read-tree-sparse-checkout.sh |   41 ++++++++++++++++++++++
>  unpack-trees.c                       |   63 ++++++++++++++++++---------------
>  2 files changed, 75 insertions(+), 29 deletions(-)
>
> diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
> index 3f9d66f..20a50eb 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -106,6 +106,47 @@ test_expect_success 'match directories without trailing slash' '
>        test -f sub/added
>  '
>
> +test_expect_success 'match directories with negated patterns' '
> +       cat >expected.swt-negation <<\EOF &&
> +S init.t
> +S sub/added
> +H sub/addedtoo
> +S subsub/added
> +EOF
> +
> +       cat >.git/info/sparse-checkout <<\EOF &&
> +sub
> +!sub/added
> +EOF
> +       git read-tree -m -u HEAD &&
> +       git ls-files -t >result &&
> +       test_cmp expected.swt-negation result &&
> +       test ! -f init.t &&
> +       test ! -f sub/added &&
> +       test -f sub/addedtoo
> +'
> +
> +test_expect_success 'match directories with negated patterns (2)' '
> +       cat >expected.swt-negation2 <<\EOF &&
> +H init.t
> +H sub/added
> +S sub/addedtoo
> +H subsub/added
> +EOF
> +
> +       cat >.git/info/sparse-checkout <<\EOF &&
> +/*
> +!sub
> +sub/added
> +EOF
> +       git read-tree -m -u HEAD &&
> +       git ls-files -t >result &&
> +       test_cmp expected.swt-negation2 result &&
> +       test -f init.t &&
> +       test -f sub/added &&
> +       test ! -f sub/addedtoo
> +'
> +
>  test_expect_success 'match directory pattern' '
>        echo "s?b" >.git/info/sparse-checkout &&
>        git read-tree -m -u HEAD &&
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 500ebcf..5b8419c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -814,43 +814,45 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
>        return mask;
>  }
>
> +static int clear_ce_flags_1(struct cache_entry **cache, int nr,
> +                           char *prefix, int prefix_len,
> +                           int select_mask, int clear_mask,
> +                           struct exclude_list *el, int defval);
> +
>  /* Whole directory matching */
>  static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
>                              char *prefix, int prefix_len,
>                              char *basename,
>                              int select_mask, int clear_mask,
> -                             struct exclude_list *el)
> +                             struct exclude_list *el, int defval)
>  {
> -       struct cache_entry **cache_end = cache + nr;
> +       struct cache_entry **cache_end;
>        int dtype = DT_DIR;
>        int ret = excluded_from_list(prefix, prefix_len, basename, &dtype, el);
>
>        prefix[prefix_len++] = '/';
>
> -       /* included, no clearing for any entries under this directory */
> -       if (!ret) {
> -               for (; cache != cache_end; cache++) {
> -                       struct cache_entry *ce = *cache;
> -                       if (strncmp(ce->name, prefix, prefix_len))
> -                               break;
> -               }
> -               return nr - (cache_end - cache);
> -       }
> +       /* If undecided, use parent directory's decision in defval */

> -       if (ret == 1) {
> -               for (; cache != cache_end; cache++) {
> -                       struct cache_entry *ce = *cache;
> -                       if (select_mask && !(ce->ce_flags & select_mask))
> -                               continue;
> -                       if (strncmp(ce->name, prefix, prefix_len))
> -                               break;
> -                       ce->ce_flags &= ~clear_mask;
> -               }
> -               return nr - (cache_end - cache);
> +       for (cache_end = cache; cache_end != cache + nr; cache_end++) {
> +               struct cache_entry *ce = *cache_end;
> +               if (strncmp(ce->name, prefix, prefix_len))
> +                       break;
>        }
>
> -       return 0;
> +       /*
> +        * TODO: check el, if there are no patterns that may conflict
> +        * with ret (iow, we know in advance the the incl/excl

> +        * decision for the entire directory), clear flag here without
> +        * calling clear_ce_flags_1(). That function will call
> +        * the expensive excluded_from_list() on every entry.
> +        */
> +       return clear_ce_flags_1(cache, cache_end - cache,
> +                               prefix, prefix_len,
> +                               select_mask, clear_mask,
> +                               el, ret);
>  }
>
>  /*
> @@ -871,7 +873,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache, int nr,
>  static int clear_ce_flags_1(struct cache_entry **cache, int nr,
>                            char *prefix, int prefix_len,
>                            int select_mask, int clear_mask,
> -                           struct exclude_list *el)
> +                           struct exclude_list *el, int defval)
>  {
>        struct cache_entry **cache_end = cache + nr;
>
> @@ -882,7 +884,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
>        while(cache != cache_end) {
>                struct cache_entry *ce = *cache;
>                const char *name, *slash;
> -               int len, dtype;
> +               int len, dtype, ret;
>
>                if (select_mask && !(ce->ce_flags & select_mask)) {
>                        cache++;
> @@ -911,7 +913,7 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
>                                                       prefix, prefix_len + len,
>                                                       prefix + prefix_len,
>                                                       select_mask, clear_mask,
> -                                                      el);
> +                                                      el, defval);
>
>                        /* clear_c_f_dir eats a whole dir already? */
>                        if (processed) {
> @@ -922,13 +924,16 @@ static int clear_ce_flags_1(struct cache_entry **cache, int nr,
>                        prefix[prefix_len + len++] = '/';
>                        cache += clear_ce_flags_1(cache, cache_end - cache,
>                                                  prefix, prefix_len + len,
> -                                                 select_mask, clear_mask, el);
> +                                                 select_mask, clear_mask, el, defval);
>                        continue;
>                }
>
>                /* Non-directory */
>                dtype = ce_to_dtype(ce);
> -               if (excluded_from_list(ce->name, ce_namelen(ce), name, &dtype, el) > 0)
> +               ret = excluded_from_list(ce->name, ce_namelen(ce), name, &dtype, el);
> +               if (ret < 0)
> +                       ret = defval;
> +               if (ret > 0)
>                        ce->ce_flags &= ~clear_mask;
>                cache++;
>        }
> @@ -943,7 +948,7 @@ static int clear_ce_flags(struct cache_entry **cache, int nr,
>        return clear_ce_flags_1(cache, nr,
>                                prefix, 0,
>                                select_mask, clear_mask,
> -                               el);
> +                               el, 0);
>  }
>
>  /*
> --
> 1.7.4.74.g639db
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

>> This generally works as long as there are no patterns to exclude parts
>> of the directory. In case of sparse checkout code, the following patterns
>>
>>  t
>>  !t/t0000-basic.sh
>>
>> will produce a worktree with full directory "t" even if t0000-basic.sh
>> is requested to stay out.
>> ...
>>        prefix[prefix_len++] = '/';
>>
>> -       /* included, no clearing for any entries under this directory */
>> -       if (!ret) {
>> -               for (; cache != cache_end; cache++) {
>> -                       struct cache_entry *ce = *cache;
>> -                       if (strncmp(ce->name, prefix, prefix_len))
>> -                               break;
>> -               }
>> -               return nr - (cache_end - cache);
>> -       }
>> +       /* If undecided, use parent directory's decision in defval */
> What means "use parent directory's decision"? Could you make this
> comment more clearer?

>> +       if (ret < 0)
>> +               ret = defval;
>>
>> -       /* excluded, clear all selected entries under this directory. */
> Start with capital letter?

>>> +       /* If undecided, use parent directory's decision in defval */
>> What means "use parent directory's decision"? Could you make this
>> comment more clearer?
>
> Take the example patterns in commit message, we know that we match
> directory "t" (pattern 1). When we check t/0001-init.sh, no patterns
> match it. But because it's under "t", so we consider it matched. On
> the other hand, t/t0000-basic.sh will match pattern 2 and override
> parent directory's decision.

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>>>> +       /* If undecided, use parent directory's decision in defval */
>>> What means "use parent directory's decision"? Could you make this
>>> comment more clearer?
>>
>> Take the example patterns in commit message, we know that we match
>> directory "t" (pattern 1). When we check t/0001-init.sh, no patterns
>> match it. But because it's under "t", so we consider it matched. On
>> the other hand, t/t0000-basic.sh will match pattern 2 and override
>> parent directory's decision.
>
> Somebody ask about the comment you wrote, and you had to explain it.
> Doesn't it tell us something about the readability of the comment?
>
> After all the request was "Could you make it clearer?"

> .gitignore support both positive and negative patterns. One may negate
> the other. Current code works well if both patterns target files in
> the same directory.
> 
> When a pattern targets a directory and an opposite pattern targets
> some files/directories within that directory, we need to descend in
> the directory until we're clear which ones are matched and which are
> not.
> 
> excluded_from_list() fails to handle this case. It too eagerly decides
> the fate of the whole directory without looking further in.

> Am 5/2/2011 14:55, schrieb Nguyễn Thái Ngọc Duy:
>> excluded_from_list() fails to handle this case. It too eagerly decides
>> the fate of the whole directory without looking further in.
>
> This has been debated just recently, and I don't think the current
> behavior is broken. See
>
> http://thread.gmane.org/gmane.comp.version-control.git/169913
> http://thread.gmane.org/gmane.comp.version-control.git/157190/focus=157196

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> ... The point is make it
>> configurable with sane default. It's up to users to decide how they
>> want to pay.
>
> Hmm, I am confused.
>
> Isn't that what we already have?  If you want to pay, you just do not
> define a do-not-descend ignore entry at such a location so close to the
> root of the working tree.

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> Yes. But git still silently ignores some rules in the .gitignore.
>
> Do you want git to report each and every entry in .gitignore saying "this
> rule does not apply"?  That sounds like madness to me.

> On Wed, May 4, 2011 at 6:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>
>>> Yes. But git still silently ignores some rules in the .gitignore.
>>
>> Do you want git to report each and every entry in .gitignore saying "this
>> rule does not apply"?  That sounds like madness to me.
>
> This rule should apply, but not because of "efficiency reasons". Not
> just about any rule.

> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
> index 8416f34..81e9d43 100644
> --- a/Documentation/gitignore.txt
> +++ b/Documentation/gitignore.txt
> @@ -71,6 +71,8 @@ PATTERN FORMAT
>     matching file excluded by a previous pattern will become
>     included again.  If a negated pattern matches, this will
>     override lower precedence patterns sources.
> +   If a directory is excluded by earlier patterns, negated
> +   patterns that touch files inside the directory will be ignored.
> 
>   - If the pattern ends with a slash, it is removed for the
>     purpose of the following description, but it would only find
> 

> Am 5/4/2011 3:05, schrieb Nguyen Thai Ngoc Duy:
>> Maybe something like this instead of a implemantation fix?
>
> Yes, but...
>
>> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
>> index 8416f34..81e9d43 100644
>> --- a/Documentation/gitignore.txt
>> +++ b/Documentation/gitignore.txt
>> @@ -71,6 +71,8 @@ PATTERN FORMAT
>>     matching file excluded by a previous pattern will become
>>     included again.  If a negated pattern matches, this will
>>     override lower precedence patterns sources.
>> +   If a directory is excluded by earlier patterns, negated
>> +   patterns that touch files inside the directory will be ignored.
>>
>>   - If the pattern ends with a slash, it is removed for the
>>     purpose of the following description, but it would only find
>>
>
> ... as I already said here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/170907/focus=170916
>
> I think that this is not quite the right place to mention this
> restriction. See my proposal in the same post.

> Sparse-setting code follows closely how files are excluded in
> read_directory(), every entry (including directories) are fed to
> excluded_from_list() to decide if the entry is suitable. Directories
> are treated no different than files. If a directory is matched (or
> not), the whole directory is considered matched (or not) and the
> process moves on.
>
> This generally works as long as there are no patterns to exclude parts
> of the directory. In case of sparse checkout code, the following patterns
>
>   t
>   !t/t0000-basic.sh
>
> will produce a worktree with full directory "t" even if t0000-basic.sh
> is requested to stay out.

> Noticed-by: <skillzero@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Fix some comments as Thiago suggested. 1/3 of this series should be
>  dropped apparently.