Adding empty directory gives bogus error message

Adding empty directory gives bogus error message

[Jonas Fonseca]

Hello,

During a talk with madduck on #git today, we stumbled upon this
confusing error message:

  $ mkdir repo
  $ cd repo/
  $ git init
  Initialized empty Git repository in .git/
  $ mkdir empty
  $ git add empty/
  The following paths are ignored by one of your .gitignore files:
  empty/ (directory)
  Use -f if you really want to add them.
  $ git add -f empty/
  fatal: unable to index file empty/

First, it misleads the user by assuming that something was added to
.gitignore or another exclude file, and the second error message is not
very helpful.

Looking at the code, I am not sure how this can be fixed. Simply
printing a warning when finding "some" empty directory might do, but it
is not the best way to help somebody new to git. Refusing to add
anything when finding an empty directory deep in some hierarchy is not a
good option either.

Git 1.4 silently did nothing, so 1.5 at least tries to be more clueful.
Anyway, I thought I would mention it and see what happens. ;)

-- 
Jonas Fonseca

Re: Adding empty directory gives bogus error message

[Jeff King]

On Sun, Jun 10, 2007 at 10:46:48PM +0200, Jonas Fonseca wrote:

> During a talk with madduck on #git today, we stumbled upon this
> confusing error message:
> 
>   $ mkdir repo
>   $ cd repo/
>   $ git init
>   Initialized empty Git repository in .git/
>   $ mkdir empty
>   $ git add empty/
>   The following paths are ignored by one of your .gitignore files:
>   empty/ (directory)
>   Use -f if you really want to add them.
>   $ git add -f empty/
>   fatal: unable to index file empty/

Urgh, that's ugly. The problem is that git-add sticks
assumed-to-be-ignored stuff back into the list of files found by
read_directory, but with some special ignored flags (which aren't used
anywhere else!). When the assumption is wrong (because the path is
_actually_ just empty), you get the bogus message, and when you try to
force it, you get an error from elsewhere in the code.

Patch series will be out momentarily.

-Peff

[PATCH 1/3] refactor dir_add_name

[Jeff King]

This is in preparation for keeping two entry lists in the
dir object.

This patch adds and uses the alloc_grow macro, which
implements the commonly used idiom of growing a dynamic
array using the alloc_nr function (not just in dir.c, but
everywhere).

We also move creation of a dir_entry to dir_entry_new.

Signed-off-by: Jeff King <peff@peff.net>
---

If we like the alloc_grow approach, there are a lot of places where we
can drop a 3-5 line conditional into a single line. I find it much more
readable, but others may disagree.

 cache.h |    6 ++++++
 dir.c   |   23 +++++++++++------------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index 5e7381e..f771519 100644
--- a/cache.h
+++ b/cache.h
@@ -224,6 +224,12 @@ extern void verify_filename(const char *prefix, const char *name);
 extern void verify_non_filename(const char *prefix, const char *name);
 
 #define alloc_nr(x) (((x)+16)*3/2)
+#define alloc_grow(x, nr, alloc) do { \
+	if(nr >= alloc) { \
+		alloc = alloc_nr(alloc); \
+		x = xrealloc((x), alloc * sizeof(*(x))); \
+	} \
+} while(0)
 
 /* Initialize and use the cache information */
 extern int read_index(struct index_state *);
diff --git a/dir.c b/dir.c
index f543f50..e810258 100644
--- a/dir.c
+++ b/dir.c
@@ -271,27 +271,26 @@ int excluded(struct dir_struct *dir, const char *pathname)
 	return 0;
 }
 
-struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)
-{
+static
+struct dir_entry *dir_entry_new(const char *pathname, int len) {
 	struct dir_entry *ent;
-
-	if (cache_name_pos(pathname, len) >= 0)
-		return NULL;
-
-	if (dir->nr == dir->alloc) {
-		int alloc = alloc_nr(dir->alloc);
-		dir->alloc = alloc;
-		dir->entries = xrealloc(dir->entries, alloc*sizeof(ent));
-	}
 	ent = xmalloc(sizeof(*ent) + len + 1);
 	ent->ignored = ent->ignored_dir = 0;
 	ent->len = len;
 	memcpy(ent->name, pathname, len);
 	ent->name[len] = 0;
-	dir->entries[dir->nr++] = ent;
 	return ent;
 }
 
+struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len)
+{
+	if (cache_name_pos(pathname, len) >= 0)
+		return NULL;
+
+	alloc_grow(dir->entries, dir->nr, dir->alloc);
+	return dir->entries[dir->nr++] = dir_entry_new(pathname, len);
+}
+
 enum exist_status {
 	index_nonexistent = 0,
 	index_directory,
-- 
1.5.2.1.958.gbaa74-dirty

Re: [PATCH 1/3] refactor dir_add_name

[Johannes Schindelin]

Hi,

On Mon, 11 Jun 2007, Jeff King wrote:

> If we like the alloc_grow approach, there are a lot of places where we 
> can drop a 3-5 line conditional into a single line. I find it much more 
> readable, but others may disagree.

I like it!

Ciao,
Dscho

Re: [PATCH 1/3] refactor dir_add_name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we like the alloc_grow approach, there are a lot of places where we
> can drop a 3-5 line conditional into a single line. I find it much more
> readable, but others may disagree.

I like the readability, but

>  cache.h |    6 ++++++
>  dir.c   |   23 +++++++++++------------
>  2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 5e7381e..f771519 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -224,6 +224,12 @@ extern void verify_filename(const char *prefix, const char *name);
>  extern void verify_non_filename(const char *prefix, const char *name);
>  
>  #define alloc_nr(x) (((x)+16)*3/2)
> +#define alloc_grow(x, nr, alloc) do { \
> +	if(nr >= alloc) { \
> +		alloc = alloc_nr(alloc); \
> +		x = xrealloc((x), alloc * sizeof(*(x))); \
> +	} \
> +} while(0)

worry a bit about macro safety.  I think the presence of an
assignment to alloc and x would make sure we would catch an
error to pass non lvalue as 'alloc' and 'x', so it may be Ok as
is.  A comment before the macro, and a space between 'if' and
opening parenthesis, would be good things to have.

Re: [PATCH 1/3] refactor dir_add_name

[Jeff King]

On Mon, Jun 11, 2007 at 09:23:05AM -0700, Junio C Hamano wrote:

> > +#define alloc_grow(x, nr, alloc) do { \
> > +	if(nr >= alloc) { \
> > +		alloc = alloc_nr(alloc); \
> > +		x = xrealloc((x), alloc * sizeof(*(x))); \
> > +	} \
> > +} while(0)
> 
> worry a bit about macro safety.  I think the presence of an

Agreed. The multiple evaluation of 'x' means that growing foo[i++] will
cause quite a confusing bug. I would prefer it as an inline, but at
least the sizeof requires macro magic. Not to mention the ugliness of
moving all those lvalues as pointers. But we could do something like
(totally untested):

#define alloc_grow(x, nr, alloc) \
  alloc_grow_helper(&(x), nr, &(alloc), sizeof(*(x)))

inline
void alloc_grow_helper(void **x, int nr, int *alloc, int size)
{
  if (nr >= *alloc) {
    *alloc = alloc_nr(*alloc);
    *x = xrealloc(*x, *alloc * size);
  }
}

Horribly ugly (I'm seeing stars!) but probably a bit safer in the long
run, and nobody needs to look at it most of the time. :)

What do you think?

> assignment to alloc and x would make sure we would catch an
> error to pass non lvalue as 'alloc' and 'x', so it may be Ok as

Even lvalues can have side effects, so multiple evaluation is still a
problem.

> is.  A comment before the macro, and a space between 'if' and
> opening parenthesis, would be good things to have.

Yes, sorry, my fingers are always forgetting the git whitespace
conventions. I (or Jonas) will submit a better commented version, but do
let us know which version you like.

-Peff

Re: [PATCH 1/3] refactor dir_add_name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... But we could do something like
> (totally untested):
>
> #define alloc_grow(x, nr, alloc) \
>   alloc_grow_helper(&(x), nr, &(alloc), sizeof(*(x)))
>
> inline
> void alloc_grow_helper(void **x, int nr, int *alloc, int size)
> {
>   if (nr >= *alloc) {
>     *alloc = alloc_nr(*alloc);
>     *x = xrealloc(*x, *alloc * size);
>   }
> }
>
> Horribly ugly (I'm seeing stars!) but probably a bit safer in the long
> run, and nobody needs to look at it most of the time. :)
>
> What do you think?

That looks ugly and also I am curious what the generated
assembly would look like.  Hopefully the compiler is clever
enough to generate the same code, but I dunno.

Unless somebody else more versed with C preprocessor tricks
comes along and offers a better advice, I would go with the
earlier simpler one with a big fat warning.  I however would
prefer all caps name for a magic macro like this, whose sole
point is a huge side effect.

Anyway, it appears that Jonas picked up your patch to polish up,
so I won't touch this series until that resurfaces.

Re: [PATCH 1/3] refactor dir_add_name

[Jeff King]

On Tue, Jun 12, 2007 at 12:13:01AM -0700, Junio C Hamano wrote:

> That looks ugly and also I am curious what the generated
> assembly would look like.  Hopefully the compiler is clever
> enough to generate the same code, but I dunno.

I was curious, too...the assembly generated by gcc -O2 is identical for
both versions.

> Unless somebody else more versed with C preprocessor tricks
> comes along and offers a better advice, I would go with the
> earlier simpler one with a big fat warning.  I however would
> prefer all caps name for a magic macro like this, whose sole
> point is a huge side effect.

Agreed on the all-caps (in either case) because of the side effects.
The more I think about it, I think the inline'd version is better.
Multiple evaluation pre-processor bugs are _nasty_ to find, and while
the implementation is ugly, it's better to contain the ugliness to one
spot than to introduce a dangerous interface that will be used all over.

I'm a bit rusty on my preprocessor tricks, but ISTR that there really
isn't a good way to portably avoid the problems. gcc has typeof, which
we could use to make temporary copies (which gcc would presumably
optimize out), but I imagine we don't want to be gcc-specific. We could
conditionally use that construct, but maybe at that point we're getting
as ugly as the inline).

-Peff

Re: [PATCH 1/3] refactor dir_add_name

[Jonas Fonseca]

Junio C Hamano <gitster@pobox.com> wrote Tue, Jun 12, 2007:
> Anyway, it appears that Jonas picked up your patch to polish up,
> so I won't touch this series until that resurfaces.

My idea was to put the nr, alloc, and entries members into a separate
struct dir_entry_list, which should allow the dir_add_* functions to be
"united" and thus somewhat remove the need for alloc_grow. However,
after spending some time on it, changing dir.nr to dir.entry_list.nr
etc. reduced readability so much that I ended up trying to make other
cleanups and the simplicity of the series was lost.

Anyway, I don't think I can come up with a more polished version without
adding a few cleanup patches along the way. So I think apart from the
amended version of 3/3 I will take back my intension to polish it up.

-- 
Jonas Fonseca

[PATCH 2/3] dir_struct: add collect_ignored option

[Jeff King]

When set, this option will cause read_directory to keep
track of which entries were ignored. While this shouldn't
effect functionality in most cases, it can make warning
messages to the user much more useful.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c |   12 ++++++++++++
 dir.h |    5 ++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/dir.c b/dir.c
index e810258..1ffc1e5 100644
--- a/dir.c
+++ b/dir.c
@@ -291,6 +291,15 @@ struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int
 	return dir->entries[dir->nr++] = dir_entry_new(pathname, len);
 }
 
+struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len)
+{
+	if(cache_name_pos(pathname, len) >= 0)
+		return NULL;
+
+	alloc_grow(dir->ignored, dir->ignored_nr, dir->ignored_alloc);
+	return dir->ignored[dir->ignored_nr++] = dir_entry_new(pathname, len);
+}
+
 enum exist_status {
 	index_nonexistent = 0,
 	index_directory,
@@ -463,6 +472,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 				continue;
 
 			exclude = excluded(dir, fullname);
+			if (exclude && dir->collect_ignored)
+				dir_add_ignored(dir, fullname, baselen + len);
 			if (exclude != dir->show_ignored) {
 				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
 					continue;
@@ -609,6 +620,7 @@ int read_directory(struct dir_struct *dir, const char *path, const char *base, i
 	read_directory_recursive(dir, path, base, baselen, 0, simplify);
 	free_simplify(simplify);
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
+	qsort(dir->ignored, dir->ignored_nr, sizeof(*dir->ignored), cmp_name);
 	return dir->nr;
 }
 
diff --git a/dir.h b/dir.h
index 172147f..c94f3cb 100644
--- a/dir.h
+++ b/dir.h
@@ -31,11 +31,14 @@ struct exclude_list {
 
 struct dir_struct {
 	int nr, alloc;
+	int ignored_nr, ignored_alloc;
 	unsigned int show_ignored:1,
 		     show_other_directories:1,
 		     hide_empty_directories:1,
-		     no_gitlinks:1;
+		     no_gitlinks:1,
+		     collect_ignored:1;
 	struct dir_entry **entries;
+	struct dir_entry **ignored;
 
 	/* Exclude info */
 	const char *exclude_per_dir;
-- 
1.5.2.1.958.gbaa74-dirty

[PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jeff King]

Previously, the code would always set up the excludes, and then manually
pick through the pathspec we were given, assuming that non-added but
existing paths were just ignored. This was mostly correct, but would
erroneously mark a totally empty directory as 'ignored'.

Instead, we now use the collect_ignored option of dir_struct, which
unambiguously tells us whether a path was ignored. This simplifies the
code, and means empty directories are now just not mentioned at all.

Furthermore, we now conditionally ask dir_struct to respect excludes,
depending on whether the '-f' flag has been set. This means we don't have
to pick through the result, checking for an 'ignored' flag; ignored entries
were either added or not in the first place.

We can safely get rid of the special 'ignored' flags to dir_entry, which
were not used anywhere else.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-add.c |   89 ++++++++++++++++++++++++++++++--------------------------
 dir.c         |    1 -
 dir.h         |    4 +--
 3 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 1591171..8988930 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -19,6 +19,23 @@ static const char builtin_add_usage[] =
 static int take_worktree_changes;
 static const char *excludes_file;
 
+static int in_pathspec(const char *k, const char **pathspec) {
+	while(*pathspec) {
+		if (!strcmp(k, *pathspec))
+			return 1;
+		pathspec++;
+	}
+	return 0;
+}
+
+static void prune_ignored(struct dir_struct *dir, const char **pathspec) {
+	int i;
+	for (i = 0; i < dir->ignored_nr; i++) {
+		if (!in_pathspec(dir->ignored[i]->name, pathspec))
+			dir->ignored[i] = NULL;
+	}
+}
+
 static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
@@ -40,42 +57,29 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	dir->nr = dst - dir->entries;
 
 	for (i = 0; i < specs; i++) {
-		struct stat st;
-		const char *match;
-		if (seen[i])
-			continue;
-
-		match = pathspec[i];
-		if (!match[0])
-			continue;
-
-		/* Existing file? We must have ignored it */
-		if (!lstat(match, &st)) {
-			struct dir_entry *ent;
-
-			ent = dir_add_name(dir, match, strlen(match));
-			ent->ignored = 1;
-			if (S_ISDIR(st.st_mode))
-				ent->ignored_dir = 1;
-			continue;
-		}
-		die("pathspec '%s' did not match any files", match);
+		if(!seen[i] && !file_exists(pathspec[i]))
+			die("pathspec '%s' did not match any files",
+					pathspec[i]);
 	}
 }
 
-static void fill_directory(struct dir_struct *dir, const char **pathspec)
+static void fill_directory(struct dir_struct *dir, const char **pathspec,
+		int ignored_too)
 {
 	const char *path, *base;
 	int baselen;
 
 	/* Set up the default git porcelain excludes */
 	memset(dir, 0, sizeof(*dir));
-	dir->exclude_per_dir = ".gitignore";
-	path = git_path("info/exclude");
-	if (!access(path, R_OK))
-		add_excludes_from_file(dir, path);
-	if (!access(excludes_file, R_OK))
-		add_excludes_from_file(dir, excludes_file);
+	if (!ignored_too) {
+		dir->collect_ignored = 1;
+		dir->exclude_per_dir = ".gitignore";
+		path = git_path("info/exclude");
+		if (!access(path, R_OK))
+			add_excludes_from_file(dir, path);
+		if (!access(excludes_file, R_OK))
+			add_excludes_from_file(dir, excludes_file);
+	}
 
 	/*
 	 * Calculate common prefix for the pathspec, and
@@ -93,8 +97,10 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec)
 
 	/* Read the directory and prune it */
 	read_directory(dir, path, base, baselen, pathspec);
-	if (pathspec)
+	if (pathspec) {
+		prune_ignored(dir, pathspec);
 		prune_directory(dir, pathspec, baselen);
+	}
 }
 
 static void update_callback(struct diff_queue_struct *q,
@@ -160,6 +166,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	const char **pathspec;
 	struct dir_struct dir;
 	int add_interactive = 0;
+	int has_ignored;
 
 	for (i = 1; i < argc; i++) {
 		if (!strcmp("--interactive", argv[i]) ||
@@ -219,13 +226,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	pathspec = get_pathspec(prefix, argv + i);
 
-	fill_directory(&dir, pathspec);
+	fill_directory(&dir, pathspec, ignored_too);
 
 	if (show_only) {
 		const char *sep = "", *eof = "";
 		for (i = 0; i < dir.nr; i++) {
-			if (!ignored_too && dir.entries[i]->ignored)
-				continue;
 			printf("%s%s", sep, dir.entries[i]->name);
 			sep = " ";
 			eof = "\n";
@@ -237,25 +242,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	if (!ignored_too) {
-		int has_ignored = 0;
-		for (i = 0; i < dir.nr; i++)
-			if (dir.entries[i]->ignored)
-				has_ignored = 1;
-		if (has_ignored) {
-			fprintf(stderr, ignore_warning);
-			for (i = 0; i < dir.nr; i++) {
-				if (!dir.entries[i]->ignored)
-					continue;
-				fprintf(stderr, "%s", dir.entries[i]->name);
-				if (dir.entries[i]->ignored_dir)
-					fprintf(stderr, " (directory)");
-				fputc('\n', stderr);
-			}
-			fprintf(stderr,
-				"Use -f if you really want to add them.\n");
-			exit(1);
+	has_ignored = 0;
+	for (i = 0; i < dir.ignored_nr; i++) {
+		if (dir.ignored[i])
+			has_ignored = 1;
+	}
+	if (has_ignored) {
+		fprintf(stderr, ignore_warning);
+		for (i = 0; i < dir.ignored_nr; i++) {
+			if (dir.ignored[i])
+				fprintf(stderr, "%s\n", dir.ignored[i]->name);
 		}
+		fprintf(stderr, "Use -f if you really want to add them.\n");
+		exit(1);
 	}
 
 	for (i = 0; i < dir.nr; i++)
diff --git a/dir.c b/dir.c
index 1ffc1e5..d903a49 100644
--- a/dir.c
+++ b/dir.c
@@ -275,7 +275,6 @@ static
 struct dir_entry *dir_entry_new(const char *pathname, int len) {
 	struct dir_entry *ent;
 	ent = xmalloc(sizeof(*ent) + len + 1);
-	ent->ignored = ent->ignored_dir = 0;
 	ent->len = len;
 	memcpy(ent->name, pathname, len);
 	ent->name[len] = 0;
diff --git a/dir.h b/dir.h
index c94f3cb..ec0e8ab 100644
--- a/dir.h
+++ b/dir.h
@@ -13,9 +13,7 @@
 
 
 struct dir_entry {
-	unsigned int ignored : 1;
-	unsigned int ignored_dir : 1;
-	unsigned int len : 30;
+	unsigned int len;
 	char name[FLEX_ARRAY]; /* more */
 };
 

Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jonas Fonseca]

Hej Jeff,

Thanks for looking into this and making these patches. :)

Jeff King <peff@peff.net> wrote Mon, Jun 11, 2007:
>  builtin-add.c |   89 ++++++++++++++++++++++++++++++--------------------------
>  dir.c         |    1 -
>  dir.h         |    4 +--
>  3 files changed, 49 insertions(+), 45 deletions(-)
> 
> diff --git a/builtin-add.c b/builtin-add.c
> index 1591171..8988930 100644
> --- a/builtin-add.c
> +++ b/builtin-add.c
> @@ -160,6 +166,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	const char **pathspec;
>  	struct dir_struct dir;
>  	int add_interactive = 0;
> +	int has_ignored;
>  
>  	for (i = 1; i < argc; i++) {
>  		if (!strcmp("--interactive", argv[i]) ||
> @@ -237,25 +242,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	if (read_cache() < 0)
>  		die("index file corrupt");
>  
> -	if (!ignored_too) {
> -		int has_ignored = 0;
> -		for (i = 0; i < dir.nr; i++)
> -			if (dir.entries[i]->ignored)
> -				has_ignored = 1;
> -		if (has_ignored) {
> -			fprintf(stderr, ignore_warning);
> -			for (i = 0; i < dir.nr; i++) {
> -				if (!dir.entries[i]->ignored)
> -					continue;
> -				fprintf(stderr, "%s", dir.entries[i]->name);
> -				if (dir.entries[i]->ignored_dir)
> -					fprintf(stderr, " (directory)");
> -				fputc('\n', stderr);
> -			}
> -			fprintf(stderr,
> -				"Use -f if you really want to add them.\n");
> -			exit(1);
> +	has_ignored = 0;
> +	for (i = 0; i < dir.ignored_nr; i++) {
> +		if (dir.ignored[i])
> +			has_ignored = 1;
> +	}
> +	if (has_ignored) {
> +		fprintf(stderr, ignore_warning);
> +		for (i = 0; i < dir.ignored_nr; i++) {
> +			if (dir.ignored[i])
> +				fprintf(stderr, "%s\n", dir.ignored[i]->name);
>  		}
> +		fprintf(stderr, "Use -f if you really want to add them.\n");
> +		exit(1);
>  	}
>  
>  	for (i = 0; i < dir.nr; i++)

I think you could even get rid of has_ignored with something like this.

diff --git a/builtin-add.c b/builtin-add.c
index 8988930..da6ab11 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -166,7 +166,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	const char **pathspec;
 	struct dir_struct dir;
 	int add_interactive = 0;
-	int has_ignored;
 
 	for (i = 1; i < argc; i++) {
 		if (!strcmp("--interactive", argv[i]) ||
@@ -242,12 +241,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	has_ignored = 0;
-	for (i = 0; i < dir.ignored_nr; i++) {
-		if (dir.ignored[i])
-			has_ignored = 1;
-	}
-	if (has_ignored) {
+	if (dir.ignored_nr) {
 		fprintf(stderr, ignore_warning);
 		for (i = 0; i < dir.ignored_nr; i++) {
 			if (dir.ignored[i])

-- 
Jonas Fonseca

Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jeff King]

On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote:

> Thanks for looking into this and making these patches. :)

No problem. I had run it across it before and it was on my "look into
this" list...thank you for providing a test case. :)

> I think you could even get rid of has_ignored with something like this.

Nope, I had originally wanted to do that, but the dir_struct.ignored
list contains _all_ ignored items, not just those that were originally
in the pathspec. The prune_ignored call sets uninteresting ones to
NULL.  That function could compact the list and re-set ignored_nr, but
it doesn't currently do so.

An even more elegant solution would be for read_directory to mark
whether an ignored file comes from a pathspec, or was found through
recursion. That would be more efficient, and it would remove the
prune_ignored thing, which is IMHO a little hack-ish.

I don't have time to work on it now, but I might look at it more tonight
or tomorrow (but please, if you are interested, take a crack at it).

-Peff

Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jonas Fonseca]

Jeff King <peff@peff.net> wrote Mon, Jun 11, 2007:
> On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote:
> > I think you could even get rid of has_ignored with something like this.
> 
> Nope, I had originally wanted to do that, but the dir_struct.ignored
> list contains _all_ ignored items, not just those that were originally
> in the pathspec. The prune_ignored call sets uninteresting ones to
> NULL.  That function could compact the list and re-set ignored_nr, but
> it doesn't currently do so.

Ah, I see.

> An even more elegant solution would be for read_directory to mark
> whether an ignored file comes from a pathspec, or was found through
> recursion. That would be more efficient, and it would remove the
> prune_ignored thing, which is IMHO a little hack-ish.
> 
> I don't have time to work on it now, but I might look at it more tonight
> or tomorrow (but please, if you are interested, take a crack at it).

Yes, I think it might be nice for me to do if you don't mind. I would
like some more experience with the git code. Maybe even redo the whole
patch series to also fix the concerns about the alloc_grow macro.

-- 
Jonas Fonseca

Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jeff King]

On Mon, Jun 11, 2007 at 09:15:54PM +0200, Jonas Fonseca wrote:

> > I don't have time to work on it now, but I might look at it more tonight
> > or tomorrow (but please, if you are interested, take a crack at it).
> 
> Yes, I think it might be nice for me to do if you don't mind. I would
> like some more experience with the git code. Maybe even redo the whole
> patch series to also fix the concerns about the alloc_grow macro.

Great, feel free to rework the series as you see fit. My "tonight"
timeframe was a bit optimistic anyway, as we have a new baby here. :)

-Peff

Re: [PATCH 3/3] builtin-add: simplify (and increase accuracy of) exclude handling

[Jonas Fonseca]

From: Jeff King <peff@peff.net>

Previously, the code would always set up the excludes, and then manually
pick through the pathspec we were given, assuming that non-added but
existing paths were just ignored. This was mostly correct, but would
erroneously mark a totally empty directory as 'ignored'.

Instead, we now use the collect_ignored option of dir_struct, which
unambiguously tells us whether a path was ignored. This simplifies the
code, and means empty directories are now just not mentioned at all.

Furthermore, we now conditionally ask dir_struct to respect excludes,
depending on whether the '-f' flag has been set. This means we don't have
to pick through the result, checking for an 'ignored' flag; ignored entries
were either added or not in the first place.

We can safely get rid of the special 'ignored' flags to dir_entry, which
were not used anywhere else.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 builtin-add.c |   69 +++++++++++++++++---------------------------------------
 dir.c         |   16 +++++++++++-
 dir.h         |    4 +--
 3 files changed, 36 insertions(+), 53 deletions(-)

 Jeff King <peff@peff.net> wrote Mon, Jun 11, 2007:
 > On Mon, Jun 11, 2007 at 05:01:23PM +0200, Jonas Fonseca wrote:
 > > I think you could even get rid of has_ignored with something like this.
 > 
 > Nope, I had originally wanted to do that, but the dir_struct.ignored
 > list contains _all_ ignored items, not just those that were originally
 > in the pathspec. The prune_ignored call sets uninteresting ones to
 > NULL.  That function could compact the list and re-set ignored_nr, but
 > it doesn't currently do so.
 > 
 > An even more elegant solution would be for read_directory to mark
 > whether an ignored file comes from a pathspec, or was found through
 > recursion. That would be more efficient, and it would remove the
 > prune_ignored thing, which is IMHO a little hack-ish.

 OK, I tried to do this, however, I got a bit confused with the intended
 behavior. Anyway, it passes the test suite, is silent for empty
 directories and will only show exact matches of the pathspec (this was
 the confusing part) as ignored items.

diff --git a/builtin-add.c b/builtin-add.c
index 1591171..ad6aca8 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -40,42 +40,29 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	dir->nr = dst - dir->entries;
 
 	for (i = 0; i < specs; i++) {
-		struct stat st;
-		const char *match;
-		if (seen[i])
-			continue;
-
-		match = pathspec[i];
-		if (!match[0])
-			continue;
-
-		/* Existing file? We must have ignored it */
-		if (!lstat(match, &st)) {
-			struct dir_entry *ent;
-
-			ent = dir_add_name(dir, match, strlen(match));
-			ent->ignored = 1;
-			if (S_ISDIR(st.st_mode))
-				ent->ignored_dir = 1;
-			continue;
-		}
-		die("pathspec '%s' did not match any files", match);
+		if(!seen[i] && !file_exists(pathspec[i]))
+			die("pathspec '%s' did not match any files",
+					pathspec[i]);
 	}
 }
 
-static void fill_directory(struct dir_struct *dir, const char **pathspec)
+static void fill_directory(struct dir_struct *dir, const char **pathspec,
+		int ignored_too)
 {
 	const char *path, *base;
 	int baselen;
 
 	/* Set up the default git porcelain excludes */
 	memset(dir, 0, sizeof(*dir));
-	dir->exclude_per_dir = ".gitignore";
-	path = git_path("info/exclude");
-	if (!access(path, R_OK))
-		add_excludes_from_file(dir, path);
-	if (!access(excludes_file, R_OK))
-		add_excludes_from_file(dir, excludes_file);
+	if (!ignored_too) {
+		dir->collect_ignored = 1;
+		dir->exclude_per_dir = ".gitignore";
+		path = git_path("info/exclude");
+		if (!access(path, R_OK))
+			add_excludes_from_file(dir, path);
+		if (!access(excludes_file, R_OK))
+			add_excludes_from_file(dir, excludes_file);
+	}
 
 	/*
 	 * Calculate common prefix for the pathspec, and
@@ -219,13 +206,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	pathspec = get_pathspec(prefix, argv + i);
 
-	fill_directory(&dir, pathspec);
+	fill_directory(&dir, pathspec, ignored_too);
 
 	if (show_only) {
 		const char *sep = "", *eof = "";
 		for (i = 0; i < dir.nr; i++) {
-			if (!ignored_too && dir.entries[i]->ignored)
-				continue;
 			printf("%s%s", sep, dir.entries[i]->name);
 			sep = " ";
 			eof = "\n";
@@ -237,25 +222,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die("index file corrupt");
 
-	if (!ignored_too) {
-		int has_ignored = 0;
-		for (i = 0; i < dir.nr; i++)
-			if (dir.entries[i]->ignored)
-				has_ignored = 1;
-		if (has_ignored) {
-			fprintf(stderr, ignore_warning);
-			for (i = 0; i < dir.nr; i++) {
-				if (!dir.entries[i]->ignored)
-					continue;
-				fprintf(stderr, "%s", dir.entries[i]->name);
-				if (dir.entries[i]->ignored_dir)
-					fprintf(stderr, " (directory)");
-				fputc('\n', stderr);
-			}
-			fprintf(stderr,
-				"Use -f if you really want to add them.\n");
-			exit(1);
+	if (dir.ignored_nr) {
+		fprintf(stderr, ignore_warning);
+		for (i = 0; i < dir.ignored_nr; i++) {
+			fprintf(stderr, "%s\n", dir.ignored[i]->name);
 		}
+		fprintf(stderr, "Use -f if you really want to add them.\n");
+		exit(1);
 	}
 
 	for (i = 0; i < dir.nr; i++)
diff --git a/dir.c b/dir.c
index 1ffc1e5..f3a6757 100644
--- a/dir.c
+++ b/dir.c
@@ -275,7 +275,6 @@ static
 struct dir_entry *dir_entry_new(const char *pathname, int len) {
 	struct dir_entry *ent;
 	ent = xmalloc(sizeof(*ent) + len + 1);
-	ent->ignored = ent->ignored_dir = 0;
 	ent->len = len;
 	memcpy(ent->name, pathname, len);
 	ent->name[len] = 0;
@@ -432,6 +431,18 @@ static int simplify_away(const char *path, int pathlen, const struct path_simpli
 	return 0;
 }
 
+static int in_pathspec(const char *path, int len, const struct path_simplify *simplify)
+{
+	if (simplify) {
+		for (; simplify->path; simplify++) {
+			if (len == simplify->len
+			    && !memcmp(path, simplify->path, len))
+				return 1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Read a directory tree. We currently ignore anything but
  * directories, regular files and symlinks. That's because git
@@ -472,7 +483,8 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 				continue;
 
 			exclude = excluded(dir, fullname);
-			if (exclude && dir->collect_ignored)
+			if (exclude && dir->collect_ignored
+			    && in_pathspec(fullname, baselen + len, simplify))
 				dir_add_ignored(dir, fullname, baselen + len);
 			if (exclude != dir->show_ignored) {
 				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
diff --git a/dir.h b/dir.h
index c94f3cb..ec0e8ab 100644
--- a/dir.h
+++ b/dir.h
@@ -13,9 +13,7 @@
 
 
 struct dir_entry {
-	unsigned int ignored : 1;
-	unsigned int ignored_dir : 1;
-	unsigned int len : 30;
+	unsigned int len;
 	char name[FLEX_ARRAY]; /* more */
 };
 
-- 
1.5.2.1.958.g264c-dirty

-- 
Jonas Fonseca