SZEDER GáborHi,
$ 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
SZEDER Gábor
Chris PackhamRe: git add -u nonexistent-file
2010/2/8 SZEDER Gábor <szeder@ira.uka.de>:
Show 17 quoted lines
> 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.
Jeff KingRe: git add -u nonexistent-file
On Mon, Feb 08, 2010 at 02:12:41PM -0500, Chris Packham wrote:
Show 7 quoted lines
> > $ 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
Chris PackhamRe: git add -u nonexistent-file
On Mon, Feb 8, 2010 at 7:39 PM, Jeff King <peff@peff.net> wrote:
Show 26 quoted lines
> On Mon, Feb 08, 2010 at 02:12:41PM -0500, Chris Packham wrote: > >> > $ 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.
Show 7 quoted lines
> 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.
Chris Packham[PATCH] test for add with non-existent pathspec
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 <judge.packham@gmail.com> ---
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
Junio C HamanoRe: git add -u nonexistent-file
Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> 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)
Chris PackhamRe: git add -u nonexistent-file
On Tue, Feb 9, 2010 at 4:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> > 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.
Chris PackhamRe: git add -u nonexistent-file
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.
Chris Packham[PATCH 1/3] test for add with non-existent pathspec
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 <judge.packham@gmail.com> --- 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
Chris Packham[PATCH 2/3] git add -u: give an error if pathspec unmatched
From: Junio C Hamano <gitster@pobox.com>
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 <judge.packham@gmail.com> --- 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
Chris Packham[PATCH 3/3] t2200-add-update.sh: change expected fail to success
This changes the test added in bb73e5c to an expected success now that Junio has supplied a fix.
Signed-off-by: Chris Packham <judge.packham@gmail.com> --- 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
Junio C HamanoRe: git add -u nonexistent-file
Chris Packham <judge.packham@gmail.com> writes:
Show 9 quoted lines
> On Tue, Feb 9, 2010 at 4:58 PM, Junio C Hamano <gitster@pobox.com> 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.
Thanks; will squash two tests into one.
Jeff KingRe: git add -u nonexistent-file
On Tue, Feb 09, 2010 at 01:58:16PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> > 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