RE: [PATCH 9/10] Remove cmd_usage() routine and re-organize the help/usage code.

["Ramsay Jones" <ramsay@ramsay1.demon.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]


On Wed 2006-08-02 at 19:18, Junio C Hamano wrote:
> 
> Martin Waitz <tali@admingilde.org> writes:
> 
> > On Wed, Aug 02, 2006 at 02:03:44AM +0100, Ramsay Jones wrote:
> >>  builtin-help.c |   54
> >> +++++++++++++++++++++++-------------------------------
> >>  builtin.h      |    7 ++-----
> >>  git.c          |    7 +++++--
> >>  3 files changed, 30 insertions(+), 38 deletions(-)
> >
> > this patch is at the tip of "master" now, but with one more change:
> >...
> > diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
> > index bf1d638..34a3ccd 100755
> > --- a/t/t9100-git-svn-basic.sh
> > +++ b/t/t9100-git-svn-basic.sh
> >...
> > this looks strange.
> 
> Ramsay's patch to cmd_help() broke this test because the test
> relied on the details of output from "git help", which the patch
> subtly changed.
> 
> I considered making the fix for broken test a separate commit,
> but the fix for the test was simple enough, so I rolled it in,
> with the additional comment in the log to explain what was going
> on -- I suspect the explanation was not clear enough.
> 
> I could have committed the fix for the test first and then this
> one.
> 

I was surprised and concerned to read that (how did I miss the
failing test?), until I found that I didn't have this test! (Phew)
I assume this is a post 1.4.1 test, and from the name, I suppose
they would have mostly failed for me anyway since I don't have
svn installed.

In any event, I'm sorry to have broken your test, I should have
documented the changed behaviour. The change should be limited to
a lower-case usage prefix and a change in exit code from 1 to 129.
The change from "Usage:" to "usage:" was for consistency with every
other command (ignoring the test-delta program). The exit code
change only applies to one of the four original cmd_usage() call
sites; namely the one changed to a usage() call.  Hmmm, should
the other three "call sites" change to exit(129) as well?

Any other change in behavior was unintentional. Did I miss
something?

Speaking of consistency, I noticed some inconsistent command
names in various usage strings. I attach a patch (p0011.txt)
for your consideration.

During one of my "grep-fests", I also noticed some calls to
die(usage_string) rather than usage(usage_string). Calling die()
rather than usage() means that a "fatal: " (rather than "usage: ")
prefix is output prior the usage string, and the exit code
is 128 (rather than 129).

It looks like to choice of die() was deliberate, particularly in
builtin-rm.c and hash-object.c since they call both die() and
usage(). However, It's not clear to me why this choice was made.
(Which is not to say there isn't a perfectly good reason for the
choice; it's just that I don't see it.)

I attach a patch (p0012.txt) which replaces these calls to die()
with usage(). Since it is possible, however unlikely, that there
are some scripts out there that depend on the current behavior,
you may not want to apply it.

Cheers,

Ramsay

[-- Attachment #2: P0011.TXT --]
[-- Type: text/plain, Size: 2727 bytes --]

From 564d11cd55388f6a8de82b34b362543f73e0096c Mon Sep 17 00:00:00 2001
From: Ramsay Allan Jones <ramsay@ramsay1.demon.co.uk>
Date: Thu, 3 Aug 2006 16:38:39 +0100
Subject: [PATCH] Fixup command names in some usage strings.

Most usage strings, such as for command xxx, start with "git-xxx".
This updates the rebels to conform to the general pattern.
(The git wrapper is an exception to this, of course ...)

Signed-off-by: Ramsay Allan Jones <ramsay@ramsay1.demon.co.uk>
---
 blame.c        |    2 +-
 builtin-diff.c |    2 +-
 builtin-push.c |    2 +-
 mktag.c        |    2 +-
 mktree.c       |    2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/blame.c b/blame.c
index c86e2fd..2f19b4d 100644
--- a/blame.c
+++ b/blame.c
@@ -20,7 +20,7 @@ #include "xdiff-interface.h"
 
 #define DEBUG 0
 
-static const char blame_usage[] = "[-c] [-l] [-t] [-S <revs-file>] [--] file [commit]\n"
+static const char blame_usage[] = "git-blame [-c] [-l] [-t] [-S <revs-file>] [--] file [commit]\n"
 	"  -c, --compability Use the same output mode as git-annotate (Default: off)\n"
 	"  -l, --long        Show long commit SHA1 (Default: off)\n"
 	"  -t, --time        Show raw timestamp (Default: off)\n"
diff --git a/builtin-diff.c b/builtin-diff.c
index 99a2f76..f934dc0 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -23,7 +23,7 @@ struct blobinfo {
 };
 
 static const char builtin_diff_usage[] =
-"diff <options> <rev>{0,2} -- <path>*";
+"git-diff <options> <rev>{0,2} -- <path>*";
 
 static int builtin_diff_files(struct rev_info *revs,
 			      int argc, const char **argv)
diff --git a/builtin-push.c b/builtin-push.c
index 66b9407..57db489 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -8,7 +8,7 @@ #include "builtin.h"
 
 #define MAX_URI (16)
 
-static const char push_usage[] = "git push [--all] [--tags] [--force] <repository> [<refspec>...]";
+static const char push_usage[] = "git-push [--all] [--tags] [--force] <repository> [<refspec>...]";
 
 static int all = 0, tags = 0, force = 0, thin = 1;
 static const char *execute = NULL;
diff --git a/mktag.c b/mktag.c
index be41513..1d0cb17 100644
--- a/mktag.c
+++ b/mktag.c
@@ -123,7 +123,7 @@ int main(int argc, char **argv)
 	unsigned char result_sha1[20];
 
 	if (argc != 1)
-		usage("cat <signaturefile> | git-mktag");
+		usage("git-mktag < signaturefile");
 
 	setup_git_directory();
 
diff --git a/mktree.c b/mktree.c
index ab63cd9..9a6f0d2 100644
--- a/mktree.c
+++ b/mktree.c
@@ -71,7 +71,7 @@ static void write_tree(unsigned char *sh
 	write_sha1_file(buffer, offset, tree_type, sha1);
 }
 
-static const char mktree_usage[] = "mktree [-z]";
+static const char mktree_usage[] = "git-mktree [-z]";
 
 int main(int ac, char **av)
 {
-- 
1.4.1


[-- Attachment #3: P0012.TXT --]
[-- Type: text/plain, Size: 2649 bytes --]

From d5c3c850036f06c35416b8a515652c248bf4a73a Mon Sep 17 00:00:00 2001
From: Ramsay Allan Jones <ramsay@ramsay1.demon.co.uk>
Date: Thu, 3 Aug 2006 16:48:41 +0100
Subject: [PATCH] Replace some calls to die(usage_str) with usage(usage_str).

The only change in behaviour should be having a "usage: " prefix
on the output string rather than "fatal: ", and an exit code of
129 rather than 128.

Signed-off-by: Ramsay Allan Jones <ramsay@ramsay1.demon.co.uk>
---
 builtin-add.c        |    2 +-
 builtin-init-db.c    |    2 +-
 builtin-rm.c         |    2 +-
 builtin-write-tree.c |    2 +-
 hash-object.c        |    4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index bfbbb1b..ced84ea 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -158,7 +158,7 @@ int cmd_add(int argc, const char **argv,
 			verbose = 1;
 			continue;
 		}
-		die(builtin_add_usage);
+		usage(builtin_add_usage);
 	}
 	git_config(git_default_config);
 	pathspec = get_pathspec(prefix, argv + i);
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 7fdd2fa..85a2afd 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -267,7 +267,7 @@ int cmd_init_db(int argc, const char **a
 		else if (!strncmp(arg, "--shared=", 9))
 			shared_repository = git_config_perm("arg", arg+9);
 		else
-			die(init_db_usage);
+			usage(init_db_usage);
 	}
 
 	/*
diff --git a/builtin-rm.c b/builtin-rm.c
index 4d56a1f..bfacbb2 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -81,7 +81,7 @@ int cmd_rm(int argc, const char **argv, 
 			force = 1;
 			continue;
 		}
-		die(builtin_rm_usage);
+		usage(builtin_rm_usage);
 	}
 	if (argc <= i)
 		usage(builtin_rm_usage);
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 70e9b6f..bbe1f07 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -74,7 +74,7 @@ int cmd_write_tree(int argc, const char 
 		else if (!strncmp(arg, "--prefix=", 9))
 			prefix = arg + 9;
 		else
-			die(write_tree_usage);
+			usage(write_tree_usage);
 		argc--; argv++;
 	}
 
diff --git a/hash-object.c b/hash-object.c
index 43bd93b..bf0b492 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -46,7 +46,7 @@ int main(int argc, char **argv)
 		if (!no_more_flags && argv[i][0] == '-') {
 			if (!strcmp(argv[i], "-t")) {
 				if (argc <= ++i)
-					die(hash_object_usage);
+					usage(hash_object_usage);
 				type = argv[i];
 			}
 			else if (!strcmp(argv[i], "-w")) {
@@ -66,7 +66,7 @@ int main(int argc, char **argv)
 				hash_stdin(type, write_object);
 			}
 			else
-				die(hash_object_usage);
+				usage(hash_object_usage);
 		} 
 		else {
 			const char *arg = argv[i];
-- 
1.4.1