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

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

[Dirk Süsserott]

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

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

[Jeff King]

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

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

[Dirk Süsserott]

Jeff King schrieb:
> 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

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

[Junio C Hamano]

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"

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

[Dirk Süsserott]

Junio C Hamano schrieb:
> 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.

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

[Alex Riesen]

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.

[PATCH] Make the exit code of add_file_to_index actually useful

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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?

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

[Alex Riesen]

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.

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

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

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

[Joachim B Haga]

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?

-j.

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

[Alex Riesen]

Joachim B Haga, Sun, Mar 02, 2008 23:04:43 +0100:
> 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

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

[Alex Riesen]

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.

[PATCH] Make the exit code of add_file_to_index actually useful

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Alex Riesen]

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

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

[Junio C Hamano]

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

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

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Junio C Hamano, Tue, May 13, 2008 05:48:22 +0200:
> 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

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

[Junio C Hamano]

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

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

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

[Alex Riesen]

Junio C Hamano, Tue, May 13, 2008 08:05:01 +0200:
> 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>

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

[Junio C Hamano]

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

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

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

[Alex Riesen]

Junio C Hamano, Mon, May 12, 2008 20:42:58 +0200:
> >
> > 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:
> 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?

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

[Junio C Hamano]

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?

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

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

[Alex Riesen]

Junio C Hamano, Tue, May 13, 2008 00:19:42 +0200:
> 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.

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

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

[Junio C Hamano]

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

Thanks.  Will queue.

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

[Alex Riesen]

Junio C Hamano, Tue, May 13, 2008 01:32:19 +0200:
> 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.

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

[Daniel Barkalow]

On Sun, 2 Mar 2008, Johannes Schindelin wrote:

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