[BUG] Git add <device file> silently fails

[BUG] Git add <device file> silently fails

[Andreas Gruenbacher]

Hello,

there is code in read-cache.c:add_to_index() which checks if a file to be 
added is a regular file, directory, or symlink; this function otherwise 
error()s out.  It looks as if add_to_index() is supposed to be called via:

  builtin/add.c:update_callback() ->
    read-cache.c:add_file_to_index() ->
      read-cache.c:add_to_index()

However, when trying to add a device special file, update_callback() ends up 
never getting called, no error message is produced, and git add silently 
fails:

	$ sudo mknod foobar c 1 3
	$ git add foobar
   $ echo $?
	0

Maybe someone familiar with run_diff_files() can have a look?

Thanks,
Andreas

Re: [BUG] Git add <device file> silently fails

[Alex Riesen]

On Sat, Apr 17, 2010 at 16:24, Andreas Gruenbacher <agruen@suse.de> wrote:
> However, when trying to add a device special file, update_callback() ends up
> never getting called, no error message is produced, and git add silently
> fails:
>
>        $ sudo mknod foobar c 1 3
>        $ git add foobar
>   $ echo $?
>        0

I think something like this should make the accident more
noticable:

diff --git a/builtin/add.c b/builtin/add.c
index 87d2980..9c4a5f2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -347,6 +347,8 @@ static int add_files(struct dir_struct *dir, int flags)
 		die("no files added");
 	}

+	if (!dir->nr)
+		die("No files selected for addition");
 	for (i = 0; i < dir->nr; i++)
 		if (add_file_to_cache(dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)

Re: [BUG] Git add <device file> silently fails

[Andreas Gruenbacher]

On Saturday 17 April 2010 16:44:05 Alex Riesen wrote:
> I think something like this should make the accident more
> noticable:

Doesn't actually tell what the problem might be, though.

Thanks,
Andreas

Re: [BUG] Git add <device file> silently fails

[Alex Riesen]

On Sat, Apr 17, 2010 at 17:00, Andreas Gruenbacher <agruen@suse.de> wrote:
> On Saturday 17 April 2010 16:44:05 Alex Riesen wrote:
>> I think something like this should make the accident more
>> noticable:
>
> Doesn't actually tell what the problem might be, though.
>

Oh, sorry. Somehow I thought, you did found what the problem is
(Git is not an archiving tool and does not store irregular files), and
just wanted to point at "git add" failing silently adding such a name.

In this particular case, the name you given to Git, is a device
entry point, which is filtered. Far too early, in my opinion, so
no sensible diagnostics can be produced. That's why I suggested
an exit with generic error message. I hope it is enough to at least
point the user to where a problem (like a fifo, device or socket
give in the command line) might be.

Re: [BUG] Git add <device file> silently fails

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> I think something like this should make the accident more
> noticable:

The early skippage done in dir.c (read-directory-recursive) should treat
these as ignored just like paths that are ignored with .gitignore
mechanism, and if we do so, we shouldn't need this patch to add another
codepath to give notification to the user (we would however still need
to reword "'add -f' if you really want to add it", though).

> diff --git a/builtin/add.c b/builtin/add.c
> index 87d2980..9c4a5f2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -347,6 +347,8 @@ static int add_files(struct dir_struct *dir, int flags)
>  		die("no files added");
>  	}
>
> +	if (!dir->nr)
> +		die("No files selected for addition");
>  	for (i = 0; i < dir->nr; i++)
>  		if (add_file_to_cache(dir->entries[i]->name, flags)) {
>  			if (!ignore_add_errors)

Re: [BUG] Git add <device file> silently fails

[Alex Riesen]

On Sat, Apr 17, 2010 at 18:38, Junio C Hamano <gitster@pobox.com> wrote:
> Alex Riesen <raa.lkml@gmail.com> writes:
>
>> I think something like this should make the accident more
>> noticable:
>
> The early skippage done in dir.c (read-directory-recursive) should treat
> these as ignored just like paths that are ignored with .gitignore
> mechanism, and if we do so, we shouldn't need this patch to add another
> codepath to give notification to the user (we would however still need
> to reword "'add -f' if you really want to add it", though).
>

I see. Special files are not treated as ignored yet (and there will be
no way to un-ignore them). I have to read the code for a while,
ignored pathnames are sometimes stored for later use too.

Re: [BUG] Git add <device file> silently fails

[Alex Riesen]

On Sat, Apr 17, 2010 at 19:32, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Sat, Apr 17, 2010 at 18:38, Junio C Hamano <gitster@pobox.com> wrote:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>>> I think something like this should make the accident more
>>> noticable:
>>
>> The early skippage done in dir.c (read-directory-recursive) should treat
>> these as ignored just like paths that are ignored with .gitignore
>> mechanism, and if we do so, we shouldn't need this patch to add another
>> codepath to give notification to the user (we would however still need
>> to reword "'add -f' if you really want to add it", though).
>>
>
> I see. Special files are not treated as ignored yet (and there will be
> no way to un-ignore them). I have to read the code for a while,
> ignored pathnames are sometimes stored for later use too.
>

I am tempted to add a mode_t or DT_something to struct dir_entry.
Users of read_directory are likely to do an lstat anyway (well,
builtin/add.c does), and sometimes there is something to fill it with
(get_dtype for DT_UNKNOWN). The only problem is that the structure
is allocated a lot...

Re: [BUG] Git add <device file> silently fails

[Andreas Gruenbacher]

On Saturday 17 April 2010 18:38:12 Junio C Hamano wrote:
> The early skippage done in dir.c (read-directory-recursive) should treat
> these as ignored just like paths that are ignored with .gitignore
> mechanism, and if we do so, we shouldn't need this patch to add another
> codepath to give notification to the user (we would however still need
> to reword "'add -f' if you really want to add it", though).

I see, but dumbing down the error message until it fits both cases doesn'
seem all that useful, either.  Here is a shot, maybe it's acceptable in
your eyes.

Andreas


Subject: [PATCH] Complain when trying to "git add" decive special files

This is done by adding a list of files to struct dir_struct which were
ignored because of their file type.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
---
 builtin/add.c |   26 +++++++++++++++++---------
 dir.c         |   19 +++++++++++++------
 dir.h         |   14 ++++++++++++--
 3 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 87d2980..a3d7839 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,9 +304,6 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 
 static struct lock_file lock_file;
 
-static const char ignore_error[] =
-"The following paths are ignored by one of your .gitignore files:\n";
-
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
 static int ignore_add_errors, addremove, intent_to_add;
 
@@ -339,14 +336,25 @@ static int add_files(struct dir_struct *dir, int flags)
 {
 	int i, exit_status = 0;
 
-	if (dir->ignored_nr) {
-		fprintf(stderr, ignore_error);
-		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");
-		die("no files added");
+	if (dir->ignored[DIR_IGNORED_FILETYPE].nr) {
+		fprintf(stderr, "The following paths are ignored "
+				"because their file types are not supported:\n");
+		for (i = 0; i < dir->ignored[DIR_IGNORED_FILETYPE].nr; i++)
+			fprintf(stderr, "%s\n",
+				dir->ignored[DIR_IGNORED_FILETYPE].entries[i]->name);
 	}
 
+	if (dir->ignored[DIR_IGNORED_EXCLUDED].nr) {
+		fprintf(stderr, "The following paths are ignored "
+				"by one of your .gitignore files; "
+				"use -f if you really want to add them:\n");
+		for (i = 0; i < dir->ignored[DIR_IGNORED_EXCLUDED].nr; i++)
+			fprintf(stderr, "%s\n",
+				dir->ignored[DIR_IGNORED_EXCLUDED].entries[i]->name);
+	}
+	if (dir->ignored[DIR_IGNORED_FILETYPE].nr || dir->ignored[DIR_IGNORED_EXCLUDED].nr)
+		die("no files added");
+
 	for (i = 0; i < dir->nr; i++)
 		if (add_file_to_cache(dir->entries[i]->name, flags)) {
 			if (!ignore_add_errors)
diff --git a/dir.c b/dir.c
index cb83332..87e7fca 100644
--- a/dir.c
+++ b/dir.c
@@ -453,13 +453,16 @@ 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)
+static struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char *pathname, int len,
+					 enum ignore_reason reason)
 {
+	struct dir_vector *vector = &dir->ignored[reason];
+
 	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);
+	ALLOC_GROW(vector->entries, vector->nr+1, vector->alloc);
+	return vector->entries[vector->nr++] = dir_entry_new(pathname, len);
 }
 
 enum exist_status {
@@ -695,7 +698,7 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 	int exclude = excluded(dir, path, &dtype);
 	if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
 	    && exclude_matches_pathspec(path, *len, simplify))
-		dir_add_ignored(dir, path, *len);
+		dir_add_ignored(dir, path, *len, DIR_IGNORED_EXCLUDED);
 
 	/*
 	 * Excluded? If we don't explicitly want to show
@@ -720,7 +723,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
 
 	switch (dtype) {
 	default:
-		return path_ignored;
+		dir_add_ignored(dir, path, *len, DIR_IGNORED_FILETYPE);
+		break;
 	case DT_DIR:
 		memcpy(path + *len, "/", 2);
 		(*len)++;
@@ -907,6 +911,7 @@ static int treat_leading_path(struct dir_struct *dir,
 int read_directory(struct dir_struct *dir, const char *path, int len, const char **pathspec)
 {
 	struct path_simplify *simplify;
+	enum ignore_reason reason;
 
 	if (has_symlink_leading_path(path, len))
 		return dir->nr;
@@ -916,7 +921,9 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const 
char
 		read_directory_recursive(dir, path, len, 0, simplify);
 	free_simplify(simplify);
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
-	qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), cmp_name);
+	for (reason = 0; reason < ARRAY_SIZE(dir->ignored); reason++)
+		qsort(dir->ignored[reason].entries, dir->ignored[reason].nr,
+		      sizeof(struct dir_entry *), cmp_name);
 	return dir->nr;
 }
 
diff --git a/dir.h b/dir.h
index 3bead5f..b7d1a21 100644
--- a/dir.h
+++ b/dir.h
@@ -31,9 +31,18 @@ struct exclude_stack {
 	int exclude_ix;
 };
 
+enum ignore_reason {
+	DIR_IGNORED_EXCLUDED,
+	DIR_IGNORED_FILETYPE,
+};
+
+struct dir_vector {
+	int nr, alloc;
+	struct dir_entry **entries;
+};
+
 struct dir_struct {
 	int nr, alloc;
-	int ignored_nr, ignored_alloc;
 	enum {
 		DIR_SHOW_IGNORED = 1<<0,
 		DIR_SHOW_OTHER_DIRECTORIES = 1<<1,
@@ -42,7 +51,8 @@ struct dir_struct {
 		DIR_COLLECT_IGNORED = 1<<4
 	} flags;
 	struct dir_entry **entries;
-	struct dir_entry **ignored;
+	struct dir_vector ignored[2];
+
 
 	/* Exclude info */
 	const char *exclude_per_dir;
-- 
1.7.1.rc1.12.ga601

Re: [BUG] Git add <device file> silently fails

[Junio C Hamano]

Andreas Gruenbacher <agruen@suse.de> writes:

> @@ -720,7 +723,8 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
>  
>  	switch (dtype) {
>  	default:
> -		return path_ignored;
> +		dir_add_ignored(dir, path, *len, DIR_IGNORED_FILETYPE);
> +		break;

Hmm, do we want to break and return path_handled here, to cause
the calling read_directory_recursive() to call dir_add_name()?

Also I suspect that (dir->flags & DIR_COLLECT_IGNORED) needs to be checked
before making this call.

> +struct dir_vector {
> +	int nr, alloc;
> +	struct dir_entry **entries;
> +};

We would probably call a structure of this shape "dir_array", as I haven't
seen us calling anything "vector" for naming consistency.

Instead of introducing two dir-arrays for different kinds of ignoredness,
it may be cleaner to add one bit (or more for later expansion) to dir_entry
and mark the ones in ignored dir-array with the ignore reason.