[PATCH 3/3] builtin-add: fix exclude handling

[PATCH 3/3] builtin-add: fix exclude handling

[Junio C Hamano]

After we finish walking the working tree to find paths to add, the
prune_directory() function checks the pathspecs to find typoes in
them.  When a given pathspec did not produce any match (either in the
untracked files, or paths already in the index), there are two cases:

 - "git add no-such-file", i.e. the pathspec was misspelled; or

 - "git add ignored-pattern.o", i.e. the pathspec exactly matches but is
   ignored by the traversal.

For the former, the function immediately errored out.  The latter were
queued in the dir structure and later used to give an error message with
"use -f if you really mean it" advice.

e96980e (builtin-add: simplify (and increase accuracy of) exclude
handling, 2007-06-12) somehow lost the latter.  This adds the logic back,
but with a bit of twist, as the code after e96980e uses collect_ignored
option that does half the necessary work during the traversal.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-add.c |   23 ++++++++++++++++++++---
 dir.c         |   10 ++++------
 dir.h         |    2 ++
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index cb6e590..c24c1bf 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -63,9 +63,26 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	fill_pathspec_matches(pathspec, seen, specs);
 
 	for (i = 0; i < specs; i++) {
-		if (!seen[i] && pathspec[i][0] && !file_exists(pathspec[i]))
-			die("pathspec '%s' did not match any files",
-					pathspec[i]);
+		const char *match;
+		if (seen[i])
+			continue;
+		match = pathspec[i];
+		if (!match[0])
+			continue;
+
+		/* Existing file?  We must have ignored it */
+		if (file_exists(match)) {
+			int len = strlen(match);
+			int i;
+			for (i = 0; i < dir->ignored_nr; i++)
+				if (dir->ignored[i]->len == len &&
+				    !memcmp(dir->ignored[i]->name, match, len))
+					break;
+			if (dir->ignored_nr <= i)
+				dir_add_ignored(dir, match, strlen(match));
+			continue;
+		}
+		die("pathspec '%s' did not match any files", match);
 	}
         free(seen);
 }
diff --git a/dir.c b/dir.c
index 00d698d..11d8954 100644
--- a/dir.c
+++ b/dir.c
@@ -413,13 +413,10 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathna
 	return dir->entries[dir->nr++] = dir_entry_new(pathname, len);
 }
 
-static struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)
+void dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)
 {
-	if (!cache_name_is_other(pathname, len))
-		return NULL;
-
 	ALLOC_GROW(dir->ignored, dir->ignored_nr+1, dir->ignored_alloc);
-	return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
+	dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
 }
 
 enum exist_status {
@@ -638,7 +635,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 {
 	int exclude = excluded(dir, path, &dtype);
 	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
-	    && in_pathspec(path, *len, simplify))
+	    && in_pathspec(path, *len, simplify)
+	    && cache_name_is_other(path, *len))
 		dir_add_ignored(dir, path, *len);
 
 	/*
diff --git a/dir.h b/dir.h
index 320b6a2..9bc3337 100644
--- a/dir.h
+++ b/dir.h
@@ -96,4 +96,6 @@ extern int remove_dir_recursively(struct strbuf *path, int flag);
 /* tries to remove the path with empty directories along it, ignores ENOENT */
 extern int remove_path(const char *path);
 
+extern void dir_add_ignored(struct dir_struct *, const char *, int);
+
 #endif
-- 
1.7.0.1.241.g6604f

Re: [PATCH 3/3] builtin-add: fix exclude handling

[Jeff King]

On Mon, Mar 01, 2010 at 12:26:37AM -0800, Junio C Hamano wrote:

>  - "git add ignored-pattern.o", i.e. the pathspec exactly matches but is
>    ignored by the traversal.
> 
> For the former, the function immediately errored out.  The latter were
> queued in the dir structure and later used to give an error message with
> "use -f if you really mean it" advice.
> 
> e96980e (builtin-add: simplify (and increase accuracy of) exclude
> handling, 2007-06-12) somehow lost the latter.  This adds the logic back,
> but with a bit of twist, as the code after e96980e uses collect_ignored
> option that does half the necessary work during the traversal.

I'm not sure that logic is accurate. The "increase accuracy" part of
e96980e was about the fact that COLLECT_IGNORED _knows_ something is
ignored, but builtin-add's prune_directory has to just _guess_ about it.

And I think your new code may have similar problems.  Look at the
failing test in t0050 (the one you tweaked in patch 1/3).  Before this
patch, it silently fails to add. Which is a bug, of course, but not this
one. But after this patch, we say "Oh, CamelCase was excluded, use -f,
etc". But that's not true. We are just making a guess after the fact
about why it wasn't included.

Now I am slightly hesitant to use that as an example, because it clearly
was also broken _before_ your patch. So it may be that the behavior it
exhibits (a pathspec not showing up as used, but the file exists and was
_not_ ignored) shouldn't ever happen for other cases.  I'm not sure,
which makes me a little nervous about the change. But I would be willing
to accept "it seems to work, so let's go with it for now and see if
anybody complains" if you want to try that.

But also:

> +		/* Existing file?  We must have ignored it */
> +		if (file_exists(match)) {
> +			int len = strlen(match);
> +			int i;
> +			for (i = 0; i < dir->ignored_nr; i++)
> +				if (dir->ignored[i]->len == len &&
> +				    !memcmp(dir->ignored[i]->name, match, len))
> +					break;
> +			if (dir->ignored_nr <= i)
> +				dir_add_ignored(dir, match, strlen(match));
> +			continue;
> +		}
> +		die("pathspec '%s' did not match any files", match);

Is that memcmp right? If I have something like:

  echo subdir >.gitignore
  git add 'sub*'

shouldn't it also say "Sorry, subdir [or sub*] was ignored"? But it
doesn't actually get added to the exclude list, and we get "pathspec did
not match any files", which is not quite true.

-Peff