threads / patch / 21417

patchclone: detect extra arguments

Subject: [PATCH] clone: detect extra arguments

## tl;dr

6 messages between Oct 29, 2009 and Oct 31, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jonathan Nieder· Oct 29, 2009, 08:10 UTC · lore

If git clone is given more than two non-option arguments, it silently throws away all but the first one. Complain instead.

Discovered by comparing the new builtin clone to the old git-clone.sh.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin-clone.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to builtin-clone.c +3 −0
diff --git a/builtin-clone.c b/builtin-clone.c
index 5762a6f..76ad581 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -377,6 +377,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 
+	if (argc > 2)
+		die("Too many arguments.");
+
 	if (argc == 0)
 		die("You must specify a repository to clone.");
 

-- 
1.6.5.2
Jeff King· Oct 29, 2009, 16:06 UTC · re: Jonathan Nieder · lore

Re: [PATCH] clone: detect extra arguments

On Thu, Oct 29, 2009 at 03:10:30AM -0500, Jonathan Nieder wrote:
Show 6 quoted lines
> If git clone is given more than two non-option arguments, it
> silently throws away all but the first one.  Complain instead.
> [...]
> +	if (argc > 2)
> +		die("Too many arguments.");
> +
Should we maybe be showing the usage in this case?
>  	if (argc == 0)
>  		die("You must specify a repository to clone.");
Probably we should do the same here, too.
-Peff
Jonathan Nieder· Oct 30, 2009, 11:19 UTC · re: Jeff King · lore

Re: [PATCH] clone: detect extra arguments

Jeff King wrote:
> Should we maybe be showing the usage in this case?
Sounds reasonable.  How about this patch on top?
-- %< --
Subject: [PATCH] clone: print usage on wrong number of arguments

git clone's short usage string is only 22 lines, so an error message plus usage string still fits comfortably on an 80x24 terminal.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin-clone.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to builtin-clone.c +4 −2
diff --git a/builtin-clone.c b/builtin-clone.c
index 76ad581..736d9e1 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -378,10 +378,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			     builtin_clone_usage, 0);
 
 	if (argc > 2)
-		die("Too many arguments.");
+		usage_msg_opt("Too many arguments.",
+			builtin_clone_usage, builtin_clone_options);
 
 	if (argc == 0)
-		die("You must specify a repository to clone.");
+		usage_msg_opt("You must specify a repository to clone.",
+			builtin_clone_usage, builtin_clone_options);
 
 	if (option_mirror)
 		option_bare = 1;

-- 
1.6.5.2
Jeff King· Oct 30, 2009, 14:45 UTC · re: Jonathan Nieder · lore

Re: [PATCH] clone: detect extra arguments

On Fri, Oct 30, 2009 at 06:19:19AM -0500, Jonathan Nieder wrote:
> > Should we maybe be showing the usage in this case?
> 
> Sounds reasonable.  How about this patch on top?
I do think it's an improvement, but...
Show 6 quoted lines
> -- %< --
> Subject: [PATCH] clone: print usage on wrong number of arguments
> 
> git clone's short usage string is only 22 lines, so an error
> message plus usage string still fits comfortably on an 80x24
> terminal.

The extra blank lines introduced by usage_msg_opt make it 25 lines, scrolling the message right off of my terminal screen. ;)

But looking at the usage message, there is some potential for cleanup. So maybe this on top (or between your 1 and 2)?

-- >8 --
Subject: [PATCH] clone: hide "naked" option from usage message

This is just a little-known synonym for bare, and there is little point in advertising both (we don't even include it in the manpage). Removing it also makes the usage message one line shorter, giving just enough room for an information message in a 24-line terminal.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-clone.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to builtin-clone.c +3 −1
diff --git a/builtin-clone.c b/builtin-clone.c
index 736d9e1..ce0d79a 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -51,7 +51,9 @@ static struct option builtin_clone_options[] = {
 	OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
 		    "don't create a checkout"),
 	OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
-	OPT_BOOLEAN(0, "naked", &option_bare, "create a bare repository"),
+	{ OPTION_BOOLEAN, 0, "naked", &option_bare, NULL,
+		"create a bare repository",
+		PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 	OPT_BOOLEAN(0, "mirror", &option_mirror,
 		    "create a mirror repository (implies bare)"),
 	OPT_BOOLEAN('l', "local", &option_local,

-- 
1.6.5.1.143.g1dab74.dirty
Jeff King· Oct 30, 2009, 14:51 UTC · re: Jeff King · lore

Re: [PATCH] clone: detect extra arguments

On Fri, Oct 30, 2009 at 10:45:25AM -0400, Jeff King wrote:
> But looking at the usage message, there is some potential for cleanup.

Also, we should probably do this (I did it as a patch on master, though, as it is an independent fix):

-- >8 --
Subject: [PATCH] clone: fix --recursive usage message
Looks like a mistaken cut-and-paste in e7fed18a.
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-clone.c +1 −1
diff --git a/builtin-clone.c b/builtin-clone.c
index 5762a6f..436e8da 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -61,7 +61,7 @@ static struct option builtin_clone_options[] = {
 	OPT_BOOLEAN('s', "shared", &option_shared,
 		    "setup as shared repository"),
 	OPT_BOOLEAN(0, "recursive", &option_recursive,
-		    "setup as shared repository"),
+		    "initialize submodules in the clone"),
 	OPT_STRING(0, "template", &option_template, "path",
 		   "path the template repository"),
 	OPT_STRING(0, "reference", &option_reference, "repo",

-- 
1.6.5.1.143.g1dab74.dirty
Johan Herland· Oct 31, 2009, 11:32 UTC · re: Jeff King · lore

Re: [PATCH] clone: detect extra arguments

On Friday 30 October 2009, Jeff King wrote:
Show 10 quoted lines
> On Fri, Oct 30, 2009 at 10:45:25AM -0400, Jeff King wrote:
> > But looking at the usage message, there is some potential for cleanup.
> 
> Also, we should probably do this (I did it as a patch on master, though,
> as it is an independent fix):
> 
> -- >8 --
> Subject: [PATCH] clone: fix --recursive usage message
> 
> Looks like a mistaken cut-and-paste in e7fed18a.
Yes. Please fix my screwup.
> Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Johan Herland <johan@herland.net>
Show 18 quoted lines
> ---
>  builtin-clone.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 5762a6f..436e8da 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -61,7 +61,7 @@ static struct option builtin_clone_options[] = {
>  	OPT_BOOLEAN('s', "shared", &option_shared,
>  		    "setup as shared repository"),
>  	OPT_BOOLEAN(0, "recursive", &option_recursive,
> -		    "setup as shared repository"),
> +		    "initialize submodules in the clone"),
>  	OPT_STRING(0, "template", &option_template, "path",
>  		   "path the template repository"),
>  	OPT_STRING(0, "reference", &option_reference, "repo",
> 
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net

← back to recent threads