{"thread":{"id":"33589","subject":"[PATCH 0/3] git remote with superfluous arguments","startedAt":"2013-04-24T13:54:34Z","lastAt":"2013-04-25T15:31:32Z","messageCount":7,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"215310","messageId":"cover.1366811347.git.trast@inf.ethz.ch","threadId":"33589","inReplyTo":null,"subject":"[PATCH 0/3] git remote with superfluous arguments","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-24T13:54:34Z","receivedAt":"2013-04-24T13:54:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Today I botched a cut&paste and ran\n\n git remote add git remote add origin \n\nonly to notice, several commands later, that while it had succeeded,\nit had in fact added a remote called 'git' with an url of 'remote'.\nSome investigation yielded these fixes.\n\n\nThomas Rast (3):\n remote: add a test for extra arguments, according to docs\n remote: check for superfluous arguments in 'git remote add'\n remote: 'show' and 'prune' take more than one remote\n\n Documentation/git-remote.txt | 4 ++--\n builtin/remote.c | 2 +-\n t/t5505-remote.sh | 22 ++++++++++++++++++++++\n 3 files changed, 25 insertions(+), 3 deletions(-)\n\n-- \n1.8.2.1.931.g0116868\n"},{"id":"215312","messageId":"372aa93e4a7a3583730c02543583ce93e095ec64.1366811347.git.trast@inf.ethz.ch","threadId":"33589","inReplyTo":"cover.1366811347.git.trast@inf.ethz.ch","subject":"[PATCH 1/3] remote: add a test for extra arguments, according to docs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-24T13:54:35Z","receivedAt":"2013-04-24T13:54:35Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"This adds one test or comment for each subcommand of git-remote\naccording to its current documentation. All but 'set-branches' and\n'update' are listed as taking only a fixed number of arguments; for\nthose we can write a test with one more (bogus) argument, and see if\nthe command notices that.\n\nThey fail on several counts: 'add' does not check for extra arguments,\nand 'show' and 'prune' actually iterate over remotes (i.e., take any\nnumber of args). We'll fix them in the next two patches.\n\nThe -f machinery is only there to make the tests readable while still\nensuring they pass as a whole, and will be removed in the final patch.\n\nSigned-off-by: Thomas Rast \n---\n t/t5505-remote.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 6579a86..764ee97 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1003,4 +1003,31 @@ test_expect_success 'remote set-url --delete baz' '\n \tcmp expect actual\n '\n \n+test_expect_success 'extra args: setup' '\n+\t# add a dummy origin so that this does not trigger failure\n+\tgit remote add origin .\n+'\n+\n+test_extra_arg () {\n+\texpect=\"success\"\n+\tif test \"z$1\" = \"z-f\"; then\n+\t\texpect=failure\n+\t\tshift\n+\tfi\n+\ttest_expect_$expect \"extra args: $*\" \"\n+\t\ttest_must_fail git remote $* bogus_extra_arg 2>actual &&\n+\t\tgrep '^usage:' actual\n+\t\"\n+}\n+\n+test_extra_arg -f add nick url\n+test_extra_arg rename origin newname\n+test_extra_arg remove origin\n+test_extra_arg set-head origin master\n+# set-branches takes any number of args\n+test_extra_arg set-url origin newurl oldurl\n+test_extra_arg -f show origin\n+test_extra_arg -f prune origin\n+# update takes any number of args\n+\n test_done\n-- \n1.8.2.1.931.g0116868\n"},{"id":"215311","messageId":"f1da9b0de420a56313760847b580ee40e2c75b0b.1366811347.git.trast@inf.ethz.ch","threadId":"33589","inReplyTo":"cover.1366811347.git.trast@inf.ethz.ch","subject":"[PATCH 2/3] remote: check for superfluous arguments in 'git remote add'","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-24T13:54:36Z","receivedAt":"2013-04-24T13:54:36Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The 'git remote add' subcommand did not check for superfluous command\nline arguments. Make it so.\n\nSigned-off-by: Thomas Rast \n---\n builtin/remote.c | 2 +-\n t/t5505-remote.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 937484d..5e54d36 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -178,7 +178,7 @@ static int add(int argc, const char **argv)\n \targc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,\n \t\t\t 0);\n \n-\tif (argc < 2)\n+\tif (argc != 2)\n \t\tusage_with_options(builtin_remote_add_usage, options);\n \n \tif (mirror && master)\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 764ee97..eea87fc 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1020,7 +1020,7 @@ test_extra_arg () {\n \t\"\n }\n \n-test_extra_arg -f add nick url\n+test_extra_arg add nick url\n test_extra_arg rename origin newname\n test_extra_arg remove origin\n test_extra_arg set-head origin master\n-- \n1.8.2.1.931.g0116868\n"},{"id":"215313","messageId":"3c40e8ca9d85f5254c2ba7d6a42e1d0d196e6faf.1366811347.git.trast@inf.ethz.ch","threadId":"33589","inReplyTo":"cover.1366811347.git.trast@inf.ethz.ch","subject":"[PATCH 3/3] remote: 'show' and 'prune' take more than one remote","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-24T13:54:37Z","receivedAt":"2013-04-24T13:54:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The 'git remote show' and 'prune' subcommands are documented as taking\nonly a single remote name argument, but that is not the case; they\nwill simply iterate the action over all remotes given. Update the\ndocumentation and tests to match.\n\nWith the last user of the -f flag gone, we also remove the code\nsupporting it.\n\nSigned-off-by: Thomas Rast \n---\n Documentation/git-remote.txt | 4 ++--\n t/t5505-remote.sh | 11 +++--------\n 2 files changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex e8c396b..7a6f354 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -18,8 +18,8 @@ SYNOPSIS\n 'git remote set-url' [--push] []\n 'git remote set-url --add' [--push] \n 'git remote set-url --delete' [--push] \n-'git remote' [-v | --verbose] 'show' [-n] \n-'git remote prune' [-n | --dry-run] \n+'git remote' [-v | --verbose] 'show' [-n] ...\n+'git remote prune' [-n | --dry-run] ...\n 'git remote' [-v | --verbose] 'update' [-p | --prune] [( | )...]\n \n DESCRIPTION\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex eea87fc..dd10ff0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1009,12 +1009,7 @@ test_expect_success 'extra args: setup' '\n '\n \n test_extra_arg () {\n-\texpect=\"success\"\n-\tif test \"z$1\" = \"z-f\"; then\n-\t\texpect=failure\n-\t\tshift\n-\tfi\n-\ttest_expect_$expect \"extra args: $*\" \"\n+\ttest_expect_success \"extra args: $*\" \"\n \t\ttest_must_fail git remote $* bogus_extra_arg 2>actual &&\n \t\tgrep '^usage:' actual\n \t\"\n@@ -1026,8 +1021,8 @@ test_extra_arg remove origin\n test_extra_arg set-head origin master\n # set-branches takes any number of args\n test_extra_arg set-url origin newurl oldurl\n-test_extra_arg -f show origin\n-test_extra_arg -f prune origin\n+# show takes any number of args\n+# prune takes any number of args\n # update takes any number of args\n \n test_done\n-- \n1.8.2.1.931.g0116868\n"},{"id":"215391","messageId":"7vhaiva9u5.fsf@alter.siamese.dyndns.org","threadId":"33589","inReplyTo":"372aa93e4a7a3583730c02543583ce93e095ec64.1366811347.git.trast@inf.ethz.ch","subject":"Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-24T21:37:54Z","receivedAt":"2013-04-24T21:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> +test_extra_arg () {\n> +\texpect=\"success\"\n> +\tif test \"z$1\" = \"z-f\"; then\n> +\t\texpect=failure\n> +\t\tshift\n> +\tfi\n> +\ttest_expect_$expect \"extra args: $*\" \"\n> +\t\ttest_must_fail git remote $* bogus_extra_arg 2>actual &&\n> +\t\tgrep '^usage:' actual\n> +\t\"\n> +}\n> +\n> +test_extra_arg -f add nick url\n> +test_extra_arg rename origin newname\n\nPerhaps just a taste in readability thing, but I would prefer to see\nthem more like\n\n\ttest_extra_arg_expect failure add nick url\n\ttest_extra_arg_expect success rename origin newname\n\nthan misunderstanding-inviting \"-f\" that often stands for \"--force\".\n\nOther than that, the whole series was a pleasant read. Thanks.\n\n\n> +test_extra_arg remove origin\n> +test_extra_arg set-head origin master\n> +# set-branches takes any number of args\n> +test_extra_arg set-url origin newurl oldurl\n> +test_extra_arg -f show origin\n> +test_extra_arg -f prune origin\n> +# update takes any number of args\n> +\n> test_done\n"},{"id":"215410","messageId":"87obd3cbux.fsf@linux-k42r.v.cablecom.net","threadId":"33589","inReplyTo":"7vhaiva9u5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-25T07:23:34Z","receivedAt":"2013-04-25T07:23:34Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> +test_extra_arg () {\n>> +\texpect=\"success\"\n>> +\tif test \"z$1\" = \"z-f\"; then\n>> +\t\texpect=failure\n>> +\t\tshift\n>> +\tfi\n>> +\ttest_expect_$expect \"extra args: $*\" \"\n>> +\t\ttest_must_fail git remote $* bogus_extra_arg 2>actual &&\n>> +\t\tgrep '^usage:' actual\n>> +\t\"\n>> +}\n>> +\n>> +test_extra_arg -f add nick url\n>> +test_extra_arg rename origin newname\n>\n> Perhaps just a taste in readability thing, but I would prefer to see\n> them more like\n>\n> \ttest_extra_arg_expect failure add nick url\n> \ttest_extra_arg_expect success rename origin newname\n>\n> than misunderstanding-inviting \"-f\" that often stands for \"--force\".\n\nHmm. I had that at first, but then the final cleanup would have had to\ntouch all tests to remove the optional argument, making it noisy.\n\nAnyway, it's probably all a bit over-engineered for only one effective\ncode change ;-)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"215447","messageId":"7vwqrq62zv.fsf@alter.siamese.dyndns.org","threadId":"33589","inReplyTo":"87obd3cbux.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-25T15:31:32Z","receivedAt":"2013-04-25T15:31:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Rast writes:\n>>\n>>> +test_extra_arg () {\n>>> +\texpect=\"success\"\n>>> +\tif test \"z$1\" = \"z-f\"; then\n>>> +\t\texpect=failure\n>>> +\t\tshift\n>>> +\tfi\n>>> +\ttest_expect_$expect \"extra args: $*\" \"\n>>> +\t\ttest_must_fail git remote $* bogus_extra_arg 2>actual &&\n>>> +\t\tgrep '^usage:' actual\n>>> +\t\"\n>>> +}\n>>> +\n>>> +test_extra_arg -f add nick url\n>>> +test_extra_arg rename origin newname\n>>\n>> Perhaps just a taste in readability thing, but I would prefer to see\n>> them more like\n>>\n>> \ttest_extra_arg_expect failure add nick url\n>> \ttest_extra_arg_expect success rename origin newname\n>>\n>> than misunderstanding-inviting \"-f\" that often stands for \"--force\".\n>\n> Hmm. I had that at first, but then the final cleanup would have had to\n> touch all tests to remove the optional argument, making it noisy.\n\nYou do not need a final cleanup, as I _never_ meant failure/success\nin the above illustration to be _optional_. Being explicit reduces\nmental burden when you later have to read such a custom scaffolding\neach test invents in an ad-hoc manner to suit its needs.\n"}]}