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

Nguyễn Thái Ngọc Duy  [off-list ref] 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 <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---

Hmmm...

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

2010/11/9 Junio C Hamano [off-list ref]:

quoted

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