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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Nov 9, 2010, 02:47 UTC
Message-ID: <AANLkTin7Rg19=vxO3PtcxEEXH2XeB92_VHxXPKgMbEqK@mail.gmail.com>
In-Reply-To: <7vfwvbpo9c.fsf@alter.siamese.dyndns.org>

2010/11/9 Junio C Hamano <gitster@pobox.com>:

Show 14 quoted lines

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