threads / patch / 33589

patch, 3 partsgit remote with superfluous arguments

Subject: [PATCH 0/3] git remote with superfluous arguments

## tl;dr

7 messages between Apr 24, 2013 and Apr 25, 2013. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Thomas Rast· Apr 24, 2013, 13:54 UTC · lore
Today I botched a cut&paste and ran
  git remote add git remote add origin <url>

only to notice, several commands later, that while it had succeeded, it had in fact added a remote called 'git' with an url of 'remote'. Some investigation yielded these fixes.

Thomas Rast (3):
  remote: add a test for extra arguments, according to docs
  remote: check for superfluous arguments in 'git remote add'
  remote: 'show' and 'prune' take more than one remote
 Documentation/git-remote.txt |  4 ++--
 builtin/remote.c             |  2 +-
 t/t5505-remote.sh            | 22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)
-- 
1.8.2.1.931.g0116868
Thomas Rast· Apr 24, 2013, 13:54 UTC · re: Thomas Rast · lore

[PATCH 1/3] remote: add a test for extra arguments, according to docs

This adds one test or comment for each subcommand of git-remote according to its current documentation. All but 'set-branches' and 'update' are listed as taking only a fixed number of arguments; for those we can write a test with one more (bogus) argument, and see if the command notices that.

They fail on several counts: 'add' does not check for extra arguments, and 'show' and 'prune' actually iterate over remotes (i.e., take any number of args). We'll fix them in the next two patches.

The -f machinery is only there to make the tests readable while still ensuring they pass as a whole, and will be removed in the final patch.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/t5505-remote.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Show changes to t/t5505-remote.sh +27 −0
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 6579a86..764ee97 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1003,4 +1003,31 @@ test_expect_success 'remote set-url --delete baz' '
 	cmp expect actual
 '
 
+test_expect_success 'extra args: setup' '
+	# add a dummy origin so that this does not trigger failure
+	git remote add origin .
+'
+
+test_extra_arg () {
+	expect="success"
+	if test "z$1" = "z-f"; then
+		expect=failure
+		shift
+	fi
+	test_expect_$expect "extra args: $*" "
+		test_must_fail git remote $* bogus_extra_arg 2>actual &&
+		grep '^usage:' actual
+	"
+}
+
+test_extra_arg -f add nick url
+test_extra_arg rename origin newname
+test_extra_arg remove origin
+test_extra_arg set-head origin master
+# set-branches takes any number of args
+test_extra_arg set-url origin newurl oldurl
+test_extra_arg -f show origin
+test_extra_arg -f prune origin
+# update takes any number of args
+
 test_done

-- 
1.8.2.1.931.g0116868
Junio C Hamano· Apr 24, 2013, 21:37 UTC · re: Thomas Rast · lore

Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs

Thomas Rast <trast@inf.ethz.ch> writes:
Show 14 quoted lines
> +test_extra_arg () {
> +	expect="success"
> +	if test "z$1" = "z-f"; then
> +		expect=failure
> +		shift
> +	fi
> +	test_expect_$expect "extra args: $*" "
> +		test_must_fail git remote $* bogus_extra_arg 2>actual &&
> +		grep '^usage:' actual
> +	"
> +}
> +
> +test_extra_arg -f add nick url
> +test_extra_arg rename origin newname

Perhaps just a taste in readability thing, but I would prefer to see them more like

	test_extra_arg_expect failure add nick url
	test_extra_arg_expect success rename origin newname
than misunderstanding-inviting "-f" that often stands for "--force".
Other than that, the whole series was a pleasant read.  Thanks.
Show 9 quoted lines
> +test_extra_arg remove origin
> +test_extra_arg set-head origin master
> +# set-branches takes any number of args
> +test_extra_arg set-url origin newurl oldurl
> +test_extra_arg -f show origin
> +test_extra_arg -f prune origin
> +# update takes any number of args
> +
>  test_done
Thomas Rast· Apr 25, 2013, 07:23 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs

Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> +test_extra_arg () {
>> +	expect="success"
>> +	if test "z$1" = "z-f"; then
>> +		expect=failure
>> +		shift
>> +	fi
>> +	test_expect_$expect "extra args: $*" "
>> +		test_must_fail git remote $* bogus_extra_arg 2>actual &&
>> +		grep '^usage:' actual
>> +	"
>> +}
>> +
>> +test_extra_arg -f add nick url
>> +test_extra_arg rename origin newname
>
> Perhaps just a taste in readability thing, but I would prefer to see
> them more like
>
> 	test_extra_arg_expect failure add nick url
> 	test_extra_arg_expect success rename origin newname
>
> than misunderstanding-inviting "-f" that often stands for "--force".

Hmm. I had that at first, but then the final cleanup would have had to touch all tests to remove the optional argument, making it noisy.

Anyway, it's probably all a bit over-engineered for only one effective code change ;-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Apr 25, 2013, 15:31 UTC · re: Thomas Rast · lore

Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs

Thomas Rast <trast@inf.ethz.ch> writes:
Show 29 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@inf.ethz.ch> writes:
>>
>>> +test_extra_arg () {
>>> +	expect="success"
>>> +	if test "z$1" = "z-f"; then
>>> +		expect=failure
>>> +		shift
>>> +	fi
>>> +	test_expect_$expect "extra args: $*" "
>>> +		test_must_fail git remote $* bogus_extra_arg 2>actual &&
>>> +		grep '^usage:' actual
>>> +	"
>>> +}
>>> +
>>> +test_extra_arg -f add nick url
>>> +test_extra_arg rename origin newname
>>
>> Perhaps just a taste in readability thing, but I would prefer to see
>> them more like
>>
>> 	test_extra_arg_expect failure add nick url
>> 	test_extra_arg_expect success rename origin newname
>>
>> than misunderstanding-inviting "-f" that often stands for "--force".
>
> Hmm.  I had that at first, but then the final cleanup would have had to
> touch all tests to remove the optional argument, making it noisy.

You do not need a final cleanup, as I _never_ meant failure/success in the above illustration to be _optional_. Being explicit reduces mental burden when you later have to read such a custom scaffolding each test invents in an ad-hoc manner to suit its needs.

Thomas Rast· Apr 24, 2013, 13:54 UTC · re: Thomas Rast · lore

[PATCH 2/3] remote: check for superfluous arguments in 'git remote add'

The 'git remote add' subcommand did not check for superfluous command line arguments. Make it so.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 builtin/remote.c  | 2 +-
 t/t5505-remote.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index 937484d..5e54d36 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -178,7 +178,7 @@ static int add(int argc, const char **argv)
 	argc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,
 			     0);
 
-	if (argc < 2)
+	if (argc != 2)
 		usage_with_options(builtin_remote_add_usage, options);
 
 	if (mirror && master)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 764ee97..eea87fc 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1020,7 +1020,7 @@ test_extra_arg () {
 	"
 }
 
-test_extra_arg -f add nick url
+test_extra_arg add nick url
 test_extra_arg rename origin newname
 test_extra_arg remove origin
 test_extra_arg set-head origin master

-- 
1.8.2.1.931.g0116868
Thomas Rast· Apr 24, 2013, 13:54 UTC · re: Thomas Rast · lore

[PATCH 3/3] remote: 'show' and 'prune' take more than one remote

The 'git remote show' and 'prune' subcommands are documented as taking only a single remote name argument, but that is not the case; they will simply iterate the action over all remotes given. Update the documentation and tests to match.

With the last user of the -f flag gone, we also remove the code supporting it.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 Documentation/git-remote.txt |  4 ++--
 t/t5505-remote.sh            | 11 +++--------
 2 files changed, 5 insertions(+), 10 deletions(-)
Show changes to 2 files +5 −10

Documentation/git-remote.txt, t/t5505-remote.sh

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index e8c396b..7a6f354 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -18,8 +18,8 @@ SYNOPSIS
 'git remote set-url' [--push] <name> <newurl> [<oldurl>]
 'git remote set-url --add' [--push] <name> <newurl>
 'git remote set-url --delete' [--push] <name> <url>
-'git remote' [-v | --verbose] 'show' [-n] <name>
-'git remote prune' [-n | --dry-run] <name>
+'git remote' [-v | --verbose] 'show' [-n] <name>...
+'git remote prune' [-n | --dry-run] <name>...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]
 
 DESCRIPTION
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index eea87fc..dd10ff0 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1009,12 +1009,7 @@ test_expect_success 'extra args: setup' '
 '
 
 test_extra_arg () {
-	expect="success"
-	if test "z$1" = "z-f"; then
-		expect=failure
-		shift
-	fi
-	test_expect_$expect "extra args: $*" "
+	test_expect_success "extra args: $*" "
 		test_must_fail git remote $* bogus_extra_arg 2>actual &&
 		grep '^usage:' actual
 	"
@@ -1026,8 +1021,8 @@ test_extra_arg remove origin
 test_extra_arg set-head origin master
 # set-branches takes any number of args
 test_extra_arg set-url origin newurl oldurl
-test_extra_arg -f show origin
-test_extra_arg -f prune origin
+# show takes any number of args
+# prune takes any number of args
 # update takes any number of args
 
 test_done

-- 
1.8.2.1.931.g0116868

← back to recent threads