Minor bug with git sparse checkout code

Minor bug with git sparse checkout code

[Thomas Rinderknecht]

Hello,

First, let me begin by just saying thanks to all the git developers. We switched
from cvs to git a few months ago, and it really is working well for us now that
we have overcome the initial learning curve.

We make extensive use of the sparse checkout functionality because our
repository is large and most developers don't need to see the entire source
tree. Using sparse checkout helps reduce disk usage, and also helps maintain
reasonable performance for commands such as "git status" that tend to get bogged
down when talking to slow network storage appliances.

I found what appears to be a bug in the sparse-checkout code. I am more than
happy to file a bug report on a bug tracking website, but I couldn't find one
with a quick web search. I hope that it is acceptable to post the bug report
here. Please let me know if further information is required, or if there is a
better way to file the bug report.

Short description of the problem:
---------------------------------
We have two subdirectories with similar names, for example "libs/lib1", and
"libs/lib1a". We want to check out just "libs/lib1". The sparse-checkout file
contains only a single line "libs/lib1/", but git still checks out both
subdirectories (it appears to be doing a partial match)

Sequence for reproducing the bug:
---------------------------------
(1) create a repository with similar directory names
mkdir /tmp/git_bug
cd /tmp/git_bug
git init
mkdir a1
touch a1/base
mkdir a1-extra
touch a1-extra/more
git add .
git commit -m "test"

(2) clone the repository, but don't check out the files
mkdir /tmp/git_bug2
cd /tmp/git_bug2
git clone -n /tmp/git_bug .
git config core.sparsecheckout true
echo "a1/" .git/info/sparse-checkout
git checkout
ls
<you should see that both a1 and a1_extra are checked out>

Note: We are currently using git version 1.7.2.3 ... I have not verified the bug
using the latest developmental code. Please accept my apologies if the issue has
already been addressed.

Thanks again,

Thomas R.

Re: Minor bug with git sparse checkout code

[Thomas Rinderknecht]

Hello,

The description of the bug in my previous e-mail was correct, but the 2nd part
of steps for replicating the bug were incorrect.  Please use the following
sequences to reproduce the bug (this time I actually verified it completely...)

(1) create the reference repository (same as previous e-mail)
mkdir /tmp/git_bug
cd /tmp/git_bug
git init
mkdir a1
touch a1/base
mkdir a1-extra
touch a1-extra/more
git add .
git commit -m "test"

(2) replicate the bug
cd /tmp/git_bug2
git clone -n /tmp/git_bug .
git config core.sparsecheckout true
echo "a1/" > .git/info/sparse-checkout
git read-tree -u -m HEAD
ls

The previous example was using "git checkout" instead of "git read-tree", and
the "echo" command was missing an output redirect operator. It actually was
checking out both directories, but for the wrong reason. With this example, if
the two directories are named as shown, they both get checked out, but if you
modify step (1), and name the other repository as "b1-extra", then only the "a1"
directory gets checked out in step (2).

Sorry about the confusion..

[PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout

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

Commit c84de70 (excluded_1(): support exclude files in index -
2009-08-20) tries to work around the fact that there is no
directory/file information in index entries, therefore
EXC_FLAG_MUSTBEDIR match would fail.

Unfortunately the workaround is flawed. This fixes it.

Reported-by: Thomas Rinderknecht <thomasr@sailguy.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c                                |    3 ++-
 t/t1011-read-tree-sparse-checkout.sh |   10 +++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index d1e5e5e..b2dfb69 100644
--- a/dir.c
+++ b/dir.c
@@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,
 
 			if (x->flags & EXC_FLAG_MUSTBEDIR) {
 				if (!dtype) {
-					if (!prefixcmp(pathname, exclude))
+					if (!prefixcmp(pathname, exclude) &&
+					    pathname[x->patternlen] == '/')
 						return to_exclude;
 					else
 						continue;
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 9a07de1..8008fa2 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -17,17 +17,19 @@ test_expect_success 'setup' '
 	cat >expected <<-\EOF &&
 	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
 	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
+	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	subsub/added
 	EOF
 	cat >expected.swt <<-\EOF &&
 	H init.t
 	H sub/added
+	H subsub/added
 	EOF
 
 	test_commit init &&
 	echo modified >>init.t &&
-	mkdir sub &&
-	touch sub/added &&
-	git add init.t sub/added &&
+	mkdir sub subsub &&
+	touch sub/added subsub/added &&
+	git add init.t sub/added subsub/added &&
 	git commit -m "modified and added" &&
 	git tag top &&
 	git rm sub/added &&
@@ -81,6 +83,7 @@ test_expect_success 'match directories with trailing slash' '
 	cat >expected.swt-noinit <<-\EOF &&
 	S init.t
 	H sub/added
+	S subsub/added
 	EOF
 
 	echo sub/ > .git/info/sparse-checkout &&
@@ -105,6 +108,7 @@ test_expect_success 'checkout area changes' '
 	cat >expected.swt-nosub <<-\EOF &&
 	H init.t
 	S sub/added
+	S subsub/added
 	EOF
 
 	echo init.t >.git/info/sparse-checkout &&
-- 
1.7.3.2.210.g045198

Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout

[Thomas Rinderknecht]

Thanks for the quick fix! I was initially worried about using sparse-checkout
since it is a fairly new feature, but it has been working well for us. I am
truly impressed with the quality of the git code, and the dedication of the
developers who are enhancing and maintaining it.

Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout

[Junio C Hamano]

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

> Commit c84de70 (excluded_1(): support exclude files in index -
> 2009-08-20) tries to work around the fact that there is no
> directory/file information in index entries, therefore
> EXC_FLAG_MUSTBEDIR match would fail.
>
> Unfortunately the workaround is flawed. This fixes it.
>
> Reported-by: Thomas Rinderknecht <thomasr@sailguy.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Hmmm...

> diff --git a/dir.c b/dir.c
> index d1e5e5e..b2dfb69 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,
>  
>  			if (x->flags & EXC_FLAG_MUSTBEDIR) {
>  				if (!dtype) {
> -					if (!prefixcmp(pathname, exclude))
> +					if (!prefixcmp(pathname, exclude) &&
> +					    pathname[x->patternlen] == '/')

- Can pathname be much shorter than x->patternlen (it doesn't matter as
  prefixcmp will return false in that case)?

- Can pathname be equal to exclude (yes it can but in that case pathname
  is not a directory but is a regular file, symlink or a submodule)?

So it may be a tricky code, but I do not think it is a "workaround" in any
way.  Isn't it just a "correct solution"?

By the way, builtin/add.c calls excluded() with DT_UNKNOWN and relies on
the fact that the macro is accidentally defined as 0.  108da0d (git add:
Add the "--ignore-missing" option for the dry run, 2010-07-10).  If it
means NULL, it should spell it out as such.

Thanks.

Re: [PATCH] dir.c: fix EXC_FLAG_MUSTBEDIR match in sparse checkout

[Nguyen Thai Ngoc Duy]

2010/11/9 Junio C Hamano <gitster@pobox.com>:
>> diff --git a/dir.c b/dir.c
>> index d1e5e5e..b2dfb69 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -360,7 +360,8 @@ int excluded_from_list(const char *pathname,
>>
>>                       if (x->flags & EXC_FLAG_MUSTBEDIR) {
>>                               if (!dtype) {
>> -                                     if (!prefixcmp(pathname, exclude))
>> +                                     if (!prefixcmp(pathname, exclude) &&
>> +                                         pathname[x->patternlen] == '/')
>
> - Can pathname be much shorter than x->patternlen (it doesn't matter as
>  prefixcmp will return false in that case)?

prefixcmp will return non-zero in that case.

> - Can pathname be equal to exclude (yes it can but in that case pathname
>  is not a directory but is a regular file, symlink or a submodule)?

In index context, regular files, symlinks and submodules are the same
and should not match here even if they're equal to exclude (the
original exclude is "foo/". The trailing slash was converted to
EXC_FLAG_MUSTBEDIR).

> So it may be a tricky code, but I do not think it is a "workaround" in any
> way.  Isn't it just a "correct solution"?

Hmm.. when I added it, unpack-trees.c was the only callsite and it was
a workaround (i.e. foo/ match foo directory, but foo alone does not).

> By the way, builtin/add.c calls excluded() with DT_UNKNOWN and relies on
> the fact that the macro is accidentally defined as 0.  108da0d (git add:
> Add the "--ignore-missing" option for the dry run, 2010-07-10).  If it
> means NULL, it should spell it out as such.

It does mean NULL. Should builtin/add.c pass a proper pointer that
contains DT_UNKNOWN instead? DT_UNKNOWN has special treatment (right
after that "if").

diff --git a/builtin/add.c b/builtin/add.c
index eed37bf..b104851 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -446,7 +446,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			if (!seen[i] && pathspec[i][0]
 			    && !file_exists(pathspec[i])) {
 				if (ignore_missing) {
-					if (excluded(&dir, pathspec[i], DT_UNKNOWN))
+					int dtype = DT_UNKNOWN;
+					if (excluded(&dir, pathspec[i], &dtype))
 						dir_add_ignored(&dir, pathspec[i], strlen(pathspec[i]));
 				} else
 					die(_("pathspec '%s' did not match any files"),
-- 
Duy