Re: [PATCH 03/12] git rm: display a warning for every unremovable file

[Jonathan Nieder <jrnieder@gmail.com>]

Peter Collingbourne wrote:

> This patch causes git-rm to display a warning if it was unable to
> remove a file other than the first one, rather than silently ignoring
> the error, which was the previous behaviour.
> 
> Signed-off-by: Peter Collingbourne <peter@pcc.me.uk>
[...]
> @@ -259,6 +259,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  			}
>  			if (!removed)
>  				die_errno("git rm: '%s'", path);
> +			else
> +				warning("git rm: '%s': %s", path, strerror(errno));

My first reaction was to wonder what is going on.  Clearly someone
had been bothered by the obvious behavior of giving a long stream
of warnings or we wouldn’t have the current behavior.

But in fact, the unchanged code comment before the code you changed
and the message for commit d9b814cc (Add builtin "git rm" command,
2006-05-19) shows me wrong:

    So what happens is that if the _first_ file fails to be removed with "-f", 
    we abort the whole "git rm". But once we've started removing, we don't 
    leave anything half done. If some of the other files don't exist, we'll 
    just ignore errors of removal from the working tree.

    This is only an issue with "-f", of course.

    I think the new behaviour is strictly an improvement, but perhaps more 
    importantly, it is _different_. As a special case, the semantics are 
    identical for the single-file case (which is the only one our test-suite 
    seems to test).

I think the commit message could avoid this confusion.  Something like:

	When ‘git rm’ was built in (d9b814cc, 2006-05-19), its
	semantics changed: before, it just removed files until it
	encountered an error and then would error out, whereas since
	then, it makes an attempt to either remove all files or remove
	none at all.  In particular, if ‘git rm’ fails to remove a
	file after other files have already been removed, it does not
	abort but instead silently accepts the error.

	Better to warn the user in this case!

	This problem is particularly noticeable when dealing with
	submodules because ...

Some tests would be nice as well --- maybe something like the following?

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0aaf0ad..a3861b8 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -126,6 +126,75 @@ test_expect_success 'Remove nonexistent file with --ignore-unmatch' '
 	git rm --ignore-unmatch nonexistent
 '
 
+test_expect_success 'If the first (in alphabetical order) removal fails, rm is cancelled' '
+	touch xyzzy &&
+	mkdir -p plugh &&
+	touch plugh/xyzzy &&
+	git add xyzzy plugh/xyzzy &&
+	git commit --allow-empty -a -m "two files to remove" &&
+	chmod a-w plugh &&
+	git ls-files --stage >before &&
+	test $(grep xyzzy before | wc -l) = 2 &&
+
+	test_must_fail git rm xyzzy plugh/xyzzy &&
+
+	test -e plugh/xyzzy &&
+	test -e xyzzy &&
+	git ls-files --stage >after &&
+	test_cmp before after
+'
+! test -e plugh || chmod 775 plugh
+rm -fr before after plugh xyzzy
+
+test_expect_success 'Best-effort behavior if the second removal fails' '
+	touch plugh &&
+	mkdir -p xyzzy &&
+	touch xyzzy/plugh &&
+	git add plugh xyzzy/plugh &&
+	git commit --allow-empty -a -m "two files to remove" &&
+	chmod a-w xyzzy &&
+	: >expect &&
+
+	git rm plugh xyzzy/plugh &&
+
+	test -e xyzzy/plugh &&
+	! test -e plugh &&
+	git ls-files --stage plugh xyzzy/plugh >actual &&
+	test_cmp expect actual
+'
+! test -e xyzzy || chmod 775 xyzzy
+rm -fr expect actual plugh xyzzy
+
+test_expect_success 'Message when first removal fails' '
+	touch xyzzy &&
+	mkdir -p plugh &&
+	touch plugh/xyzzy &&
+	git add xyzzy plugh/xyzzy &&
+	git commit --allow-empty -a -m "two files to remove" &&
+	chmod a-w plugh &&
+
+	test_must_fail git rm xyzzy plugh/xyzzy 2>msg &&
+
+	grep "git rm: '\''plugh/xyzzy'\'':" msg
+'
+! test -e plugh || chmod 775 plugh
+rm -fr msg plugh xyzzy
+
+test_expect_success 'Message when second removal fails' '
+	touch plugh &&
+	mkdir -p xyzzy &&
+	touch xyzzy/plugh &&
+	git add plugh xyzzy/plugh &&
+	git commit --allow-empty -a -m "two files to remove" &&
+	chmod a-w xyzzy &&
+
+	git rm plugh xyzzy/plugh 2>msg &&
+
+	grep "git rm: '\''xyzzy/plugh'\'':" msg
+'
+! test -e xyzzy || chmod 775 xyzzy
+rm -fr expect actual plugh xyzzy
+
 test_expect_success '"rm" command printed' '
 	echo frotz > test-file &&
 	git add test-file &&