{"thread":{"id":"40827","subject":"git rm --recursive","startedAt":"2015-11-18T15:06:38Z","lastAt":"2015-11-24T02:12:42Z","messageCount":2,"participants":["Hans Ginzel","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273475","messageId":"20151118150637.GA49061@artax.karlin.mff.cuni.cz","threadId":"40827","inReplyTo":null,"subject":"git rm --recursive","fromName":"Hans Ginzel","fromEmail":"hans@matfyz.cz","sentAt":"2015-11-18T15:06:38Z","receivedAt":"2015-11-18T15:06:38Z","isPatch":false,"sender":{"key":"hans@matfyz.cz","avatar":"https://gravatar.com/avatar/6b8af172eb5ec2b675d3966812ceaf2aa6d487191e47c811f2661fb660e6af4b?d=mp&s=160"},"body":"Hello!\n\nI have added the --recursive alias for the -r option to the rm command.\n\nH.\nFrom 83f197151c04164b0dfd4d127e72439aebaf8b71 Mon Sep 17 00:00:00 2001\nFrom: Hans Ginzel <hans@matfyz.cz>\nDate: Wed, 18 Nov 2015 15:44:56 +0100\nSubject: [PATCH] builtin: rm: add --recursive to be consistent with GNU rm\n\n\ndiff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt\nindex f1efc11..0ab1cd4 100644\n--- a/Documentation/git-rm.txt\n+++ b/Documentation/git-rm.txt\n@@ -47,6 +47,7 @@ OPTIONS\n \tby the command.\n \n -r::\n+--recursive::\n         Allow recursive removal when a leading directory name is\n         given.\n \ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 80b972f..311b4da 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -269,7 +269,7 @@ static struct option builtin_rm_options[] = {\n \tOPT__QUIET(&quiet, N_(\"do not list removed files\")),\n \tOPT_BOOL( 0 , \"cached\",         &index_only, N_(\"only remove from the index\")),\n \tOPT__FORCE(&force, N_(\"override the up-to-date check\")),\n-\tOPT_BOOL('r', NULL,             &recursive,  N_(\"allow recursive removal\")),\n+\tOPT_BOOL('r', \"recursive\",      &recursive,  N_(\"allow recursive removal\")),\n \tOPT_BOOL( 0 , \"ignore-unmatch\", &ignore_unmatch,\n \t\t\t\tN_(\"exit with a zero status even if nothing matched\")),\n \tOPT_END(),\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 9d90d2c..d7b73f9 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -207,12 +207,25 @@ test_expect_success 'Recursive with -r but dirty' '\n \ttest -f frotz/nitfol\n '\n \n+test_expect_success 'Recursive with --recursive but dirty' '\n+\techo qfwfq >>frotz/nitfol &&\n+\ttest_must_fail git rm --recursive frotz &&\n+\ttest -d frotz &&\n+\ttest -f frotz/nitfol\n+'\n+\n test_expect_success 'Recursive with -r -f' '\n \tgit rm -f -r frotz &&\n \t! test -f frotz/nitfol &&\n \t! test -d frotz\n '\n \n+test_expect_success 'Recursive with --recursive -f' '\n+\tgit rm -f --recursive frotz &&\n+\t! test -f frotz/nitfol &&\n+\t! test -d frotz\n+'\n+\n test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n \ttest_must_fail git rm nonexistent\n '\n-- \n1.9.1\n"},{"id":"273630","messageId":"CAPig+cRF+VumPsbZR0mrGyY7Zbiz+yqRK-29iXUxjneNy4efqw@mail.gmail.com","threadId":"40827","inReplyTo":"20151118150637.GA49061@artax.karlin.mff.cuni.cz","subject":"Re: git rm --recursive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-24T02:12:42Z","receivedAt":"2015-11-24T02:12:42Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 18, 2015 at 10:06 AM, Hans Ginzel <hans@matfyz.cz> wrote:\n> I have added the --recursive alias for the -r option to the rm command.\n\nWhen sending a patch, separate the patch itself from the above\ncommentary with a scissor line (--- 8< ---) so that git-am can extract\nthe patch automatically. Alternately, appease git-am by placing the\ncommentary below the \"---\" line just above the diffstat.\n\n> From 83f197151c04164b0dfd4d127e72439aebaf8b71 Mon Sep 17 00:00:00 2001\n> From: Hans Ginzel <hans@matfyz.cz>\n> Date: Wed, 18 Nov 2015 15:44:56 +0100\n\nAll three above lines should be dropped. \"From SHA1\" is meaningful\nonly in your repository, thus not helpful in the patch. \"From:\" is the\nsame as the email address from which you sent the patch, so git-am can\njust pick it up from the email envelope. On this project, the\nimportant date is when the maintainer applies the patch, so the above\n\"Date:\" is not interesting.\n\n> Subject: [PATCH] builtin: rm: add --recursive to be consistent with GNU rm\n\nThe body of the commit message (following the subject) would be a good\nplace to explain why --recursive is desirable. Consistency with GNU\n'rm' may be reasonable, but you may need to be more persuasive to\nconvince people that the project should support yet another alias for\nan existing option and that whatever future maintenance that involves\n(documentation and code) is really worthwhile, especially since nobody\nhas expressed interest in such an alias as yet.\n\nAlso, if you're aiming for consistency with GNU 'rm' (or even BSD\n'rm'), wouldn't you want to support -R, as well?\n\nSimilarly, git-rm's -f option behaves differently than -f of Unix 'rm'\nin that even with -f, git-rm still expects the named path to exist,\nwhereas Unix 'rm' does not. Is that difference also worth addressing,\nor is that an argument against trying to make git-rm consistent with\nGNU 'rm'?\n\nYour Signed-off-by: is missing. See Documentation/SubmittingPatches.\n\nMore below...\n\n> diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt\n> @@ -47,6 +47,7 @@ OPTIONS\n> -r::\n> +--recursive::\n>         Allow recursive removal when a leading directory name is\n>         given.\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> @@ -269,7 +269,7 @@ static struct option builtin_rm_options[] = {\n> -       OPT_BOOL('r', NULL,             &recursive,  N_(\"allow recursive\n> removal\")),\n> +       OPT_BOOL('r', \"recursive\",      &recursive,  N_(\"allow recursive\n> removal\")),\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> @@ -207,12 +207,25 @@ test_expect_success 'Recursive with -r but dirty' '\n>         test -f frotz/nitfol\n> '\n>\n> +test_expect_success 'Recursive with --recursive but dirty' '\n> +       echo qfwfq >>frotz/nitfol &&\n> +       test_must_fail git rm --recursive frotz &&\n> +       test -d frotz &&\n> +       test -f frotz/nitfol\n> +'\n\nThis copy/pasted test isn't doing what you think it's doing. It would\npass even if you had totally botched the implementation of\n--recursive. That's because it actively expects \"git rm --recursive\"\nto fail, so it would \"succeed\" for any failure, for instance, due to a\nbotched implementation.\n\nWhat you probably want to do instead is create a test which verifies\nthat \"git rm\" alone fails when attempting to remove a directory, and\nthat \"git rm --recursive\" succeeds.\n\n> test_expect_success 'Recursive with -r -f' '\n>         git rm -f -r frotz &&\n>         ! test -f frotz/nitfol &&\n>         ! test -d frotz\n> '\n>\n> +test_expect_success 'Recursive with --recursive -f' '\n> +       git rm -f --recursive frotz &&\n> +       ! test -f frotz/nitfol &&\n> +       ! test -d frotz\n> +'\n\nThis copy/pasted test is in even worse shape. It doesn't pass at all,\neven with a perfectly operational --recursive implementation. (I'm\nguessing that you didn't run the tests after adding this.) This is\nbecause the preceding test, from which this was copied, destroys state\nprepared by the earlier \"Recursive test setup\" test upon which this\nnew test depends. Without that state, the test can not pass.\n\nAnyhow, if you follow the advice above and just craft a new test which\nensures that \"git rm\" fails on a directory and \"git rm --recursive\"\nsucceeds, then that should be sufficient to verify that --recursive\nworks as an alias for -r, and you can drop these two (bogus)\ncopy/pasted tests.\n\n> test_expect_success 'Remove nonexistent file returns nonzero exit status' '\n>         test_must_fail git rm nonexistent\n> '\n> --\n> 1.9.1\n"}]}