From: Thiago Farina Date: Tue, 03 May 2011 02:14:21 GMT Subject: Re: [PATCH 3/3] sparse checkout: do not eagerly decide the fate for whole directory Message-ID: In-Reply-To: <1304340464-14829-3-git-send-email-pclouds@gmail.com> 2011/5/2 Nguyễn Thái Ngọc Duy : > 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 Maybe: can be decided? > included/excluded as same as their parent directory. > > Noticed-by: > Signed-off-by: Nguyễn Thái Ngọc Duy > --- >  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 */ 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 (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 the the ? > +        * 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 >