{"thread":{"id":"12229","subject":"[PATCH] git-clean: handle errors if removing files fails","startedAt":"2008-02-20T23:41:54Z","lastAt":"2008-02-21T01:44:46Z","messageCount":3,"participants":["Miklos Vajna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"69427","messageId":"20080220234154.GS31441@genesis.frugalware.org","threadId":"12229","inReplyTo":null,"subject":"[PATCH] git-clean: handle errors if removing files fails","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-02-20T23:41:54Z","receivedAt":"2008-02-20T23:41:54Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Consider the following case:\n\n$ sudo mkdir foo\n$ sudo touch foo/bar\n\nThis is the old output:\n\n$ git clean -f -d\nRemoving foo/\n\nNo error message.\n\nThis is the new output:\n\n$ ~/git/git/git clean -f -d\nRemoving foo/\nfatal: failed to remove 'foo/'\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n builtin-clean.c  |   14 ++++++++------\n t/t7300-clean.sh |    9 +++++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex eb853a3..c8753a5 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -137,12 +137,13 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\tif (show_only && (remove_directories || matches)) {\n \t\t\t\tprintf(\"Would remove %s\\n\",\n \t\t\t\t       directory.buf + prefix_offset);\n-\t\t\t} else if (quiet && (remove_directories || matches)) {\n-\t\t\t\tremove_dir_recursively(&directory, 0);\n \t\t\t} else if (remove_directories || matches) {\n-\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n-\t\t\t\tremove_dir_recursively(&directory, 0);\n+\t\t\t\tif (!quiet)\n+\t\t\t\t\tprintf(\"Removing %s\\n\",\n+\t\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tif (remove_dir_recursively(&directory, 0) != 0)\n+\t\t\t\t\tdie(\"failed to remove '%s'\",\n+\t\t\t\t\t\tdirectory.buf + prefix_offset);\n \t\t\t} else if (show_only) {\n \t\t\t\tprintf(\"Would not remove %s\\n\",\n \t\t\t\t       directory.buf + prefix_offset);\n@@ -162,7 +163,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(\"Removing %s\\n\",\n \t\t\t\t       ent->name + prefix_offset);\n \t\t\t}\n-\t\t\tunlink(ent->name);\n+\t\t\tif (unlink(ent->name) != 0)\n+\t\t\t\tdie(\"failed to remove '%s'\", ent->name);\n \t\t}\n \t}\n \tfree(seen);\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex dfd1188..bd0a814 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -316,4 +316,13 @@ test_expect_success 'core.excludesfile' '\n \n '\n \n+test_expect_success 'removal failure' '\n+\n+\tmkdir foo &&\n+\ttouch foo/bar &&\n+\tchmod 0 foo &&\n+\t! git clean -f -d\n+\n+'\n+\n test_done\n-- \n1.5.4.1\n"},{"id":"69435","messageId":"7vodab182n.fsf@gitster.siamese.dyndns.org","threadId":"12229","inReplyTo":"20080220234154.GS31441@genesis.frugalware.org","subject":"Re: [PATCH] git-clean: handle errors if removing files fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-21T00:12:16Z","receivedAt":"2008-02-21T00:12:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> Consider the following case:\n>\n> $ sudo mkdir foo\n> $ sudo touch foo/bar\n>\n> This is the old output:\n>\n> $ git clean -f -d\n> Removing foo/\n>\n> No error message.\n>\n> This is the new output:\n>\n> $ ~/git/git/git clean -f -d\n> Removing foo/\n> fatal: failed to remove 'foo/'\n\nThat's quite different style from the other commit log messages\nin the project, isn't it?\n\nWhile I agree reporting an error is definitely an improvement, I\ndo not think dying in the middle is the right thing to do.\n\nShouldn't it note the error, remove other cruft, and then\nfinally signal the error by exiting non-zero?\n"},{"id":"69441","messageId":"20080221014446.GT31441@genesis.frugalware.org","threadId":"12229","inReplyTo":"7vodab182n.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] git-clean: handle errors if removing files fails","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-02-21T01:44:46Z","receivedAt":"2008-02-21T01:44:46Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"git-clean simply ignored errors if removing a file or directory failed. This\npatch makes it raise a warning and the exit code also greater than zero if\nthere are remaining files.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Wed, Feb 20, 2008 at 04:12:16PM -0800, Junio C Hamano <gitster@pobox.com> wrote:\n> That's quite different style from the other commit log messages\n> in the project, isn't it?\n\nright, i rewrote it.\n\n> While I agree reporting an error is definitely an improvement, I\n> do not think dying in the middle is the right thing to do.\n>\n> Shouldn't it note the error, remove other cruft, and then\n> finally signal the error by exiting non-zero?\n\nsomething like this?\n\ni also noticed that we need a chmod 755 after the last test, otherwise\nremoving the trash directory will not be possible. i haven't hit this\nbug by running the test only once.\n\n builtin-clean.c  |   22 ++++++++++++++--------\n t/t7300-clean.sh |   10 ++++++++++\n 2 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex eb853a3..5ed57d7 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -29,7 +29,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tint show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;\n-\tint ignored_only = 0, baselen = 0, config_set = 0;\n+\tint ignored_only = 0, baselen = 0, config_set = 0, errors = 0;\n \tstruct strbuf directory;\n \tstruct dir_struct dir;\n \tconst char *path, *base;\n@@ -137,12 +137,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\tif (show_only && (remove_directories || matches)) {\n \t\t\t\tprintf(\"Would remove %s\\n\",\n \t\t\t\t       directory.buf + prefix_offset);\n-\t\t\t} else if (quiet && (remove_directories || matches)) {\n-\t\t\t\tremove_dir_recursively(&directory, 0);\n \t\t\t} else if (remove_directories || matches) {\n-\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n-\t\t\t\tremove_dir_recursively(&directory, 0);\n+\t\t\t\tif (!quiet)\n+\t\t\t\t\tprintf(\"Removing %s\\n\",\n+\t\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n+\t\t\t\t\twarning(\"failed to remove '%s'\",\n+\t\t\t\t\t\tdirectory.buf + prefix_offset);\n+\t\t\t\t\terrors++;\n+\t\t\t\t}\n \t\t\t} else if (show_only) {\n \t\t\t\tprintf(\"Would not remove %s\\n\",\n \t\t\t\t       directory.buf + prefix_offset);\n@@ -162,11 +165,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(\"Removing %s\\n\",\n \t\t\t\t       ent->name + prefix_offset);\n \t\t\t}\n-\t\t\tunlink(ent->name);\n+\t\t\tif (unlink(ent->name) != 0) {\n+\t\t\t\twarning(\"failed to remove '%s'\", ent->name);\n+\t\t\t\terrors++;\n+\t\t\t}\n \t\t}\n \t}\n \tfree(seen);\n \n \tstrbuf_release(&directory);\n-\treturn 0;\n+\treturn errors;\n }\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex dfd1188..3840364 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -316,4 +316,14 @@ test_expect_success 'core.excludesfile' '\n \n '\n \n+test_expect_success 'removal failure' '\n+\n+\tmkdir foo &&\n+\ttouch foo/bar &&\n+\tchmod 0 foo &&\n+\t! git clean -f -d\n+\n+'\n+chmod 755 foo\n+\n test_done\n-- \n1.5.4.1\n"}]}