[PATCH/RFC] add: listen to --ignore-errors for submodule-errors

[PATCH/RFC] add: listen to --ignore-errors for submodule-errors

[Erik Faye-Lund]

"git add --ignore-errors -- some-submodule/foo" surprisingly
throws an error saying "fatal: Path 'some-submodule/foo' is in
submodule 'some-submodule/foo'".

Fix this by making sure we consult the flag, and propagate the
error code properly.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

I recently tried to do the following in the msysGit-repo:

 $ git add --ignore-errors -- */.gitignore
 fatal: Path 'src/git-cheetah/.gitignore' is in submodule 'src/git-cheetah'

I was a bit puzzled by this; I explicitly specified --ignore-errors
because I did not want to be stopped due to src/git-cheetah/.gitignore
being located in a submodule.

The documentation seems to suggest that this is what is supposed to
happen, and this seems like the most likely behavior that the user
wanted. After all, there's no good reason submodules are special
in this regard, no?

 builtin/add.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 87446cf..6e6feb0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -20,7 +20,7 @@ static const char * const builtin_add_usage[] = {
 	NULL
 };
 static int patch_interactive, add_interactive, edit_interactive;
-static int take_worktree_changes;
+static int take_worktree_changes, ignore_add_errors;
 
 struct update_callback_data {
 	int flags;
@@ -153,9 +153,9 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 	return seen;
 }
 
-static void treat_gitlinks(const char **pathspec)
+static int treat_gitlinks(const char **pathspec)
 {
-	int i;
+	int i, exit_status = 0;
 
 	if (!pathspec || !*pathspec)
 		return;
@@ -172,12 +172,15 @@ static void treat_gitlinks(const char **pathspec)
 				if (len2 == len + 1)
 					/* strip trailing slash */
 					pathspec[j] = xstrndup(ce->name, len);
-				else
+				else if (!ignore_add_errors)
 					die (_("Path '%s' is in submodule '%.*s'"),
 						pathspec[j], len, ce->name);
+				else
+					exit_status = 1;
 			}
 		}
 	}
+	return exit_status;
 }
 
 static void refresh(int verbose, const char **pathspec)
@@ -312,7 +315,7 @@ static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
+static int addremove, intent_to_add, ignore_missing = 0;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, "dry run"),
@@ -418,7 +421,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
-	treat_gitlinks(pathspec);
+	exit_status |= treat_gitlinks(pathspec);
 
 	if (add_new_files) {
 		int baselen;
-- 
1.7.11.msysgit.0.3.g3006a55

Re: [PATCH/RFC] add: listen to --ignore-errors for submodule-errors

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> I recently tried to do the following in the msysGit-repo:
>
>  $ git add --ignore-errors -- */.gitignore
>  fatal: Path 'src/git-cheetah/.gitignore' is in submodule 'src/git-cheetah'
>
> I was a bit puzzled by this; I explicitly specified --ignore-errors
> because I did not want to be stopped due to src/git-cheetah/.gitignore
> being located in a submodule.

If I recall correctly, originally --ignore-errors was added was by
those who (arguably misguidedly) wanted to randomly run "git add"
that can potentially race with ongoing working tree updates
(i.e. think of a poor-man's unreliable snapshotting filesystem), to
which "git add" will notice that the working tree file it was asked
to index changed while it was reading and error out.  Also on some
systems, "git add" on files that are currently open may not be able
to read from them, which would also cause a run-time error.  The
kind of errors the option was meant to ignore were "these paths are
perfectly OK to add, but for some reason, adding them fails at this
moment, and for the purpose of poor-man's unreliable snapshot, it is
OK not to pick the exact current state up, as we will pick it up the
next round", not your kind of request that will lead to an error of
the "adding this path will break the structural integrity of the
repository and git should error out" kind.

> The documentation seems to suggest that this is what is supposed to
> happen, and this seems like the most likely behavior that the user
> wanted. After all, there's no good reason submodules are special
> in this regard, no?

How does "git add .git" or "git add .git/config" behave with your
patch applied?  It is exactly the same kind of error that breaks the
structural integrity of the repository as adding src/cheetah/.gitignore
to the top-level project repository, and there is no good reason to
special case submodules, either.

Re: [PATCH/RFC] add: listen to --ignore-errors for submodule-errors

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I recently tried to do the following in the msysGit-repo:
>>
>>  $ git add --ignore-errors -- */.gitignore
>>  fatal: Path 'src/git-cheetah/.gitignore' is in submodule 'src/git-cheetah'
>>
>> I was a bit puzzled by this; I explicitly specified --ignore-errors
>> because I did not want to be stopped due to src/git-cheetah/.gitignore
>> being located in a submodule.
>
> If I recall correctly, originally --ignore-errors was added was by
> those who (arguably misguidedly) wanted to randomly run "git add"
> that can potentially race with ongoing working tree updates
> (i.e. think of a poor-man's unreliable snapshotting filesystem), to
> which "git add" will notice that the working tree file it was asked
> to index changed while it was reading and error out.  Also on some
> systems, "git add" on files that are currently open may not be able
> to read from them, which would also cause a run-time error.  The
> kind of errors the option was meant to ignore were "these paths are
> perfectly OK to add, but for some reason, adding them fails at this
> moment, and for the purpose of poor-man's unreliable snapshot, it is
> OK not to pick the exact current state up, as we will pick it up the
> next round", not your kind of request that will lead to an error of
> the "adding this path will break the structural integrity of the
> repository and git should error out" kind.
>
>> The documentation seems to suggest that this is what is supposed to
>> happen, and this seems like the most likely behavior that the user
>> wanted. After all, there's no good reason submodules are special
>> in this regard, no?
>
> How does "git add .git" or "git add .git/config" behave with your
> patch applied?  It is exactly the same kind of error that breaks the
> structural integrity of the repository as adding src/cheetah/.gitignore
> to the top-level project repository, and there is no good reason to
> special case submodules, either.

For that matter, running "git add ../../foo" from the top-level of
the working tree falls into the same category.  There are boundaries
in both upward and downward directions that define the area you
could add to the index.

Now, I am not saying that "--ignore-errors" should _never_ mean to
ignore errors of this kind.  I was merely giving the historical
background for the semantics you are observing.  I personally think
it might even be an improvement if you made the option to instruct
the command to consistently ignore requests to add these paths
outside the working tree boundary in any direction (not just the
downwards boundary defined by the presense of a submodule, but the
downwards boundary defined by the ".git" directory, and the upward
boundary at the root of the working tree), and add only remaining
valid paths to the index.  I do not think it is the right thing to
special case only the submodules, though.

Re: [PATCH/RFC] add: listen to --ignore-errors for submodule-errors

[Erik Faye-Lund]

On Tue, Jun 26, 2012 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I recently tried to do the following in the msysGit-repo:
>>
>>  $ git add --ignore-errors -- */.gitignore
>>  fatal: Path 'src/git-cheetah/.gitignore' is in submodule 'src/git-cheetah'
>>
>> I was a bit puzzled by this; I explicitly specified --ignore-errors
>> because I did not want to be stopped due to src/git-cheetah/.gitignore
>> being located in a submodule.
>
> If I recall correctly, originally --ignore-errors was added was by
> those who (arguably misguidedly) wanted to randomly run "git add"
> that can potentially race with ongoing working tree updates
> (i.e. think of a poor-man's unreliable snapshotting filesystem), to
> which "git add" will notice that the working tree file it was asked
> to index changed while it was reading and error out.  Also on some
> systems, "git add" on files that are currently open may not be able
> to read from them, which would also cause a run-time error.  The
> kind of errors the option was meant to ignore were "these paths are
> perfectly OK to add, but for some reason, adding them fails at this
> moment, and for the purpose of poor-man's unreliable snapshot, it is
> OK not to pick the exact current state up, as we will pick it up the
> next round", not your kind of request that will lead to an error of
> the "adding this path will break the structural integrity of the
> repository and git should error out" kind.
>

Right. I guess my confusion was due to the documentation wording "If
some files could not be added *because of errors indexing them*, do
not abort the operation". It's not obvious to me that "because if
errors indexing them" means what you describe above.

>> The documentation seems to suggest that this is what is supposed to
>> happen, and this seems like the most likely behavior that the user
>> wanted. After all, there's no good reason submodules are special
>> in this regard, no?
>
> How does "git add .git" or "git add .git/config" behave with your
> patch applied?  It is exactly the same kind of error that breaks the
> structural integrity of the repository as adding src/cheetah/.gitignore
> to the top-level project repository, and there is no good reason to
> special case submodules, either.

Both "git add .git" and "git add --ignore-errors .git" returned
silently without error even before my patch.

"git add .git/config" outputs
"error: Invalid path '.git/config'
error: unable to add .git/config to index
fatal: adding files failed"

...and "git add --ignore-errors .git/config" outputs
"error: Invalid path '.git/config'
error: unable to add .git/config to index"

...this is both before and after my patch. I didn't touch that part of
the code-path, so this already treats these errors as non-fatal when
--ignore-errors are specified.

I simply found a new code-path where errors weren't ignored, but there
might of course be more. However, the lack of a "fatal: adding files
failed" for files inside .git seems to suggest to me that even though
the intent might be what you describe above, that is not actually what
the code does.

Of course, silently ignoring "git add .git" seems odd to me.