[PATCH] git-clean: handle errors if removing files fails

[PATCH] git-clean: handle errors if removing files fails

[Miklos Vajna]

Consider the following case:

$ sudo mkdir foo
$ sudo touch foo/bar

This is the old output:

$ git clean -f -d
Removing foo/

No error message.

This is the new output:

$ ~/git/git/git clean -f -d
Removing foo/
fatal: failed to remove 'foo/'

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-clean.c  |   14 ++++++++------
 t/t7300-clean.sh |    9 +++++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index eb853a3..c8753a5 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -137,12 +137,13 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (show_only && (remove_directories || matches)) {
 				printf("Would remove %s\n",
 				       directory.buf + prefix_offset);
-			} else if (quiet && (remove_directories || matches)) {
-				remove_dir_recursively(&directory, 0);
 			} else if (remove_directories || matches) {
-				printf("Removing %s\n",
-				       directory.buf + prefix_offset);
-				remove_dir_recursively(&directory, 0);
+				if (!quiet)
+					printf("Removing %s\n",
+					       directory.buf + prefix_offset);
+				if (remove_dir_recursively(&directory, 0) != 0)
+					die("failed to remove '%s'",
+						directory.buf + prefix_offset);
 			} else if (show_only) {
 				printf("Would not remove %s\n",
 				       directory.buf + prefix_offset);
@@ -162,7 +163,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				printf("Removing %s\n",
 				       ent->name + prefix_offset);
 			}
-			unlink(ent->name);
+			if (unlink(ent->name) != 0)
+				die("failed to remove '%s'", ent->name);
 		}
 	}
 	free(seen);
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index dfd1188..bd0a814 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -316,4 +316,13 @@ test_expect_success 'core.excludesfile' '
 
 '
 
+test_expect_success 'removal failure' '
+
+	mkdir foo &&
+	touch foo/bar &&
+	chmod 0 foo &&
+	! git clean -f -d
+
+'
+
 test_done
-- 
1.5.4.1

Re: [PATCH] git-clean: handle errors if removing files fails

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Consider the following case:
>
> $ sudo mkdir foo
> $ sudo touch foo/bar
>
> This is the old output:
>
> $ git clean -f -d
> Removing foo/
>
> No error message.
>
> This is the new output:
>
> $ ~/git/git/git clean -f -d
> Removing foo/
> fatal: failed to remove 'foo/'

That's quite different style from the other commit log messages
in the project, isn't it?

While I agree reporting an error is definitely an improvement, I
do not think dying in the middle is the right thing to do.

Shouldn't it note the error, remove other cruft, and then
finally signal the error by exiting non-zero?

[PATCH] git-clean: handle errors if removing files fails

[Miklos Vajna]

git-clean simply ignored errors if removing a file or directory failed. This
patch makes it raise a warning and the exit code also greater than zero if
there are remaining files.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Wed, Feb 20, 2008 at 04:12:16PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> That's quite different style from the other commit log messages
> in the project, isn't it?

right, i rewrote it.

> While I agree reporting an error is definitely an improvement, I
> do not think dying in the middle is the right thing to do.
>
> Shouldn't it note the error, remove other cruft, and then
> finally signal the error by exiting non-zero?

something like this?

i also noticed that we need a chmod 755 after the last test, otherwise
removing the trash directory will not be possible. i haven't hit this
bug by running the test only once.

 builtin-clean.c  |   22 ++++++++++++++--------
 t/t7300-clean.sh |   10 ++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index eb853a3..5ed57d7 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -29,7 +29,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
-	int ignored_only = 0, baselen = 0, config_set = 0;
+	int ignored_only = 0, baselen = 0, config_set = 0, errors = 0;
 	struct strbuf directory;
 	struct dir_struct dir;
 	const char *path, *base;
@@ -137,12 +137,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (show_only && (remove_directories || matches)) {
 				printf("Would remove %s\n",
 				       directory.buf + prefix_offset);
-			} else if (quiet && (remove_directories || matches)) {
-				remove_dir_recursively(&directory, 0);
 			} else if (remove_directories || matches) {
-				printf("Removing %s\n",
-				       directory.buf + prefix_offset);
-				remove_dir_recursively(&directory, 0);
+				if (!quiet)
+					printf("Removing %s\n",
+					       directory.buf + prefix_offset);
+				if (remove_dir_recursively(&directory, 0) != 0) {
+					warning("failed to remove '%s'",
+						directory.buf + prefix_offset);
+					errors++;
+				}
 			} else if (show_only) {
 				printf("Would not remove %s\n",
 				       directory.buf + prefix_offset);
@@ -162,11 +165,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				printf("Removing %s\n",
 				       ent->name + prefix_offset);
 			}
-			unlink(ent->name);
+			if (unlink(ent->name) != 0) {
+				warning("failed to remove '%s'", ent->name);
+				errors++;
+			}
 		}
 	}
 	free(seen);
 
 	strbuf_release(&directory);
-	return 0;
+	return errors;
 }
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index dfd1188..3840364 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -316,4 +316,14 @@ test_expect_success 'core.excludesfile' '
 
 '
 
+test_expect_success 'removal failure' '
+
+	mkdir foo &&
+	touch foo/bar &&
+	chmod 0 foo &&
+	! git clean -f -d
+
+'
+chmod 755 foo
+
 test_done
-- 
1.5.4.1