[PATCH 0/4] Migrate simple builtins to parse-options

[PATCH 0/4] Migrate simple builtins to parse-options

[Stephen Boyd]

These are builtins with options that are fairly simple and the number
of options is small so code review will hopefully be easier.

Stephen Boyd (4):
  write-tree: migrate to parse-options
  tag-verify: migrate to parse-options
  verify-pack: migrate to parse-options
  prune-packed: migrate to parse-options

 Documentation/git-prune-packed.txt |    4 ++-
 Documentation/git-verify-pack.txt  |    3 +-
 builtin-prune-packed.c             |   29 ++++++++++---------------
 builtin-verify-pack.c              |   40 ++++++++++++++++-------------------
 builtin-verify-tag.c               |   21 ++++++++++--------
 builtin-write-tree.c               |   40 +++++++++++++++++------------------
 6 files changed, 66 insertions(+), 71 deletions(-)

[PATCH 1/4] write-tree: migrate to parse-options

[Stephen Boyd]

A check for extra options has been dropped, it could never be triggered
in the original code as the usage message would be printed instead.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

For historical purposes (and those interested), the die() became dead
code in commit 6bd2035 (write-tree: --prefix=<path> 2006-04-26) when
the line if (argc == 2) was changed to a while loop.

 builtin-write-tree.c |   40 +++++++++++++++++++---------------------
 1 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 3a24ce8..b223af4 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -7,9 +7,12 @@
 #include "cache.h"
 #include "tree.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
-static const char write_tree_usage[] =
-"git write-tree [--missing-ok] [--prefix=<prefix>/]";
+static const char * const write_tree_usage[] = {
+	"git write-tree [--missing-ok] [--prefix=<prefix>/]",
+	NULL
+};
 
 int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 {
@@ -17,27 +20,22 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	const char *prefix = NULL;
 	unsigned char sha1[20];
 	const char *me = "git-write-tree";
+	struct option write_tree_options[] = {
+		OPT_BIT(0, "missing-ok", &flags, "allow missing objects",
+			WRITE_TREE_MISSING_OK),
+		{ OPTION_STRING, 0, "prefix", &prefix, "<prefix>/",
+		  "write tree object for a subdirectory <prefix>" ,
+		  PARSE_OPT_LITERAL_ARGHELP },
+		{ OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
+		  "only useful for debugging",
+		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
+		  WRITE_TREE_IGNORE_CACHE_TREE },
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--missing-ok"))
-			flags |= WRITE_TREE_MISSING_OK;
-		else if (!prefixcmp(arg, "--prefix="))
-			prefix = arg + 9;
-		else if (!prefixcmp(arg, "--ignore-cache-tree"))
-			/*
-			 * This is only useful for debugging, so I
-			 * do not bother documenting it.
-			 */
-			flags |= WRITE_TREE_IGNORE_CACHE_TREE;
-		else
-			usage(write_tree_usage);
-		argc--; argv++;
-	}
-
-	if (argc > 2)
-		die("too many options");
+	argc = parse_options(argc, argv, unused_prefix, write_tree_options,
+			     write_tree_usage, 0);
 
 	ret = write_cache_as_tree(sha1, flags, prefix);
 	switch (ret) {
-- 
1.6.3.3.385.g60647

[PATCH 2/4] tag-verify: migrate to parse-options

[Stephen Boyd]

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

It may be nicer to die with a message like "Must give a tag to verify"
or something when a tag isn't given, but that could probably be a
later patch.

 builtin-verify-tag.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index 7f7fda4..9f482c2 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -10,9 +10,12 @@
 #include "tag.h"
 #include "run-command.h"
 #include <signal.h>
+#include "parse-options.h"
 
-static const char builtin_verify_tag_usage[] =
-		"git verify-tag [-v|--verbose] <tag>...";
+static const char * const verify_tag_usage[] = {
+		"git verify-tag [-v|--verbose] <tag>...",
+		NULL
+};
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 
@@ -89,17 +92,17 @@ static int verify_tag(const char *name, int verbose)
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
+	const struct option verify_tag_options[] = {
+		OPT__VERBOSE(&verbose),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 
-	if (argc > 1 &&
-	    (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose"))) {
-		verbose = 1;
-		i++;
-	}
-
+	argc = parse_options(argc, argv, prefix, verify_tag_options,
+			     verify_tag_usage, PARSE_OPT_KEEP_ARGV0);
 	if (argc <= i)
-		usage(builtin_verify_tag_usage);
+		usage_with_options(verify_tag_usage, verify_tag_options);
 
 	/* sometimes the program was terminated because this signal
 	 * was received in the process of writing the gpg input: */
-- 
1.6.3.3.385.g60647

[PATCH 3/4] verify-pack: migrate to parse-options

[Stephen Boyd]

OPT__VERBOSE adds a --verbose option, so document the new addition.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 Documentation/git-verify-pack.txt |    3 +-
 builtin-verify-pack.c             |   40 ++++++++++++++++--------------------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt
index c861163..d791a80 100644
--- a/Documentation/git-verify-pack.txt
+++ b/Documentation/git-verify-pack.txt
@@ -8,7 +8,7 @@ git-verify-pack - Validate packed git archive files
 
 SYNOPSIS
 --------
-'git verify-pack' [-v] [--] <pack>.idx ...
+'git verify-pack' [-v|--verbose] [--] <pack>.idx ...
 
 
 DESCRIPTION
@@ -23,6 +23,7 @@ OPTIONS
 	The idx files to verify.
 
 -v::
+--verbose::
 	After verifying the pack, show list of objects contained
 	in the pack.
 \--::
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 0ee0a9a..ebd6dff 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "pack.h"
 #include "pack-revindex.h"
+#include "parse-options.h"
 
 #define MAX_CHAIN 50
 
@@ -107,36 +108,31 @@ static int verify_one_pack(const char *path, int verbose)
 	return err;
 }
 
-static const char verify_pack_usage[] = "git verify-pack [-v] <pack>...";
+static const char * const verify_pack_usage[] = {
+	"git verify-pack [-v|--verbose] <pack>...",
+	NULL
+};
 
 int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 {
 	int err = 0;
 	int verbose = 0;
-	int no_more_options = 0;
-	int nothing_done = 1;
+	int i;
+	const struct option verify_pack_options[] = {
+		OPT__VERBOSE(&verbose),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
-	while (1 < argc) {
-		if (!no_more_options && argv[1][0] == '-') {
-			if (!strcmp("-v", argv[1]))
-				verbose = 1;
-			else if (!strcmp("--", argv[1]))
-				no_more_options = 1;
-			else
-				usage(verify_pack_usage);
-		}
-		else {
-			if (verify_one_pack(argv[1], verbose))
-				err = 1;
-			discard_revindex();
-			nothing_done = 0;
-		}
-		argc--; argv++;
+	argc = parse_options(argc, argv, prefix, verify_pack_options,
+			     verify_pack_usage, 0);
+	if (argc < 1)
+		usage_with_options(verify_pack_usage, verify_pack_options);
+	for (i = 0; i < argc; i++) {
+		if (verify_one_pack(argv[i], verbose))
+			err = 1;
+		discard_revindex();
 	}
 
-	if (nothing_done)
-		usage(verify_pack_usage);
-
 	return err;
 }
-- 
1.6.3.3.385.g60647

[PATCH 4/4] prune-packed: migrate to parse-options

[Stephen Boyd]

Add --dry-run and --quiet to be more consistent with the rest of git.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 Documentation/git-prune-packed.txt |    4 +++-
 builtin-prune-packed.c             |   29 ++++++++++++-----------------
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-prune-packed.txt b/Documentation/git-prune-packed.txt
index b5f26ce..abfc6b6 100644
--- a/Documentation/git-prune-packed.txt
+++ b/Documentation/git-prune-packed.txt
@@ -8,7 +8,7 @@ git-prune-packed - Remove extra objects that are already in pack files
 
 SYNOPSIS
 --------
-'git prune-packed' [-n] [-q]
+'git prune-packed' [-n|--dry-run] [-q|--quiet]
 
 
 DESCRIPTION
@@ -28,10 +28,12 @@ disk storage, etc.
 OPTIONS
 -------
 -n::
+--dry-run::
         Don't actually remove any objects, only show those that would have been
         removed.
 
 -q::
+--quiet::
 	Squelch the progress indicator.
 
 Author
diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index 00590b1..be99eb0 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -1,9 +1,12 @@
 #include "builtin.h"
 #include "cache.h"
 #include "progress.h"
+#include "parse-options.h"
 
-static const char prune_packed_usage[] =
-"git prune-packed [-n] [-q]";
+static const char * const prune_packed_usage[] = {
+	"git prune-packed [-n|--dry-run] [-q|--quiet]",
+	NULL
+};
 
 #define DRY_RUN 01
 #define VERBOSE 02
@@ -68,24 +71,16 @@ void prune_packed_objects(int opts)
 
 int cmd_prune_packed(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	int opts = VERBOSE;
+	const struct option prune_packed_options[] = {
+		OPT_BIT('n', "dry-run", &opts, "dry run", DRY_RUN),
+		OPT_NEGBIT('q', "quiet", &opts, "be quiet", VERBOSE),
+		OPT_END()
+	};
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, prefix, prune_packed_options,
+			     prune_packed_usage, 0);
 
-		if (*arg == '-') {
-			if (!strcmp(arg, "-n"))
-				opts |= DRY_RUN;
-			else if (!strcmp(arg, "-q"))
-				opts &= ~VERBOSE;
-			else
-				usage(prune_packed_usage);
-			continue;
-		}
-		/* Handle arguments here .. */
-		usage(prune_packed_usage);
-	}
 	prune_packed_objects(opts);
 	return 0;
 }
-- 
1.6.3.3.385.g60647

Re: [PATCH 4/4] prune-packed: migrate to parse-options

[Johannes Schindelin]

Hi,

I reviewed all four patches, and like them.  Just a minor nit:

On Tue, 7 Jul 2009, Stephen Boyd wrote:

> Add --dry-run and --quiet to be more consistent with the rest of git.

You should say here that you add the _long_ options (the short options 
were supported already); I almost expected you to sneak in additional 
features with the parseoptification.

Same goes for 3/4, I guess.

Thanks,
Dscho

Re: [PATCH 4/4] prune-packed: migrate to parse-options

[Stephen Boyd]

Johannes Schindelin wrote:
> Hi,
>
> I reviewed all four patches, and like them.  Just a minor nit:
>
> On Tue, 7 Jul 2009, Stephen Boyd wrote:
>   
>> Add --dry-run and --quiet to be more consistent with the rest of git.
>>     
>
> You should say here that you add the _long_ options (the short options 
> were supported already); I almost expected you to sneak in additional 
> features with the parseoptification.
>
> Same goes for 3/4, I guess.
>   


Sure, this sounds reasonable. I'll send follow up commit message fixups.

Re: [PATCH 3/4] verify-pack: migrate to parse-options

[Stephen Boyd]

Stephen Boyd wrote:
> OPT__VERBOSE adds a --verbose option, so document the new addition.
>   

OPT__VERBOSE introduces the long option (--verbose) in addition to the
already present short option (-v),  so document this new addition.

> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
>   

Re: [PATCH 4/4] prune-packed: migrate to parse-options

[Stephen Boyd]

Stephen Boyd wrote:
> Add --dry-run and --quiet to be more consistent with the rest of git.

Add long options for dry run and quiet to be more consistent with the
rest of git.

> Signed-off-by: Stephen Boyd <bebarino@gmail.com>