Re: [PATCH 3/3] sparse checkout: do not eagerly decide the fate for whole directory

[Thiago Farina <tfransosi@xxxxxxxxx>]

2011/5/2 Nguyán ThÃi Ngác Duy <pclouds@xxxxxxxxx>:
> 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: <skillzero@xxxxxxxxx>
> Signed-off-by: Nguyán ThÃi Ngác Duy <pclouds@xxxxxxxxx>
> ---
> Â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@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj· ŠßžØn‡r¡öë¨è&£ûz¹Þúzf£¢·hšˆ§~†­†Ûÿÿïÿ‘ê_èæ+v‰¨þ)ßø