[PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[Steven Grimm]

This suppresses the output of "rm" commands, and also exits with a zero
exit code when no files match.  This allows "git rm" (and more importantly,
"git rm -r") to be used as an index filter with cg-admin-rewritehist.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
 Documentation/git-rm.txt |    4 ++++
 builtin-rm.c             |   26 ++++++++++++++++++++------
 t/t3600-rm.sh            |   28 ++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 6feebc0..c354134 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -33,6 +33,10 @@ OPTIONS
         Don't actually remove the file(s), just show if they exist in
         the index.
 
+-q::
+	Don't output the names of the files being removed, and exit
+	with a zero status even if no files matched.
+
 -r::
         Allow recursive removal when a leading directory name is
         given.
diff --git a/builtin-rm.c b/builtin-rm.c
index 8a0738f..3d438de 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -10,7 +10,7 @@
 #include "tree-walk.h"
 
 static const char builtin_rm_usage[] =
-"git-rm [-f] [-n] [-r] [--cached] [--] <file>...";
+"git-rm [-f] [-n] [-q] [-r] [--cached] [--] <file>...";
 
 static struct {
 	int nr, alloc;
@@ -104,7 +104,7 @@ static struct lock_file lock_file;
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	int i, newfd;
-	int show_only = 0, force = 0, index_only = 0, recursive = 0;
+	int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 	const char **pathspec;
 	char *seen;
 
@@ -130,6 +130,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			index_only = 1;
 		else if (!strcmp(arg, "-f"))
 			force = 1;
+		else if (!strcmp(arg, "-q"))
+			quiet = 1;
 		else if (!strcmp(arg, "-r"))
 			recursive = 1;
 		else
@@ -153,14 +155,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	if (pathspec) {
 		const char *match;
+		int seen_any = 0;
 		for (i = 0; (match = pathspec[i]) != NULL ; i++) {
-			if (!seen[i])
-				die("pathspec '%s' did not match any files",
-				    match);
+			if (!seen[i]) {
+				if (! quiet) {
+					die("pathspec '%s' did not match any files",
+					    match);
+				}
+			}
+			else {
+				seen_any = 1;
+			}
 			if (!recursive && seen[i] == MATCHED_RECURSIVELY)
 				die("not removing '%s' recursively without -r",
 				    *match ? match : ".");
 		}
+
+		if (! seen_any)
+			exit(0);
 	}
 
 	/*
@@ -187,7 +199,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 */
 	for (i = 0; i < list.nr; i++) {
 		const char *path = list.name[i];
-		printf("rm '%s'\n", path);
+		if (!quiet) {
+			printf("rm '%s'\n", path);
+		}
 
 		if (remove_file_from_cache(path))
 			die("git-rm: unable to remove %s", path);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index e31cf93..bd72feb 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -84,6 +84,30 @@ test_expect_success \
     'When the rm in "git-rm -f" fails, it should not remove the file from the index' \
     'git-ls-files --error-unmatch baz'
 
+test_expect_success 'Remove nonexistent file with -q returns zero exit status' '
+	git rm -q nonexistent
+'
+
+test_expect_success '"rm" command printed' '
+	echo frotz > test-file &&
+	git add test-file &&
+	git commit -m "add file for rm test" &&
+	git rm test-file > rm-output &&
+	test `egrep "^rm " rm-output | wc -l` = 1 &&
+	rm -f test-file rm-output &&
+	git commit -m "remove file from rm test"
+'
+
+test_expect_success '"rm" command suppressed with -q' '
+	echo frotz > test-file &&
+	git add test-file &&
+	git commit -m "add file for rm -q test" &&
+	git rm -q test-file > rm-output &&
+	test `wc -l < rm-output` = 0 &&
+	rm -f test-file rm-output &&
+	git commit -m "remove file from rm test"
+'
+
 # Now, failure cases.
 test_expect_success 'Re-add foo and baz' '
 	git add foo baz &&
@@ -154,4 +178,8 @@ test_expect_success 'Recursive with -r -f' '
 	! test -d frotz
 '
 
+test_expect_failure 'Remove nonexistent file returns nonzero exit status' '
+	git rm nonexistent
+'
+
 test_done
-- 
1.5.1.1.99.g0ea98

Re: [PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> +-q::
> +	Don't output the names of the files being removed, and exit
> +	with a zero status even if no files matched.
> +

Suppressing output is understandable and probably is a useful
thing to do, but I do not see a justification to tie that
quietness to making the status unuable...

Re: [PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[Steven Grimm]

Junio C Hamano wrote:
> Suppressing output is understandable and probably is a useful
> thing to do, but I do not see a justification to tie that
> quietness to making the status unuable...
>   

The status is unusable as is, actually, for the particular use case of 
cg-admin-rewritehist. If you try to use git-rm as an index filter, 
cg-admin-rewritehist will stop running as soon as you hit a revision 
that doesn't contain the file you're looking to filter out. (If the file 
doesn't exist in the first revision in your repo, that means it will do 
no work at all.)

My justification was that the "-f" option to the normal UNIX "rm" 
command does the same thing: it suppresses output and causes the command 
to exit with a zero status code even if no files match. But "git rm -f" 
was already taken.

If you like I'll be happy to split it into two separate options.

-Steve

Re: [PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> Junio C Hamano wrote:
>
>> Suppressing output is understandable and probably is a useful
>> thing to do, but I do not see a justification to tie that
>> quietness to making the status unuable...
>
> The status is unusable as is, actually, for the particular use case of
> cg-admin-rewritehist. If you try to use git-rm as an index filter,
> cg-admin-rewritehist will stop running as soon as you hit a revision
> that doesn't contain the file you're looking to filter out. (If the
> file doesn't exist in the first revision in your repo, that means it
> will do no work at all.)

Probably "git-grep foo" wouldn't be suitable as the index filter
for admin-rewritehist, either, nor "git-fsck", nor many other
things.

What does it have to do with anything?

Saying "git rm --quiet foo" from the command line, wishing to
supress the output, is very understandable.

Saying "git rm --ignore-unmatch foo bar baz", wishing to remove
bar (which exists) even when foo does not exist, is also very
understandable.

I think they are pretty much independent options.

[PATCH 1/2] Add --quiet option to suppress output of "rm" commands for removed files.

[Steven Grimm]

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

Splitting this into two different options (and two patches) per Junio's
comments.

 Documentation/git-rm.txt |    4 ++++
 builtin-rm.c             |   10 +++++++---
 t/t3600-rm.sh            |   24 ++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 6feebc0..b051ccb 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -47,6 +47,10 @@ OPTIONS
 	the paths only from the index, leaving working tree
 	files.
 
+\--quiet::
+	git-rm normally outputs one line (in the form of an "rm" command)
+	for each file removed. This option suppresses that output.
+
 
 DISCUSSION
 ----------
diff --git a/builtin-rm.c b/builtin-rm.c
index 8a0738f..7eb9a42 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -10,7 +10,7 @@
 #include "tree-walk.h"
 
 static const char builtin_rm_usage[] =
-"git-rm [-f] [-n] [-r] [--cached] [--] <file>...";
+"git-rm [-f] [-n] [-r] [--cached] [--quiet] [--] <file>...";
 
 static struct {
 	int nr, alloc;
@@ -104,7 +104,7 @@ static struct lock_file lock_file;
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	int i, newfd;
-	int show_only = 0, force = 0, index_only = 0, recursive = 0;
+	int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 	const char **pathspec;
 	char *seen;
 
@@ -132,6 +132,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			force = 1;
 		else if (!strcmp(arg, "-r"))
 			recursive = 1;
+		else if (!strcmp(arg, "--quiet"))
+			quiet = 1;
 		else
 			usage(builtin_rm_usage);
 	}
@@ -187,7 +189,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 */
 	for (i = 0; i < list.nr; i++) {
 		const char *path = list.name[i];
-		printf("rm '%s'\n", path);
+		if (!quiet) {
+			printf("rm '%s'\n", path);
+		}
 
 		if (remove_file_from_cache(path))
 			die("git-rm: unable to remove %s", path);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index e31cf93..da9da92 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -84,6 +84,26 @@ test_expect_success \
     'When the rm in "git-rm -f" fails, it should not remove the file from the index' \
     'git-ls-files --error-unmatch baz'
 
+test_expect_success '"rm" command printed' '
+	echo frotz > test-file &&
+	git add test-file &&
+	git commit -m "add file for rm test" &&
+	git rm test-file > rm-output &&
+	test `egrep "^rm " rm-output | wc -l` = 1 &&
+	rm -f test-file rm-output &&
+	git commit -m "remove file from rm test"
+'
+
+test_expect_success '"rm" command suppressed with --quiet' '
+	echo frotz > test-file &&
+	git add test-file &&
+	git commit -m "add file for rm --quiet test" &&
+	git rm --quiet test-file > rm-output &&
+	test `wc -l < rm-output` = 0 &&
+	rm -f test-file rm-output &&
+	git commit -m "remove file from rm --quiet test"
+'
+
 # Now, failure cases.
 test_expect_success 'Re-add foo and baz' '
 	git add foo baz &&
@@ -154,4 +174,8 @@ test_expect_success 'Recursive with -r -f' '
 	! test -d frotz
 '
 
+test_expect_failure 'Remove nonexistent file returns nonzero exit status' '
+	git rm nonexistent
+'
+
 test_done
-- 
1.5.1.1.99.g0ea98

[PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Steven Grimm]

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

This allows "git rm -r --ignore-notfound" to be used as an index filter
with cg-admin-rewritehist. The documentation for that command recommends
using git-update-index --remove to filter files out of a tree's history,
but that doesn't support recursive deletion like git-rm does, making it
less convenient to filter directories from history.

 Documentation/git-rm.txt |    3 +++
 builtin-rm.c             |   21 +++++++++++++++++----
 t/t3600-rm.sh            |    4 ++++
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index b051ccb..9ffb515 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -47,6 +47,9 @@ OPTIONS
 	the paths only from the index, leaving working tree
 	files.
 
+\--ignore-notfound::
+	Exit with a zero status even if no files matched.
+
 \--quiet::
 	git-rm normally outputs one line (in the form of an "rm" command)
 	for each file removed. This option suppresses that output.
diff --git a/builtin-rm.c b/builtin-rm.c
index 7eb9a42..71166fb 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -10,7 +10,7 @@
 #include "tree-walk.h"
 
 static const char builtin_rm_usage[] =
-"git-rm [-f] [-n] [-r] [--cached] [--quiet] [--] <file>...";
+"git-rm [-f] [-n] [-r] [--cached] [--ignore-notfound] [--quiet] [--] <file>...";
 
 static struct {
 	int nr, alloc;
@@ -105,6 +105,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	int i, newfd;
 	int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
+	int ignore_notfound = 0;
 	const char **pathspec;
 	char *seen;
 
@@ -134,6 +135,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			recursive = 1;
 		else if (!strcmp(arg, "--quiet"))
 			quiet = 1;
+		else if (!strcmp(arg, "--ignore-notfound"))
+			ignore_notfound = 1;
 		else
 			usage(builtin_rm_usage);
 	}
@@ -155,14 +158,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	if (pathspec) {
 		const char *match;
+		int seen_any = 0;
 		for (i = 0; (match = pathspec[i]) != NULL ; i++) {
-			if (!seen[i])
-				die("pathspec '%s' did not match any files",
-				    match);
+			if (!seen[i]) {
+				if (!ignore_notfound) {
+					die("pathspec '%s' did not match any files",
+					    match);
+				}
+			}
+			else {
+				seen_any = 1;
+			}
 			if (!recursive && seen[i] == MATCHED_RECURSIVELY)
 				die("not removing '%s' recursively without -r",
 				    *match ? match : ".");
 		}
+
+		if (! seen_any)
+			exit(0);
 	}
 
 	/*
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index da9da92..665b8b0 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -84,6 +84,10 @@ test_expect_success \
     'When the rm in "git-rm -f" fails, it should not remove the file from the index' \
     'git-ls-files --error-unmatch baz'
 
+test_expect_success 'Remove nonexistent file with --ignore-notfound' '
+	git rm --ignore-notfound nonexistent
+'
+
 test_expect_success '"rm" command printed' '
 	echo frotz > test-file &&
 	git add test-file &&
-- 
1.5.1.1.99.g0ea98

Re: [PATCH] Add -q option to "git rm" to suppress output when there aren't errors.

[Alex Riesen]

On 4/16/07, Junio C Hamano <junkio@cox.net> wrote:
> What does it have to do with anything?
>
> Saying "git rm --quiet foo" from the command line, wishing to
> supress the output, is very understandable.
>
> Saying "git rm --ignore-unmatch foo bar baz", wishing to remove
> bar (which exists) even when foo does not exist, is also very
> understandable.

How about doing what rm(1) does? Something like "rm -f"?
It returns 0 even if nothing given in command-line, but any
error (like permissions) is reported and the status is not 0.

It is often that it is not the commands output which is
annoying, but the _unwanted_ output. No one wants
to see "rm 'file'". No one besides people debugging
git-rm (you can't even use it for copy and paste of the filenames:
it has "rm"'s in the text).

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

>  Documentation/git-rm.txt |    3 +++
>  builtin-rm.c             |   21 +++++++++++++++++----
>  t/t3600-rm.sh            |    4 ++++
>  3 files changed, 24 insertions(+), 4 deletions(-)

I find it easier to see what is going on with two independent
features separated out, especially with tests.  Much nicer.

> @@ -47,6 +47,9 @@ OPTIONS
>  	the paths only from the index, leaving working tree
>  	files.
>  
> +\--ignore-notfound::
> +	Exit with a zero status even if no files matched.
> +

ls-files has --error-unmatch so we may want to make the name
consistent by saying --ignore-unmatch?  I particularly do not
care about the minute naming issues _right_ _now_, but we might
regret it in the long run.

No need to resend unless you have an actual fix; I'll queue your
patch as is in 'pu', wait for a few days for comments on the
naming, and amend it as needed.

Thanks.

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Steven Grimm]

Steven Grimm wrote:
> This allows "git rm -r --ignore-notfound" to be used as an index filter
> with cg-admin-rewritehist. The documentation for that command recommends
> using git-update-index --remove to filter files out of a tree's history,
> but that doesn't support recursive deletion like git-rm does, making it
> less convenient to filter directories from history.
>   

Having said that, in some sense the problem here is arguably with 
cg-admin-rewritehist bailing out when a filter command exits with a 
nonzero status; given that git-rm is likely to be a common filter people 
will want to use, ideally cg-admin-rewritehist should do the right thing 
with git-rm's normal exit codes. But it appears Cogito is not being 
actively maintained any more (correct me if I'm wrong; the latest stable 
release is half a year old at this point, while git has changed 
substantially since then) so it didn't seem too worthwhile to try to 
patch things from that side.

I think it makes sense to be able to optionally treat a lack of matches 
as a non-error condition like "rm -f" does, so I'm comfortable 
submitting this patch regardless.

On a related note, last time I asked a question that caused someone to 
point me to cg-admin-rewritehist, Linus said he'd like to see its 
functionality folded into git proper. I have to agree with that idea; 
there are some obvious enhancements that program could use, and it'll be 
easier to do that if it's part of an actively maintained code base.

-Steve

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Steven Grimm]

Junio C Hamano wrote:
> ls-files has --error-unmatch so we may want to make the name
> consistent by saying --ignore-unmatch?  I particularly do not
> care about the minute naming issues _right_ _now_, but we might
> regret it in the long run.
>   

Sure, UI consistency is a good thing. Feel free to amend.

-Steve

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Jeff King]

On Mon, Apr 16, 2007 at 12:59:35AM -0700, Junio C Hamano wrote:

> > +\--ignore-notfound::
> > +	Exit with a zero status even if no files matched.
> > +
> ls-files has --error-unmatch so we may want to make the name
> consistent by saying --ignore-unmatch?  I particularly do not
> care about the minute naming issues _right_ _now_, but we might
> regret it in the long run.

Isn't this quite similar to the '-k' option to git-mv?

-Peff

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 16, 2007 at 12:59:35AM -0700, Junio C Hamano wrote:
>
>> > +\--ignore-notfound::
>> > +	Exit with a zero status even if no files matched.
>> > +
>> ls-files has --error-unmatch so we may want to make the name
>> consistent by saying --ignore-unmatch?  I particularly do not
>> care about the minute naming issues _right_ _now_, but we might
>> regret it in the long run.
>
> Isn't this quite similar to the '-k' option to git-mv?

Heh, I never use 'git-mv' and 'git-rm' myself, so I did not know
about that option, but from the Documentation/git-mv.txt, it
sounds similar.  What does it stand for, I wonder?

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Jeff King]

On Mon, Apr 16, 2007 at 01:53:46AM -0700, Junio C Hamano wrote:

> >> > +\--ignore-notfound::
> >> > +	Exit with a zero status even if no files matched.
> >> > +
> >> ls-files has --error-unmatch so we may want to make the name
> >> consistent by saying --ignore-unmatch?  I particularly do not
> >> care about the minute naming issues _right_ _now_, but we might
> >> regret it in the long run.
> >
> > Isn't this quite similar to the '-k' option to git-mv?
> 
> Heh, I never use 'git-mv' and 'git-rm' myself, so I did not know
> about that option, but from the Documentation/git-mv.txt, it
> sounds similar.  What does it stand for, I wonder?

As you are so fond of telling everyone else, use git-blame to find out. :)

Looks like '-k' got added in the move from git-rename to git-mv. Josef,
can you shed any light on the choice of '-k'?

-Peff

Re: [PATCH 2/2] Add --ignore-notfound option to exit with zero status when no files are removed.

[Josef Weidendorfer]

On Monday 16 April 2007, you wrote:
> On Mon, Apr 16, 2007 at 01:53:46AM -0700, Junio C Hamano wrote:
> 
> > >> > +\--ignore-notfound::
> > >> > +	Exit with a zero status even if no files matched.
> > >> > +
> > >> ls-files has --error-unmatch so we may want to make the name
> > >> consistent by saying --ignore-unmatch?  I particularly do not
> > >> care about the minute naming issues _right_ _now_, but we might
> > >> regret it in the long run.
> > >
> > > Isn't this quite similar to the '-k' option to git-mv?
> > 
> > Heh, I never use 'git-mv' and 'git-rm' myself, so I did not know
> > about that option, but from the Documentation/git-mv.txt, it
> > sounds similar.  What does it stand for, I wonder?
> 
> As you are so fond of telling everyone else, use git-blame to find out. :)
> 
> Looks like '-k' got added in the move from git-rename to git-mv. Josef,
> can you shed any light on the choice of '-k'?

That is similiar to "-k" option to "make", synonym for "--keep-going". 
I thought this was quite natural; at least I am used to this option for "make".


Josef