[PATCH 1/3] t3700: note a .gitignore matching fault

[PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyễn Thái Ngọc Duy]

.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() (or its callers) fails to handle this case. It
too eagerly decides the fate of the whole directory without looking
further in.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Haven't figured out how to cleanly fix this yet, unfortunately.

 t/t3700-add.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 7de42fa..6c3eed6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -295,4 +295,20 @@ test_expect_success C_LOCALE_OUTPUT 'git add --dry-run --ignore-missing of non-e
 	test_cmp expect.err actual.err
 '
 
+cat >expected <<EOF
+add 'test/.gitignore'
+add 'test/out/in'
+EOF
+
+test_expect_failure C_LOCALE_OUTPUT '' '
+	mkdir -p test/out &&
+	touch test/out/in test/out/out &&
+	cat >test/.gitignore <<EOF &&
+out
+!out/in
+EOF
+	git add --dry-run test >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.4.74.g639db

[PATCH 2/3] t1011: fix sparse-checkout initialization and add new file

[Nguyễn Thái Ngọc Duy]

Do not append to $GIT_DIR/info/sparse-checkout at each test, overwrite
it instead.

Also add sub/addedtoo for more complex tests later on

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index de84e35..3f9d66f 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -17,19 +17,21 @@ test_expect_success 'setup' '
 	cat >expected <<-\EOF &&
 	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
 	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/addedtoo
 	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	subsub/added
 	EOF
 	cat >expected.swt <<-\EOF &&
 	H init.t
 	H sub/added
+	H sub/addedtoo
 	H subsub/added
 	EOF
 
 	test_commit init &&
 	echo modified >>init.t &&
 	mkdir sub subsub &&
-	touch sub/added subsub/added &&
-	git add init.t sub/added subsub/added &&
+	touch sub/added sub/addedtoo subsub/added &&
+	git add init.t sub/added sub/addedtoo subsub/added &&
 	git commit -m "modified and added" &&
 	git tag top &&
 	git rm sub/added &&
@@ -83,6 +85,7 @@ test_expect_success 'match directories with trailing slash' '
 	cat >expected.swt-noinit <<-\EOF &&
 	S init.t
 	H sub/added
+	H sub/addedtoo
 	S subsub/added
 	EOF
 
@@ -95,7 +98,7 @@ test_expect_success 'match directories with trailing slash' '
 '
 
 test_expect_success 'match directories without trailing slash' '
-	echo sub >>.git/info/sparse-checkout &&
+	echo sub >.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-noinit result &&
@@ -104,7 +107,7 @@ test_expect_success 'match directories without trailing slash' '
 '
 
 test_expect_success 'match directory pattern' '
-	echo "s?b" >>.git/info/sparse-checkout &&
+	echo "s?b" >.git/info/sparse-checkout &&
 	git read-tree -m -u HEAD &&
 	git ls-files -t >result &&
 	test_cmp expected.swt-noinit result &&
@@ -116,6 +119,7 @@ test_expect_success 'checkout area changes' '
 	cat >expected.swt-nosub <<-\EOF &&
 	H init.t
 	S sub/added
+	S sub/addedtoo
 	S subsub/added
 	EOF
 
-- 
1.7.4.74.g639db

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

[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
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 < 0)
+		ret = defval;
 
-	/* excluded, clear all selected entries under this directory. */
-	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

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

[Thiago Farina]

2011/5/2 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> 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@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 */
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
>

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

[Nguyen Thai Ngoc Duy]

2011/5/3 Thiago Farina <tfransosi@gmail.com>:
>> 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?

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.

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

It's a line removal, what can I do about it?
-- 
Duy

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

[Junio C Hamano]

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?"

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

[Nguyen Thai Ngoc Duy]

On Wed, May 11, 2011 at 6:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?"

He did not reply whether my further explanation was good enough. Maybe this?

excluded_from_list() can't decide whether the entry matches given
patterns. If the current directory is matched before (ie. in defval),
then by default the entry is also matched.
-- 
Duy

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> 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.

That roughly corresponds to having

	!t
        t/t0000-basic.sh

in gitignore mechanism, right?  Generally t/ is not to be excluded, but
only t0000-basic.sh should be.  Sounds like a right thing to do.

> 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.

I am not quite sure what to do with this patch though.

You marked this as 3/3 without saying anything about where 1 and 2 are (or
if they even exist).  You hint there is a 1 that should be dropped here.

Is this a re-rolled round, and if so where are the previous ones?  Could
you make things easier to find (just saying something like [PATCH v2 3/3]
in the Subject: is good enough) next time?

[PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyễn Thái Ngọc Duy]

.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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Sorry, forgot the test name.

 t/t3700-add.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 7de42fa..c9f3a28 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -295,4 +295,20 @@ test_expect_success C_LOCALE_OUTPUT 'git add --dry-run --ignore-missing of non-e
 	test_cmp expect.err actual.err
 '
 
+cat >expected <<EOF
+add 'test/.gitignore'
+add 'test/out/in'
+EOF
+
+test_expect_failure C_LOCALE_OUTPUT 'positive/negative patterns at different dir levels' '
+	mkdir -p test/out &&
+	touch test/out/in test/out/out &&
+	cat >test/.gitignore <<EOF &&
+out
+!out/in
+EOF
+	git add --dry-run test >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.4.74.g639db

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Johannes Sixt]

Am 5/2/2011 14:55, schrieb Nguyễn Thái Ngọc Duy:
> .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.

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

-- Hannes

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyen Thai Ngoc Duy]

2011/5/2 Johannes Sixt <j.sixt@viscovery.net>:
> 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

Because it's expensive to do does not make it right not to do it. I
agree that traversing all the way down just to make the final decision
is expensive. But we could at least state that we only support looking
for .gitignore down to some level (0 as in current git), or based on
.gitignore paths in index, then stop. The point is make it
configurable with sane default. It's up to users to decide how they
want to pay.
-- 
Duy

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Junio C Hamano]

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.

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyen Thai Ngoc Duy]

On Wed, May 4, 2011 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yes. But git still silently ignores some rules in the .gitignore.
There's another case when you rather update $GIT_DIR/info/exclude than
 .gitignore in the tree (import tree for example). Same trap again.
-- 
Duy

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Junio C Hamano]

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.

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyen Thai Ngoc Duy]

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.
-- 
Duy

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyen Thai Ngoc Duy]

On Wed, May 4, 2011 at 7:41 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 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.

Maybe something like this instead of a implemantation fix?

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

-- 
Duy

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Johannes Sixt]

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.

-- Hannes

Re: [PATCH 1/3] t3700: note a .gitignore matching fault

[Nguyen Thai Ngoc Duy]

On Wed, May 4, 2011 at 1:06 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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.

Thanks. I missed that post. NOTES is fine although I prefer BUGS (or
LIMITATIONS). We may decide not to look for more .gitignore inside,
but we should respect all patterns we have got. Can you please submit
your changes in a patch?
-- 
Duy