git add -u nonexistent-file

Hi,


$ git --version
git version 1.7.0.rc1.84.g9879
$ git add -u nonexistent-file
$ echo $?
0

No error message, no error in exit status.

Is it OK this way?  Why?


Best,
Gábor

2010/2/8 SZEDER Gábor [off-list ref]:

Hi,


$ git --version
git version 1.7.0.rc1.84.g9879
$ git add -u nonexistent-file
$ echo $?
0

No error message, no error in exit status.

Is it OK this way?  Why?


Best,
Gábor

Hi,

I see the same behaviour with git version 1.6.4.2.

From the man page of git-add

-u, --update
           Update only files that git already knows about, staging
modified content for
           commit and marking deleted files for removal.

So git will only look at files that are already in the repository.

It looks like in the case you've highlighted git is ignoring the extra
non-option parameters on the command line. I'll let other people argue
whether this is by design or omission.

'git add nonexistent-file' works as expected, exiting with an error
message and non-zero exit status.

On Mon, Feb 08, 2010 at 02:12:41PM -0500, Chris Packham wrote:

quoted

$ git add -u nonexistent-file
$ echo $?
0

[...]
It looks like in the case you've highlighted git is ignoring the extra
non-option parameters on the command line. I'll let other people argue
whether this is by design or omission.

It's not ignoring the extra parameters. They limit the scope of the
operation. So:

  $ git init
  $ touch file && mkdir subdir && touch subdir/file
  $ git add . && git commit -m one
  $ echo changes >file && echo changes >subdir/file
  $ git add -u subdir
  $ git status
  # On branch master
  # Changes to be committed:
  #       modified:   subdir/file
  #
  # Changed but not updated:
  #       modified:   file
  #

That being said, you noticed that the regular add case notes unused
pathspecs on the command line:

  $ git add bogus
  fatal: pathspec 'bogus' did not match any files

We could probably do the same here.

-Peff

On Mon, Feb 8, 2010 at 7:39 PM, Jeff King [off-list ref] wrote:

On Mon, Feb 08, 2010 at 02:12:41PM -0500, Chris Packham wrote:

quoted

quoted

$ git add -u nonexistent-file
$ echo $?
0

[...]
It looks like in the case you've highlighted git is ignoring the extra
non-option parameters on the command line. I'll let other people argue
whether this is by design or omission.

It's not ignoring the extra parameters. They limit the scope of the
operation. So:

 $ git init
 $ touch file && mkdir subdir && touch subdir/file
 $ git add . && git commit -m one
 $ echo changes >file && echo changes >subdir/file
 $ git add -u subdir
 $ git status
 # On branch master
 # Changes to be committed:
 #       modified:   subdir/file
 #
 # Changed but not updated:
 #       modified:   file
 #

Yep my bad. I tried the non-existent case but not the "normal" case.
Re reading the man page it makes sense it just happens that the
<filepattern> part scrolls off the top when I get to the -u part.

That being said, you noticed that the regular add case notes unused
pathspecs on the command line:

 $ git add bogus
 fatal: pathspec 'bogus' did not match any files

We could probably do the same here.

I think so. By not having this error it led me to think "OK it just
throws away the pathspec". Gabor rightly thought that it does use it
so shouldn't it be giving an error.

If I get brave enough I could attempt a patch but I wouldn't let that
dissuade anyone that actually knows what they're doing from jumping in
with a patch.

Add a test for 'git add -u pathspec' and 'git add pathspec' where
pathspec does not exist. The expected result is that git add exits with
an error message and an appropriate exit code.

This adds 1 expected failure to t/t2200-add-update.sh.

Signed-off-by: Chris Packham <redacted>
---

If anyone feels like pursuing this heres a patch to two simple tests around
this behaviour. I made an initial attempt to change builtin-add.c to print an
error message and set the exit code to a non-zero value but my naive attempt
broke the normal usage of 'git add pathspec'.

 t/t2200-add-update.sh |    5 +++++
 t/t3700-add.sh        |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 9120750..dbabc3c 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh

@@ -176,4 +176,9 @@ test_expect_success 'add -u resolves unmerged paths' '
 
 '
 
+test_expect_failure 'error out when attempting to add -u non-existent pathspec' '
+	test_must_fail git add -u non-existent &&
+	! (git ls-files | grep "non-existent")
+'
+
 test_done

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 85eb0fb..c77bb71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -255,4 +255,9 @@ test_expect_success 'git add to resolve conflicts on otherwise ignored path' '
 	git add track-this
 '
 
+test_expect_success 'error out when attempting to add non-existent pathspec' '
+	test_must_fail git add non-existent &&
+	! (git ls-files | grep "non-existent")
+'
+
 test_done

-- 
1.6.4.2

Jeff King [off-list ref] writes:

It's not ignoring the extra parameters. They limit the scope of the
operation. So:

  $ git init
  $ touch file && mkdir subdir && touch subdir/file
  $ git add . && git commit -m one
  $ echo changes >file && echo changes >subdir/file
  $ git add -u subdir
  $ git status
  # On branch master
  # Changes to be committed:
  #       modified:   subdir/file
  #
  # Changed but not updated:
  #       modified:   file
  #

That being said, you noticed that the regular add case notes unused
pathspecs on the command line:

  $ git add bogus
  fatal: pathspec 'bogus' did not match any files

We could probably do the same here.

It won't be entirely trivial to do so efficiently but it shouldn't be a
rocket surgery.

Something like this (untested of course)?

 builtin-add.c |   38 +++++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 2705f8d..87d2980 100644
--- a/builtin-add.c
+++ b/builtin-add.c

@@ -117,7 +117,19 @@ static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
 	}
 }
 
-static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+static char *find_used_pathspec(const char **pathspec)
+{
+	char *seen;
+	int i;
+
+	for (i = 0; pathspec[i];  i++)
+		; /* just counting */
+	seen = xcalloc(i, 1);
+	fill_pathspec_matches(pathspec, seen, i);
+	return seen;
+}
+
+static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
 	int i, specs;

@@ -137,13 +149,7 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	}
 	dir->nr = dst - dir->entries;
 	fill_pathspec_matches(pathspec, seen, specs);
-
-	for (i = 0; i < specs; i++) {
-		if (!seen[i] && pathspec[i][0] && !file_exists(pathspec[i]))
-			die("pathspec '%s' did not match any files",
-					pathspec[i]);
-	}
-        free(seen);
+	return seen;
 }
 
 static void treat_gitlinks(const char **pathspec)

@@ -359,6 +365,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int flags;
 	int add_new_files;
 	int require_pathspec;
+	char *seen = NULL;
 
 	git_config(add_config, NULL);
 

@@ -418,7 +425,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		/* This picks up the paths that are not tracked */
 		baselen = fill_directory(&dir, pathspec);
 		if (pathspec)
-			prune_directory(&dir, pathspec, baselen);
+			seen = prune_directory(&dir, pathspec, baselen);
 	}
 
 	if (refresh_only) {

@@ -426,6 +433,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		goto finish;
 	}
 
+	if (pathspec) {
+		int i;
+		if (!seen)
+			seen = find_used_pathspec(pathspec);
+		for (i = 0; pathspec[i]; i++) {
+			if (!seen[i] && pathspec[i][0]
+			    && !file_exists(pathspec[i]))
+				die("pathspec '%s' did not match any files",
+				    pathspec[i]);
+		}
+		free(seen);
+	}
+
 	exit_status |= add_files_to_cache(prefix, pathspec, flags);
 
 	if (add_new_files)

On Tue, Feb 9, 2010 at 4:58 PM, Junio C Hamano [off-list ref] wrote:

It won't be entirely trivial to do so efficiently but it shouldn't be a
rocket surgery.

Something like this (untested of course)?

Passes my new test and all the rest in t2200-add-update.sh and t3700-add.sh.

Hows this for a commit message:

  git add -u: give an error if pathspec unmatched

  If a pathspec is supplied to 'git add -u' and no matching path is
  matched, fail with an approriate error message and exit code.

Add a test for 'git add -u pathspec' and 'git add pathspec' where
pathspec does not exist. The expected result is that git add exits with
an error message and an appropriate exit code.

This adds 1 expected failure to t/t2200-add-update.sh.

Signed-off-by: Chris Packham <redacted>
---
 t/t2200-add-update.sh |    5 +++++
 t/t3700-add.sh        |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 9120750..dbabc3c 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh

@@ -176,4 +176,9 @@ test_expect_success 'add -u resolves unmerged paths' '
 
 '
 
+test_expect_failure 'error out when attempting to add -u non-existent pathspec' '
+	test_must_fail git add -u non-existent &&
+	! (git ls-files | grep "non-existent")
+'
+
 test_done

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 85eb0fb..c77bb71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh

@@ -255,4 +255,9 @@ test_expect_success 'git add to resolve conflicts on otherwise ignored path' '
 	git add track-this
 '
 
+test_expect_success 'error out when attempting to add non-existent pathspec' '
+	test_must_fail git add non-existent &&
+	! (git ls-files | grep "non-existent")
+'
+
 test_done

-- 
1.6.4.2

From: Junio C Hamano <redacted>

If a pathspec is supplied to 'git add -u' and no matching path is
matched, fail with an approriate error message and exit code.

Tested-by: Chris Packham <redacted>
---
 builtin-add.c |   38 +++++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 2705f8d..87d2980 100644
--- a/builtin-add.c
+++ b/builtin-add.c

@@ -117,7 +117,19 @@ static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
 	}
 }
 
-static void prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+static char *find_used_pathspec(const char **pathspec)
+{
+	char *seen;
+	int i;
+
+	for (i = 0; pathspec[i];  i++)
+		; /* just counting */
+	seen = xcalloc(i, 1);
+	fill_pathspec_matches(pathspec, seen, i);
+	return seen;
+}
+
+static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
 	int i, specs;

@@ -137,13 +149,7 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p
 	}
 	dir->nr = dst - dir->entries;
 	fill_pathspec_matches(pathspec, seen, specs);
-
-	for (i = 0; i < specs; i++) {
-		if (!seen[i] && pathspec[i][0] && !file_exists(pathspec[i]))
-			die("pathspec '%s' did not match any files",
-					pathspec[i]);
-	}
-        free(seen);
+	return seen;
 }
 
 static void treat_gitlinks(const char **pathspec)

@@ -359,6 +365,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int flags;
 	int add_new_files;
 	int require_pathspec;
+	char *seen = NULL;
 
 	git_config(add_config, NULL);
 

@@ -418,7 +425,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		/* This picks up the paths that are not tracked */
 		baselen = fill_directory(&dir, pathspec);
 		if (pathspec)
-			prune_directory(&dir, pathspec, baselen);
+			seen = prune_directory(&dir, pathspec, baselen);
 	}
 
 	if (refresh_only) {

@@ -426,6 +433,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		goto finish;
 	}
 
+	if (pathspec) {
+		int i;
+		if (!seen)
+			seen = find_used_pathspec(pathspec);
+		for (i = 0; pathspec[i]; i++) {
+			if (!seen[i] && pathspec[i][0]
+			    && !file_exists(pathspec[i]))
+				die("pathspec '%s' did not match any files",
+				    pathspec[i]);
+		}
+		free(seen);
+	}
+
 	exit_status |= add_files_to_cache(prefix, pathspec, flags);
 
 	if (add_new_files)

-- 
1.6.4.2

Heres the up to date series with Junios fix and my test. The two test patches
could be squashed together if we want to change the order.

This changes the test added in bb73e5c to an expected success now that
Junio has supplied a fix.

Signed-off-by: Chris Packham <redacted>
---
 t/t2200-add-update.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index dbabc3c..6302137 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh

@@ -176,7 +176,7 @@ test_expect_success 'add -u resolves unmerged paths' '
 
 '
 
-test_expect_failure 'error out when attempting to add -u non-existent pathspec' '
+test_expect_success 'error out when attempting to add -u non-existent pathspec' '
 	test_must_fail git add -u non-existent &&
 	! (git ls-files | grep "non-existent")
 '

-- 
1.6.4.2

On Tue, Feb 09, 2010 at 01:58:16PM -0800, Junio C Hamano wrote:

quoted

That being said, you noticed that the regular add case notes unused
pathspecs on the command line:

  $ git add bogus
  fatal: pathspec 'bogus' did not match any files

We could probably do the same here.

It won't be entirely trivial to do so efficiently but it shouldn't be a
rocket surgery.

Something like this (untested of course)?

Looks like Chris gave it some basic testing. I read over the patch
itself, and it looks sane to me.

-Peff