threads / discuss / 12408

How to "git add ." when some files are not accessible (permission denied)?

Subject: How to "git add ." when some files are not accessible (permission denied)?

## tl;dr

32 messages between Mar 1, 2008 and May 13, 2008.

replies: 31people: 7as markdown or json

Dirk Süsserott· Mar 1, 2008, 13:46 UTC · lore
Hello --

First of all: I'm using Git with Windows. Most of the time it works *very* good, but now I've a problem that might appear to Unix users as well. I had the idea to use Git to track changes in my C:\WINDOWS directory. I thought the following would work:

$ cd /c/WINDOWS (1) $ git init (2) $ git add . (3) $ git commit -m "Initial" (4)

And then issue "git status" or so to see the differences after installing or running some software.

However, when issueing (3) "git add ." it adds hundreds and thousands of files and then stops with

     error: open("foo"): Permission denied: foo
     fatal: unable to index file foo

The file "foo" is not accessible for me, even though I'm administrator. This might occur to Windows and Linux persons as well, I guess. The adding stops overall.

The question is: is there a way to tell "git add ." to add all files but ignore those that cannot be added due to lack of authorization?

Or, more generally spoken: can I tell "git add" to add only those files it can handle and ignore the rest? The "-f" switch doesn't work and some exclude lists on a per file basis are not applicable for my purpose as I don't know the files in advance.

I'm aware that I could do it with some fancy shell commands, but very often I was surprised how many really cool commands Git offers to "do what I mean". Wished other software would be so usable :-).

Cheers,
  -- Dirk
Jeff King· Mar 2, 2008, 01:19 UTC · re: Dirk Süsserott · lore

Re: How to "git add ." when some files are not accessible (permission denied)?

On Sat, Mar 01, 2008 at 02:46:28PM +0100, Dirk Süsserott wrote:
> The question is: is there a way to tell "git add ." to add all files but
> ignore those that cannot be added due to lack of authorization?

No, there isn't. Under Linux, I would come up with a list of files I was interested in and then pipe it to "xargs git-add", which is probably unhelpful for Windows.

But I think more fundamentally, you probably _do_ want to come up with a list of files that you can't access and add them to your .gitignore (or your .git/info/exclude file if they are purely a local matter). That is the official way to tell all git commands "I really don't care about these files". Otherwise they will keep getting brought up when you do, e.g., a git-status.

> Or, more generally spoken: can I tell "git add" to add only those files  
> it can handle and ignore the rest? The "-f" switch doesn't work and some
> exclude lists on a per file basis are not applicable for my purpose as I
> don't know the files in advance.

The only reason I can think of to not want to generate such an ignore list is if you are frequently and automagically doing a "git add ." to pick up new files. For that reason, a "try to continue even if we can't look at some files" option to git add might make some sense.

-Peff
Dirk Süsserott· Mar 3, 2008, 19:17 UTC · re: Jeff King · lore

Re: How to "git add ." when some files are not accessible (permission denied)?

Jeff King schrieb:
Show 12 quoted lines
> On Sat, Mar 01, 2008 at 02:46:28PM +0100, Dirk Süsserott wrote:
>
>   
>> The question is: is there a way to tell "git add ." to add all files but
>> ignore those that cannot be added due to lack of authorization?
>>     
>
> No, there isn't. Under Linux, I would come up with a list of files I was
> interested in and then pipe it to "xargs git-add", which is probably
> unhelpful for Windows.
>
>   

Not quite. I'm using the msysGit package from http://code.google.com/p/msysgit/downloads/list and that comes with some fundamental unix tools like a sound shell, find, xargs, and many more. Very good! This way prepared, I used "git ls-files -o | xargs git add -v" until most of my files were added. For the rest I did "xargs -l" (ell) so that the files got added one by one. The files that still refused to be added are finally ignored by "git ls-files -o >> .gitignore".

Caveat: filenames containing blanks are not handled properly as they are 
not surrounded by quotes. "git add" thinks of them as two or more files 
and fails.
I figure xargs has some cool switches to sourround the parameters with 
quotes, but I didn't find them. An option was to write a script or shell 
function that does it and pipe the filenames through that function or -- 
as filenames with blanks aren't so numerous -- to add them manually with 
"git gui".
Eventually, I solved the problem. Thanks for and to your pointers. :-)
  -- Dirk
Junio C Hamano· Mar 3, 2008, 20:06 UTC · re: Dirk Süsserott · lore

Re: How to "git add ." when some files are not accessible (permission denied)?

Dirk Süsserott <newsletter@dirk.my1.cc> writes:
Show 6 quoted lines
> This way prepared, I used "git ls-files -o | xargs git add -v" until
> most of my files were added.
> ...
> Caveat: filenames containing blanks are not handled properly as they
> are not surrounded by quotes. "git add" thinks of them as two or more
> files and fails.
Perhaps "git ls-files -z -o | git update-index --add --stdin"
Dirk Süsserott· Mar 3, 2008, 20:32 UTC · re: Junio C Hamano · lore

Re: How to "git add ." when some files are not accessible (permission denied)?

Junio C Hamano schrieb:
Show 14 quoted lines
> Dirk Süsserott <newsletter@dirk.my1.cc> writes:
>
>   
>> This way prepared, I used "git ls-files -o | xargs git add -v" until
>> most of my files were added.
>> ...
>> Caveat: filenames containing blanks are not handled properly as they
>> are not surrounded by quotes. "git add" thinks of them as two or more
>> files and fails.
>>     
>
> Perhaps "git ls-files -z -o | git update-index --add --stdin"
>
>   

That's exactly what I meant when I wrote "Git almost always has a solution for 'do what I mean'". It is, however, sometimes hard to find, but the solution is always there. Its such a great piece of software. Congratulations to you all.

Alex Riesen· Mar 2, 2008, 15:41 UTC · re: Dirk Süsserott · lore

Re: How to "git add ." when some files are not accessible (permission denied)?

Dirk Süsserott, Sat, Mar 01, 2008 14:46:28 +0100:
> Or, more generally spoken: can I tell "git add" to add only those files it 
> can handle and ignore the rest? The "-f" switch doesn't work and some
> exclude lists on a per file basis are not applicable for my purpose as I
> don't know the files in advance.

Well, "-f" means something else (include the ignored files). It is unfortunate, because (I think) your case fits better its traditional meaning...

You can try the following patches, which add "--ignore-errors" to git-add. Maybe it will be enough... It is generally considered not safe to ignore errors.

Alex Riesen· Mar 2, 2008, 15:42 UTC · re: Alex Riesen · lore

[PATCH] Make the exit code of add_file_to_index actually useful

Update the programs which used the function (as add_file_to_cache).
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-add.c    |    6 ++++--
 builtin-commit.c |    7 ++++---
 builtin-mv.c     |    3 ++-
 read-cache.c     |    8 ++++----
 4 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index 820110e..abfe473 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -94,7 +94,8 @@ static void update_callback(struct diff_queue_struct *q,
 		case DIFF_STATUS_UNMERGED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
-			add_file_to_cache(path, verbose);
+			if (add_file_to_cache(path, verbose))
+				exit(1);
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
@@ -266,7 +267,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < dir.nr; i++)
-		add_file_to_cache(dir.entries[i]->name, verbose);
+		if (add_file_to_cache(dir.entries[i]->name, verbose))
+			exit(1);
 
  finish:
 	if (active_cache_changed) {
diff --git a/builtin-commit.c b/builtin-commit.c
index f49c22e..fb1e588 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -176,9 +176,10 @@ static void add_remove_files(struct path_list *list)
 	int i;
 	for (i = 0; i < list->nr; i++) {
 		struct path_list_item *p = &(list->items[i]);
-		if (file_exists(p->path))
-			add_file_to_cache(p->path, 0);
-		else
+		if (file_exists(p->path)) {
+			if (add_file_to_cache(p->path, 0))
+				exit(1);
+		} else
 			remove_file_from_cache(p->path);
 	}
 }
diff --git a/builtin-mv.c b/builtin-mv.c
index 68aa2a6..ec6e09d 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -260,7 +260,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		for (i = 0; i < added.nr; i++) {
 			const char *path = added.items[i].path;
-			add_file_to_cache(path, verbose);
+			if (add_file_to_cache(path, verbose))
+				exit(1);
 		}
 
 		for (i = 0; i < deleted.nr; i++)
diff --git a/read-cache.c b/read-cache.c
index 657f0c5..4a4f511 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -461,10 +461,10 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)
 	unsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_RACY_IS_DIRTY;
 
 	if (lstat(path, &st))
-		die("%s: unable to stat (%s)", path, strerror(errno));
+		return error("%s: unable to stat (%s)", path, strerror(errno));
 
 	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode) && !S_ISDIR(st.st_mode))
-		die("%s: can only add regular files, symbolic links or git-directories", path);
+		return error("%s: can only add regular files, symbolic links or git-directories", path);
 
 	namelen = strlen(path);
 	if (S_ISDIR(st.st_mode)) {
@@ -501,9 +501,9 @@ int add_file_to_index(struct index_state *istate, const char *path, int verbose)
 	}
 
 	if (index_path(ce->sha1, path, &st, 1))
-		die("unable to index file %s", path);
+		return error("unable to index file %s", path);
 	if (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
-		die("unable to add %s to index",path);
+		return error("unable to add %s to index",path);
 	if (verbose)
 		printf("add '%s'\n", path);
 	return 0;
-- 
1.5.4.3.391.gf5a0c
Alex Riesen· Mar 2, 2008, 15:43 UTC · re: Alex Riesen · lore

[PATCH] Extend interface of add_files_to_cache to allow ignore indexing errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-add.c      |   37 ++++++++++++++++++++++++++++---------
 builtin-checkout.c |    2 +-
 builtin-commit.c   |    2 +-
 cache.h            |    8 +++++++-
 4 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index abfe473..bc55a0e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -79,12 +79,18 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
 		prune_directory(dir, pathspec, baselen);
 }
 
+struct update_callback_data
+{
+	int flags;
+	int add_errors;
+};
+
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
-	int i, verbose;
+	int i;
+	struct update_callback_data *data = cbdata;
 
-	verbose = *((int *)cbdata);
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
@@ -94,28 +100,35 @@ static void update_callback(struct diff_queue_struct *q,
 		case DIFF_STATUS_UNMERGED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
-			if (add_file_to_cache(path, verbose))
-				exit(1);
+			if (add_file_to_cache(path, data->flags & ADD_FILES_VERBOSE)) {
+				if (!(data->flags & ADD_FILES_IGNORE_ERRORS))
+					exit(1);
+				data->add_errors++;
+			}
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
-			if (verbose)
+			if (data->flags & ADD_FILES_VERBOSE)
 				printf("remove '%s'\n", path);
 			break;
 		}
 	}
 }
 
-void add_files_to_cache(int verbose, const char *prefix, const char **pathspec)
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
+	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.prune_data = pathspec;
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	rev.diffopt.format_callback_data = &verbose;
+	data.flags = flags;
+	data.add_errors = 0;
+	rev.diffopt.format_callback_data = &data;
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	return !!data.add_errors;
 }
 
 static void refresh(int verbose, const char **pathspec)
@@ -193,6 +206,7 @@ static struct option builtin_add_options[] = {
 
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
+	int exit_status = 0;
 	int i, newfd;
 	const char **pathspec;
 	struct dir_struct dir;
@@ -209,11 +223,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	newfd = hold_locked_index(&lock_file, 1);
 
 	if (take_worktree_changes) {
+		int flags = 0;
 		const char **pathspec;
 		if (read_cache() < 0)
 			die("index file corrupt");
 		pathspec = get_pathspec(prefix, argv);
-		add_files_to_cache(verbose, prefix, pathspec);
+
+		if (verbose)
+			flags |= ADD_FILES_VERBOSE;
+
+		exit_status = add_files_to_cache(prefix, pathspec, flags);
 		goto finish;
 	}
 
@@ -277,5 +296,5 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			die("Unable to write new index file");
 	}
 
-	return 0;
+	return exit_status;
 }
diff --git a/builtin-checkout.c b/builtin-checkout.c
index b0cd788..2def093 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -273,7 +273,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(0, NULL, NULL);
+			add_files_to_cache(NULL, NULL, 0);
 			work = write_tree_from_memory();
 
 			ret = reset_to_new(new->commit->tree, opts->quiet);
diff --git a/builtin-commit.c b/builtin-commit.c
index fb1e588..d5e8c4c 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -242,7 +242,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	 */
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(0, also ? prefix : NULL, pathspec);
+		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
 		refresh_cache(REFRESH_QUIET);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
diff --git a/cache.h b/cache.h
index f16d341..c6c1659 100644
--- a/cache.h
+++ b/cache.h
@@ -748,7 +748,13 @@ extern int convert_to_git(const char *path, const char *src, size_t len,
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
-void add_files_to_cache(int verbose, const char *prefix, const char **pathspec);
+#define ADD_FILES_VERBOSE	01
+#define ADD_FILES_IGNORE_ERRORS	02
+/*
+ * return 0 if success, 1 - if addition of a file failed and
+ * ADD_FILES_IGNORE_ERRORS was specified in flags
+ */
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
-- 
1.5.4.3.391.gf5a0c
Alex Riesen· Mar 2, 2008, 15:44 UTC · re: Alex Riesen · lore

[PATCH] Add --ignore-errors to git-add to allow it to skip files with read errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 Documentation/git-add.txt |    7 ++++++-
 builtin-add.c             |   11 +++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 4779909..9360a4f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [-u] [--refresh]
-          [--] <filepattern>...
+	  [--ignore-errors] [--] <filepattern>...
 
 DESCRIPTION
 -----------
@@ -81,6 +81,11 @@ OPTIONS
 	Don't add the file(s), but only refresh their stat()
 	information in the index.
 
+\--ignore-errors::
+	If some files could not be added because of errors indexing
+	them, do not abort the operation, but continue adding the
+	others. The command shall still exit with non-zero status.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin-add.c b/builtin-add.c
index bc55a0e..b67ad3f 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -191,6 +191,7 @@ 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;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only),
@@ -201,6 +202,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
+	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
 	OPT_END(),
 };
 
@@ -231,6 +233,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 		if (verbose)
 			flags |= ADD_FILES_VERBOSE;
+		if (ignore_add_errors)
+			flags |= ADD_FILES_IGNORE_ERRORS;
 
 		exit_status = add_files_to_cache(prefix, pathspec, flags);
 		goto finish;
@@ -286,8 +290,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < dir.nr; i++)
-		if (add_file_to_cache(dir.entries[i]->name, verbose))
-			exit(1);
+		if (add_file_to_cache(dir.entries[i]->name, verbose)) {
+			if (!ignore_add_errors)
+				exit(1);
+			exit_status = 1;
+		}
 
  finish:
 	if (active_cache_changed) {
-- 
1.5.4.3.391.gf5a0c
Alex Riesen· Mar 2, 2008, 15:44 UTC · re: Alex Riesen · lore

[PATCH] Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..ca3e33d 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose --ignore-errors .
+	git ls-files |grep foo1
+'
+
 test_done
-- 
1.5.4.3.391.gf5a0c
Johannes Schindelin· Mar 2, 2008, 15:57 UTC · re: Alex Riesen · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Hi,
On Sun, 2 Mar 2008, Alex Riesen wrote:
> -			add_file_to_cache(path, verbose);
> +			if (add_file_to_cache(path, verbose))
> +				exit(1);

Does it really, really _have_ to be exit(1)? I mean, now you block even the faintest chance that we can libify libgit.a by overriding die_routine.

A "return -1" might make _much_ more sense, too.

Ciao, Dscho

Junio C Hamano· Mar 2, 2008, 16:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> On Sun, 2 Mar 2008, Alex Riesen wrote:
>
>> -			add_file_to_cache(path, verbose);
>> +			if (add_file_to_cache(path, verbose))
>> +				exit(1);
>
> Does it really, really _have_ to be exit(1)?  I mean, now you block even 
> the faintest chance that we can libify libgit.a by overriding die_routine.

I think Alex did so not to break the existing scripts that rely on these dying, but it should have been exit(128) to really stay compatible.

Why is this even needed to begin with? I am aware of Dirk's original issue discussed elsewhere, but we try fairly hard to be A-O-N when we can afford to, and this option deliberately breaks it. What is the real reason why such an unreadable (either for privilege or for I/O error) file should not live in .gitignore?

Alex Riesen· Mar 2, 2008, 21:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 2 Mar 2008, Alex Riesen wrote:
> >
> >> -			add_file_to_cache(path, verbose);
> >> +			if (add_file_to_cache(path, verbose))
> >> +				exit(1);
> >
> > Does it really, really _have_ to be exit(1)?  I mean, now you block even 
> > the faintest chance that we can libify libgit.a by overriding die_routine.
> 
> I think Alex did so not to break the existing scripts that rely on these
> dying, but it should have been exit(128) to really stay compatible.

Sorry, this time it was actually mostly accident. I just selected the first non-zero.

Show 5 quoted lines
> Why is this even needed to begin with?  I am aware of Dirk's original
> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
> afford to, and this option deliberately breaks it.  What is the real
> reason why such an unreadable (either for privilege or for I/O error)
> file should not live in .gitignore?

Another program keeps the file open. There is an exclusive mode for opening files, which locks the files for everyone. I believe it is even default mode, unless selected otherwise.

Joachim B Haga· Mar 2, 2008, 22:04 UTC · re: Alex Riesen · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Alex Riesen <raa.lkml@gmail.com> writes:
Show 10 quoted lines
> Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
>> Why is this even needed to begin with?  I am aware of Dirk's original
>> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
>> afford to, and this option deliberately breaks it.  What is the real
>> reason why such an unreadable (either for privilege or for I/O error)
>> file should not live in .gitignore?
>
> Another program keeps the file open. There is an exclusive mode for
> opening files, which locks the files for everyone. I believe it is
> even default mode, unless selected otherwise.

Another minor annoyance in this area, is when a wildcard add fails because of ignored files:

  potassium ~/svn/Deformation|master 0$ ls EpetraMatrix.*
  EpetraMatrix.cpp  EpetraMatrix.cpp~  EpetraMatrix.h  EpetraMatrix.o
  potassium ~/svn/Deformation|master 0$ git add EpetraMatrix.*
  The following paths are ignored by one of your .gitignore files:
  EpetraMatrix.cpp~
  EpetraMatrix.o
  Use -f if you really want to add them.
  potassium ~/svn/New-Deformation|master 0$ git status
  # On branch master
  # Changed but not updated:
  #   (use "git add <file>..." to update what will be committed)
  #
  [...]

I don't want to add them, I just want to ignore them completely (i.e., add the un-ignored ones).

Would this case also be covered by the new switch?
-j.
Alex Riesen· Mar 3, 2008, 06:57 UTC · re: Joachim B Haga · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Joachim B Haga, Sun, Mar 02, 2008 23:04:43 +0100:
Show 33 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> > Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
> >> Why is this even needed to begin with?  I am aware of Dirk's original
> >> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
> >> afford to, and this option deliberately breaks it.  What is the real
> >> reason why such an unreadable (either for privilege or for I/O error)
> >> file should not live in .gitignore?
> >
> > Another program keeps the file open. There is an exclusive mode for
> > opening files, which locks the files for everyone. I believe it is
> > even default mode, unless selected otherwise.
> 
> Another minor annoyance in this area, is when a wildcard add fails
> because of ignored files:
> 
>   potassium ~/svn/Deformation|master 0$ ls EpetraMatrix.*
>   EpetraMatrix.cpp  EpetraMatrix.cpp~  EpetraMatrix.h  EpetraMatrix.o
>   potassium ~/svn/Deformation|master 0$ git add EpetraMatrix.*
>   The following paths are ignored by one of your .gitignore files:
>   EpetraMatrix.cpp~
>   EpetraMatrix.o
>   Use -f if you really want to add them.
>   potassium ~/svn/New-Deformation|master 0$ git status
>   # On branch master
>   # Changed but not updated:
>   #   (use "git add <file>..." to update what will be committed)
>   #
>   [...]
> 
> I don't want to add them, I just want to ignore them completely (i.e., 
> add the un-ignored ones).
> 
> Would this case also be covered by the new switch?
No. This is entirely different usability issue
Alex Riesen· May 12, 2008, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
Show 12 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > On Sun, 2 Mar 2008, Alex Riesen wrote:
> >
> >> -			add_file_to_cache(path, verbose);
> >> +			if (add_file_to_cache(path, verbose))
> >> +				exit(1);
> >
> > Does it really, really _have_ to be exit(1)?  I mean, now you block even 
> > the faintest chance that we can libify libgit.a by overriding die_routine.
> 
> I think Alex did so not to break the existing scripts that rely on these
> dying, but it should have been exit(128) to really stay compatible.

I corrected the series to use die() again and rebased it off current master (65ea3b8c). So it is more compatible with libification (does not hinder it more than previos code) and keep the exit code.

Alex Riesen· May 12, 2008, 17:57 UTC · re: Alex Riesen · lore

[PATCH] Make the exit code of add_file_to_index actually useful

Update the programs which used the function (as add_file_to_cache).
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-add.c    |    6 ++++--
 builtin-commit.c |    7 ++++---
 builtin-mv.c     |    3 ++-
 read-cache.c     |    6 +++---
 4 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index 4a91e3e..4d72ab6 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -94,7 +94,8 @@ static void update_callback(struct diff_queue_struct *q,
 		case DIFF_STATUS_UNMERGED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
-			add_file_to_cache(path, verbose);
+			if (add_file_to_cache(path, verbose))
+				die("updating files failed");
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
@@ -254,7 +255,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < dir.nr; i++)
-		add_file_to_cache(dir.entries[i]->name, verbose);
+		if (add_file_to_cache(dir.entries[i]->name, verbose))
+			die("adding files failed");
 
  finish:
 	if (active_cache_changed) {
diff --git a/builtin-commit.c b/builtin-commit.c
index a65c2b8..ae29d35 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -178,9 +178,10 @@ static void add_remove_files(struct path_list *list)
 		struct stat st;
 		struct path_list_item *p = &(list->items[i]);
 
-		if (!lstat(p->path, &st))
-			add_to_cache(p->path, &st, 0);
-		else
+		if (!lstat(p->path, &st)) {
+			if (add_to_cache(p->path, &st, 0))
+				die("updating files failed");
+		} else
 			remove_file_from_cache(p->path);
 	}
 }
diff --git a/builtin-mv.c b/builtin-mv.c
index 94f6dd2..fb8ffb4 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -256,7 +256,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 
 		for (i = 0; i < added.nr; i++) {
 			const char *path = added.items[i].path;
-			add_file_to_cache(path, verbose);
+			if (add_file_to_cache(path, verbose))
+				die("updating index entries failed");
 		}
 
 		for (i = 0; i < deleted.nr; i++)
diff --git a/read-cache.c b/read-cache.c
index 0382804..8b467f8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -470,7 +470,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	unsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_RACY_IS_DIRTY;
 
 	if (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))
-		die("%s: can only add regular files, symbolic links or git-directories", path);
+		return error("%s: can only add regular files, symbolic links or git-directories", path);
 
 	namelen = strlen(path);
 	if (S_ISDIR(st_mode)) {
@@ -505,12 +505,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 		return 0;
 	}
 	if (index_path(ce->sha1, path, st, 1))
-		die("unable to index file %s", path);
+		return error("unable to index file %s", path);
 	if (ignore_case && alias && different_name(ce, alias))
 		ce = create_alias_ce(ce, alias);
 	ce->ce_flags |= CE_ADDED;
 	if (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
-		die("unable to add %s to index",path);
+		return error("unable to add %s to index",path);
 	if (verbose)
 		printf("add '%s'\n", path);
 	return 0;
-- 
1.5.5.1.184.g5bee
Alex Riesen· May 12, 2008, 17:58 UTC · re: Alex Riesen · lore

[PATCH] Extend interface of add_files_to_cache to allow ignore indexing errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-add.c      |   37 ++++++++++++++++++++++++++++---------
 builtin-checkout.c |    2 +-
 builtin-commit.c   |    2 +-
 cache.h            |    8 +++++++-
 4 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index 4d72ab6..7862808 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -79,12 +79,18 @@ static void fill_directory(struct dir_struct *dir, const char **pathspec,
 		prune_directory(dir, pathspec, baselen);
 }
 
+struct update_callback_data
+{
+	int flags;
+	int add_errors;
+};
+
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
-	int i, verbose;
+	int i;
+	struct update_callback_data *data = cbdata;
 
-	verbose = *((int *)cbdata);
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
@@ -94,28 +100,35 @@ static void update_callback(struct diff_queue_struct *q,
 		case DIFF_STATUS_UNMERGED:
 		case DIFF_STATUS_MODIFIED:
 		case DIFF_STATUS_TYPE_CHANGED:
-			if (add_file_to_cache(path, verbose))
-				die("updating files failed");
+			if (add_file_to_cache(path, data->flags & ADD_FILES_VERBOSE)) {
+				if (!(data->flags & ADD_FILES_IGNORE_ERRORS))
+					die("updating files failed");
+				data->add_errors++;
+			}
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
-			if (verbose)
+			if (data->flags & ADD_FILES_VERBOSE)
 				printf("remove '%s'\n", path);
 			break;
 		}
 	}
 }
 
-void add_files_to_cache(int verbose, const char *prefix, const char **pathspec)
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
+	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.prune_data = pathspec;
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	rev.diffopt.format_callback_data = &verbose;
+	data.flags = flags;
+	data.add_errors = 0;
+	rev.diffopt.format_callback_data = &data;
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	return !!data.add_errors;
 }
 
 static void refresh(int verbose, const char **pathspec)
@@ -193,6 +206,7 @@ static struct option builtin_add_options[] = {
 
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
+	int exit_status = 0;
 	int i, newfd;
 	const char **pathspec;
 	struct dir_struct dir;
@@ -209,11 +223,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	newfd = hold_locked_index(&lock_file, 1);
 
 	if (take_worktree_changes) {
+		int flags = 0;
 		const char **pathspec;
 		if (read_cache() < 0)
 			die("index file corrupt");
 		pathspec = get_pathspec(prefix, argv);
-		add_files_to_cache(verbose, prefix, pathspec);
+
+		if (verbose)
+			flags |= ADD_FILES_VERBOSE;
+
+		exit_status = add_files_to_cache(prefix, pathspec, flags);
 		goto finish;
 	}
 
@@ -265,5 +284,5 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			die("Unable to write new index file");
 	}
 
-	return 0;
+	return exit_status;
 }
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 10ec137..05c0642 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -282,7 +282,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 			 * entries in the index.
 			 */
 
-			add_files_to_cache(0, NULL, NULL);
+			add_files_to_cache(NULL, NULL, 0);
 			work = write_tree_from_memory();
 
 			ret = reset_to_new(new->commit->tree, opts->quiet);
diff --git a/builtin-commit.c b/builtin-commit.c
index ae29d35..6a2f5c3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -246,7 +246,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	 */
 	if (all || (also && pathspec && *pathspec)) {
 		int fd = hold_locked_index(&index_lock, 1);
-		add_files_to_cache(0, also ? prefix : NULL, pathspec);
+		add_files_to_cache(also ? prefix : NULL, pathspec, 0);
 		refresh_cache(REFRESH_QUIET);
 		if (write_cache(fd, active_cache, active_nr) ||
 		    close_lock_file(&index_lock))
diff --git a/cache.h b/cache.h
index 9cee9a5..4fb6290 100644
--- a/cache.h
+++ b/cache.h
@@ -781,7 +781,13 @@ extern int convert_to_git(const char *path, const char *src, size_t len,
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */
-void add_files_to_cache(int verbose, const char *prefix, const char **pathspec);
+#define ADD_FILES_VERBOSE	01
+#define ADD_FILES_IGNORE_ERRORS	02
+/*
+ * return 0 if success, 1 - if addition of a file failed and
+ * ADD_FILES_IGNORE_ERRORS was specified in flags
+ */
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
-- 
1.5.5.1.184.g5bee
Alex Riesen· May 12, 2008, 17:58 UTC · re: Alex Riesen · lore

[PATCH] Add --ignore-errors to git-add to allow it to skip files with read errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 Documentation/git-add.txt |    7 ++++++-
 builtin-add.c             |   11 +++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index e0e730b..bb4abe2 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-add' [-n] [-v] [-f] [--interactive | -i] [--patch | -p] [-u] [--refresh]
-          [--] <filepattern>...
+	  [--ignore-errors] [--] <filepattern>...
 
 DESCRIPTION
 -----------
@@ -83,6 +83,11 @@ OPTIONS
 	Don't add the file(s), but only refresh their stat()
 	information in the index.
 
+\--ignore-errors::
+	If some files could not be added because of errors indexing
+	them, do not abort the operation, but continue adding the
+	others. The command shall still exit with non-zero status.
+
 \--::
 	This option can be used to separate command-line options from
 	the list of files, (useful when filenames might be mistaken
diff --git a/builtin-add.c b/builtin-add.c
index 7862808..522519e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -191,6 +191,7 @@ 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;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only),
@@ -201,6 +202,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
 	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update tracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
+	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
 	OPT_END(),
 };
 
@@ -231,6 +233,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 		if (verbose)
 			flags |= ADD_FILES_VERBOSE;
+		if (ignore_add_errors)
+			flags |= ADD_FILES_IGNORE_ERRORS;
 
 		exit_status = add_files_to_cache(prefix, pathspec, flags);
 		goto finish;
@@ -274,8 +278,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < dir.nr; i++)
-		if (add_file_to_cache(dir.entries[i]->name, verbose))
-			die("adding files failed");
+		if (add_file_to_cache(dir.entries[i]->name, verbose)) {
+			if (!ignore_add_errors)
+				die("adding files failed");
+			exit_status = 1;
+		}
 
  finish:
 	if (active_cache_changed) {
-- 
1.5.5.1.184.g5bee
Alex Riesen· May 12, 2008, 17:58 UTC · re: Alex Riesen · lore

[PATCH] Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..ca3e33d 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose --ignore-errors .
+	git ls-files |grep foo1
+'
+
 test_done
-- 
1.5.5.1.184.g5bee
Alex Riesen· May 12, 2008, 17:59 UTC · re: Alex Riesen · lore

[PATCH] Add a config option to ignore errors for git-add

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-add.c  |   11 ++++++++++-
 t/t3700-add.sh |   12 ++++++++++++
 2 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index 522519e..73235ed 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -206,6 +206,15 @@ static struct option builtin_add_options[] = {
 	OPT_END(),
 };
 
+static int add_config(const char *var, const char *value)
+{
+	if (!strcasecmp(var, "add.ignore-errors")) {
+		ignore_add_errors = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value);
+}
+
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -220,7 +229,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (add_interactive)
 		exit(interactive_add(argc, argv, prefix));
 
-	git_config(git_default_config);
+	git_config(add_config);
 
 	newfd = hold_locked_index(&lock_file, 1);
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index ca3e33d..08f1641 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -188,4 +188,16 @@ test_expect_success 'git add --ignore-errors' '
 	git ls-files |grep foo1
 '
 
+rm -f foo2
+
+test_expect_success 'git add (add.ignore-errors)' '
+	git config add.ignore-errors 1 &&
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	git add --verbose .
+	git ls-files |grep foo1
+'
+
 test_done
-- 
1.5.5.1.184.g5bee
Junio C Hamano· May 13, 2008, 03:48 UTC · re: Alex Riesen · lore

Re: [PATCH] Add a test for git-add --ignore-errors

Alex Riesen <raa.lkml@gmail.com> writes:
Show 23 quoted lines
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>  t/t3700-add.sh |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 287e058..ca3e33d 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
>  	test -z "`git diff-index HEAD -- foo`"
>  '
>  
> +test_expect_success 'git add --ignore-errors' '
> +	git reset --hard &&
> +	date >foo1 &&
> +	date >foo2 &&
> +	chmod 0 foo2 &&
> +	git add --verbose --ignore-errors .
> +	git ls-files |grep foo1
> +'
> +
>  test_done

I like the fact that you added --ignore-errors and made it still error out when it cannot read some files. Shouldn't we be testing it here with "must-fail"?

Alex Riesen· May 13, 2008, 06:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Junio C Hamano, Tue, May 13, 2008 05:48:22 +0200:
Show 15 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> > +test_expect_success 'git add --ignore-errors' '
> > +	git reset --hard &&
> > +	date >foo1 &&
> > +	date >foo2 &&
> > +	chmod 0 foo2 &&
> > +	git add --verbose --ignore-errors .
> > +	git ls-files |grep foo1
> > +'
> > +
> >  test_done
> 
> I like the fact that you added --ignore-errors and made it still error out
> when it cannot read some files.  Shouldn't we be testing it here with
> "must-fail"?
Yes. Would you mind replacing that patch with this one?
 t/t3700-add.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..17ab05a 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose --ignore-errors . &&
+	git ls-files |grep foo1
+'
+
 test_done
-- 
1.5.5.1.206.g7103c
Junio C Hamano· May 13, 2008, 06:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add a test for git-add --ignore-errors

Junio C Hamano <junio@pobox.com> writes:
Show 29 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
>
>> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
>> ---
>>  t/t3700-add.sh |    9 +++++++++
>>  1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
>> index 287e058..ca3e33d 100755
>> --- a/t/t3700-add.sh
>> +++ b/t/t3700-add.sh
>> @@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
>>  	test -z "`git diff-index HEAD -- foo`"
>>  '
>>  
>> +test_expect_success 'git add --ignore-errors' '
>> +	git reset --hard &&
>> +	date >foo1 &&
>> +	date >foo2 &&
>> +	chmod 0 foo2 &&
>> +	git add --verbose --ignore-errors .
>> +	git ls-files |grep foo1
>> +'
>> +
>>  test_done
>
> I like the fact that you added --ignore-errors and made it still error out
> when it cannot read some files.  Shouldn't we be testing it here with
> "must-fail"?

It is human nature to get too enthusiastic demonstrating his own shiny new toy and forget to check that it does not kick in when not asked. There is no test for a case to make sure "git add" fails when foo2 is not readable and does not add "foo1".

Here is a replacement I've queued. I have a similar addition to the test in the patch after this one that demonstrates the configuration variable. I added tests to check the case when the variable is set to false.

-- >8 -- Add a test for git-add --ignore-errors

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3700-add.sh |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 287e058..01e4d62 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -179,4 +179,26 @@ test_expect_success 'git add --refresh' '
 	test -z "`git diff-index HEAD -- foo`"
 '
 
+test_expect_success 'git add should fail atomically upon an unreadable file' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose . &&
+	! ( git ls-files foo1 | grep foo1 )
+'
+
+rm -f foo2
+
+test_expect_success 'git add --ignore-errors' '
+	git reset --hard &&
+	date >foo1 &&
+	date >foo2 &&
+	chmod 0 foo2 &&
+	test_must_fail git add --verbose --ignore-errors . &&
+	git ls-files foo1 | grep foo1
+'
+
+rm -f foo2
+
 test_done
-- 
1.5.5.1.340.g39dc6
Alex Riesen· May 13, 2008, 22:28 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add a test for git-add --ignore-errors

Junio C Hamano, Tue, May 13, 2008 08:05:01 +0200:
Show 36 quoted lines
> Junio C Hamano <junio@pobox.com> writes:
> 
> > Alex Riesen <raa.lkml@gmail.com> writes:
> >
> >> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> >> ---
> >>  t/t3700-add.sh |    9 +++++++++
> >>  1 files changed, 9 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> >> index 287e058..ca3e33d 100755
> >> --- a/t/t3700-add.sh
> >> +++ b/t/t3700-add.sh
> >> @@ -179,4 +179,13 @@ test_expect_success 'git add --refresh' '
> >>  	test -z "`git diff-index HEAD -- foo`"
> >>  '
> >>  
> >> +test_expect_success 'git add --ignore-errors' '
> >> +	git reset --hard &&
> >> +	date >foo1 &&
> >> +	date >foo2 &&
> >> +	chmod 0 foo2 &&
> >> +	git add --verbose --ignore-errors .
> >> +	git ls-files |grep foo1
> >> +'
> >> +
> >>  test_done
> >
> > I like the fact that you added --ignore-errors and made it still error out
> > when it cannot read some files.  Shouldn't we be testing it here with
> > "must-fail"?
> 
> It is human nature to get too enthusiastic demonstrating his own shiny new
> toy and forget to check that it does not kick in when not asked.  There is
> no test for a case to make sure "git add" fails when foo2 is not readable
> and does not add "foo1".
Yes, I missed that. Exactly for the reason :)
> Here is a replacement I've queued.  I have a similar addition to the test
> in the patch after this one that demonstrates the configuration variable.
> I added tests to check the case when the variable is set to false.
Thanks.
> -- >8 --
> Add a test for git-add --ignore-errors
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
Acked-by: Alex Riesen <raa.lkml@gmail.com>
Junio C Hamano· May 12, 2008, 18:42 UTC · re: Alex Riesen · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Alex Riesen <raa.lkml@gmail.com> writes:
Show 17 quoted lines
> Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > On Sun, 2 Mar 2008, Alex Riesen wrote:
>> >
>> >> -			add_file_to_cache(path, verbose);
>> >> +			if (add_file_to_cache(path, verbose))
>> >> +				exit(1);
>> >
>> > Does it really, really _have_ to be exit(1)?  I mean, now you block even 
>> > the faintest chance that we can libify libgit.a by overriding die_routine.
>> 
>> I think Alex did so not to break the existing scripts that rely on these
>> dying, but it should have been exit(128) to really stay compatible.
>
> I corrected the series to use die() again and rebased it off current
> master (65ea3b8c). So it is more compatible with libification (does
> not hinder it more than previos code) and keep the exit code.
But you did not answer my question in the part you did not quote, did you?

Now when somebody either forgets to check the return value from this function, or deliberately ignores it, the resulting index will not match what the code is told to update it with.

Alex Riesen· May 12, 2008, 20:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Junio C Hamano, Mon, May 12, 2008 20:42:58 +0200:
Show 6 quoted lines
> >
> > I corrected the series to use die() again and rebased it off current
> > master (65ea3b8c). So it is more compatible with libification (does
> > not hinder it more than previos code) and keep the exit code.
> 
> But you did not answer my question in the part you did not quote, did you?
I believe I did:
Date:	Sun, 2 Mar 2008 22:42:41 +0100
Subject: Re: [PATCH] Make the exit code of add_file_to_index actually useful
Message-ID: <20080302214241.GB13954@steel.home>
Junio C Hamano, Sun, Mar 02, 2008 17:59:13 +0100:
Show 5 quoted lines
> Why is this even needed to begin with?  I am aware of Dirk's original
> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
> afford to, and this option deliberately breaks it.  What is the real
> reason why such an unreadable (either for privilege or for I/O error)
> file should not live in .gitignore?

Another program keeps the file open. There is an exclusive mode for opening files, which locks the files for everyone. I believe it is even default mode, unless selected otherwise.

> Now when somebody either forgets to check the return value from this
> function, or deliberately ignores it, the resulting index will not match
> what the code is told to update it with.
I think I got them all in the current code:
    $git grep -E 'add_(file_)?to_(index|cache)'
    Documentation/technical/api-in-core-index.txt:* add_file_to_index()
    builtin-add.c:                  if (add_file_to_cache(path, data->flags
    builtin-add.c:          if (add_file_to_cache(dir.entries[i]->name, ver
    builtin-commit.c:                       if (add_to_cache(p->path, &st,
    builtin-mv.c:                   if (add_file_to_cache(path, verbose))
    cache.h:#define add_to_cache(path, st, verbose) add_to_index(&the_index
    cache.h:#define add_file_to_cache(path, verbose) add_file_to_index(&the
    cache.h:extern int add_to_index(struct index_state *, const char *path,
    cache.h:extern int add_file_to_index(struct index_state *, const char *
    read-cache.c:int add_to_index(struct index_state *istate, const char *p
    read-cache.c:int add_file_to_index(struct index_state *istate, const ch
    read-cache.c:   return add_to_index(istate, path, &st, verbose);

Regarding the return value: isn't it very often a bug to ignore them? Or do you mean to say I should have renamed the function so that old interface cannot be used accidentally by someone how just knows it never returns in case of an error?

Junio C Hamano· May 12, 2008, 22:19 UTC · re: Alex Riesen · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Alex Riesen <raa.lkml@gmail.com> writes:
Show 9 quoted lines
>> Why is this even needed to begin with?  I am aware of Dirk's original
>> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
>> afford to, and this option deliberately breaks it.  What is the real
>> reason why such an unreadable (either for privilege or for I/O error)
>> file should not live in .gitignore?
>
> Another program keeps the file open. There is an exclusive mode for
> opening files, which locks the files for everyone. I believe it is
> even default mode, unless selected otherwise.

I would understand there can be some files that cannot be read. But when there is such a file, why is it Ok to ignore an error to update the contents from that file if/when the user asks to index the current contents, provided if the contents of that file is to be tracked? Isn't it the true cause of the problem that the file is being tracked but it shouldn't?

Show 5 quoted lines
>> Now when somebody either forgets to check the return value from this
>> function, or deliberately ignores it, the resulting index will not match
>> what the code is told to update it with.
>
> I think I got them all in the current code...

Not checking the return code from this function that now diagnoses and returns error code is a bug as you said, and the codebase after your patch may not have that bug.

But mistakes happen.

That is why I am asking why it is Ok to sometimes ignore the error to begin with. If we do not need to ignore this condition, then new callers have one less thing to worry about, and we would have one less cause of an unnecessary bug.

Alex Riesen· May 12, 2008, 22:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Junio C Hamano, Tue, May 13, 2008 00:19:42 +0200:
Show 18 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> >> Why is this even needed to begin with?  I am aware of Dirk's original
> >> issue discussed elsewhere, but we try fairly hard to be A-O-N when we can
> >> afford to, and this option deliberately breaks it.  What is the real
> >> reason why such an unreadable (either for privilege or for I/O error)
> >> file should not live in .gitignore?
> >
> > Another program keeps the file open. There is an exclusive mode for
> > opening files, which locks the files for everyone. I believe it is
> > even default mode, unless selected otherwise.
> 
> I would understand there can be some files that cannot be read.  But when
> there is such a file, why is it Ok to ignore an error to update the
> contents from that file if/when the user asks to index the current
> contents, provided if the contents of that file is to be tracked?  Isn't
> it the true cause of the problem that the file is being tracked but it
> shouldn't?

No, I don't think so. Consider "git add dir/". It is _not_ 1 (one) operation. It is many operations (add every file in the "dir/"). Why should all of them be considered failed just because the third file from the bottom could not be read (and the user may have not even seen it, because it wasn't there before, like a temporary file from Excel). And for a user (for me, at least) "git add" is an intermediate operation anyway: there'll be a review in form "git status" or "git commit" afterwards. And there was a clear (sadly invisible with --verbose) warning regarding some file having problems.

Show 16 quoted lines
> >> Now when somebody either forgets to check the return value from this
> >> function, or deliberately ignores it, the resulting index will not match
> >> what the code is told to update it with.
> >
> > I think I got them all in the current code...
> 
> Not checking the return code from this function that now diagnoses and
> returns error code is a bug as you said, and the codebase after your patch
> may not have that bug.
> 
> But mistakes happen.
> 
> That is why I am asking why it is Ok to sometimes ignore the error to
> begin with.  If we do not need to ignore this condition, then new callers
> have one less thing to worry about, and we would have one less cause of an
> unnecessary bug.

For the reasons outlined? Where the user is in a situation when he has to override the safety checks. Just because it is more convenient to type --ignore-errors than edit .gitignore and add there a whimsical patterns like "~*.xls", which one day have to be overridden because that project got an excel file which begins with "~"?

I am not suggesting making it default. And actually, the last patch, with a config for add.ignore-errors option, better be ignored - it was just too simple to code up. I never used the option. I had cases for --ignore-errors, sadly.

Junio C Hamano· May 12, 2008, 23:32 UTC · re: Alex Riesen · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Alex Riesen <raa.lkml@gmail.com> writes:
Show 16 quoted lines
> Junio C Hamano, Tue, May 13, 2008 00:19:42 +0200:
> ...
>> I would understand there can be some files that cannot be read.  But when
>> there is such a file, why is it Ok to ignore an error to update the
>> contents from that file if/when the user asks to index the current
>> contents, provided if the contents of that file is to be tracked?  Isn't
>> it the true cause of the problem that the file is being tracked but it
>> shouldn't?
>
> No, I don't think so. Consider "git add dir/". It is _not_ 1 (one)
> operation. It is many operations (add every file in the "dir/"). Why
> should all of them be considered failed just because the third file
> from the bottom could not be read (and the user may have not even seen
> it, because it wasn't there before, like a temporary file from Excel).
> And for a user (for me, at least) "git add" is an intermediate
> operation anyway...

Ah, Ok, I was overly cautious, and the worry is unfounded, as long as you do not trigger this "ignore" thing upon "git commit -a".

Thanks.  Will queue.
Alex Riesen· May 13, 2008, 06:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

Junio C Hamano, Tue, May 13, 2008 01:32:19 +0200:
Show 21 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Junio C Hamano, Tue, May 13, 2008 00:19:42 +0200:
> > ...
> >> I would understand there can be some files that cannot be read.  But when
> >> there is such a file, why is it Ok to ignore an error to update the
> >> contents from that file if/when the user asks to index the current
> >> contents, provided if the contents of that file is to be tracked?  Isn't
> >> it the true cause of the problem that the file is being tracked but it
> >> shouldn't?
> >
> > No, I don't think so. Consider "git add dir/". It is _not_ 1 (one)
> > operation. It is many operations (add every file in the "dir/"). Why
> > should all of them be considered failed just because the third file
> > from the bottom could not be read (and the user may have not even seen
> > it, because it wasn't there before, like a temporary file from Excel).
> > And for a user (for me, at least) "git add" is an intermediate
> > operation anyway...
> 
> Ah, Ok, I was overly cautious, and the worry is unfounded, as long as you
> do not trigger this "ignore" thing upon "git commit -a".

Yes, builtin-commit.c explicitely keeps its behaviour: the 0 in flags argument makes sure die() is called. "Ignore errors" must be requested with ADD_FILES_IGNORE_ERRORS.

Daniel Barkalow· Mar 3, 2008, 18:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Make the exit code of add_file_to_index actually useful

On Sun, 2 Mar 2008, Johannes Schindelin wrote:
Show 10 quoted lines
> Hi,
> 
> On Sun, 2 Mar 2008, Alex Riesen wrote:
> 
> > -			add_file_to_cache(path, verbose);
> > +			if (add_file_to_cache(path, verbose))
> > +				exit(1);
> 
> Does it really, really _have_ to be exit(1)?  I mean, now you block even 
> the faintest chance that we can libify libgit.a by overriding die_routine.

It would be handy to have a die_no_message(), for cases like this where a function wants to print an error message but it's up to the caller whether to abort (in the contextually reasonable way).

	-Daniel
*This .sig left intentionally blank*

← back to recent threads