[PATCH] clone: detect extra arguments

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 <redacted>
---
 builtin-clone.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

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

On Thu, Oct 29, 2009 at 03:10:30AM -0500, Jonathan Nieder wrote:

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

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 <redacted>
---
 builtin-clone.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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

On Fri, Oct 30, 2009 at 06:19:19AM -0500, Jonathan Nieder wrote:

quoted

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...

-- %< --
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 <redacted>
---
 builtin-clone.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

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

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 <redacted>
---
 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",

-- 
1.6.5.1.143.g1dab74.dirty

On Friday 30 October 2009, Jeff King wrote:

On Fri, Oct 30, 2009 at 10:45:25AM -0400, Jeff King wrote:

quoted

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 <redacted>

Acked-by: Johan Herland <redacted>

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net