[PATCH 0/9] Extend use of parse_options()

[PATCH 0/9] Extend use of parse_options()

[Michele Ballabio]

These patches spread the use of parse_options() to some more commands.
I tried to come up with a short description for every option, but
I'm not sure I fully succeeded. In particular, the option "sparse"
in builtin-rev-list.c has no description.

Michele Ballabio (9):
  builtin-verify-tag.c: use parse_options()
  builtin-write-tree.c: use parse_options()
  builtin-prune-packed.c: use parse_options()
  builtin-ls-tree.c: use parse_options()
  builtin-rev-list.c: use parse_options()
  builtin-init-db.c: use parse_options()
  builtin-checkout-index.c: use parse_options()
  builtin-fetch-pack.c: use parse_options()
  builtin-mailinfo.c: use parse_options()

 builtin-checkout-index.c |  146 +++++++++++++++++++++++++---------------------
 builtin-fetch-pack.c     |  144 ++++++++++++++++++++++++++++-----------------
 builtin-init-db.c        |   56 +++++++++++-------
 builtin-ls-tree.c        |   92 +++++++++++------------------
 builtin-mailinfo.c       |   39 +++++++------
 builtin-prune-packed.c   |   38 ++++++------
 builtin-rev-list.c       |  132 ++++++++++++++++++++---------------------
 builtin-verify-tag.c     |   25 +++++---
 builtin-write-tree.c     |   31 +++++-----
 9 files changed, 376 insertions(+), 327 deletions(-)

[PATCH 1/9] builtin-verify-tag.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-verify-tag.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index 7d837f0..590038b 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -9,10 +9,13 @@
 #include "builtin.h"
 #include "tag.h"
 #include "run-command.h"
+#include "parse-options.h"
 #include <signal.h>
 
-static const char builtin_verify_tag_usage[] =
-		"git verify-tag [-v|--verbose] <tag>...";
+static const char * const builtin_verify_tag_usage[] = {
+	"git verify-tag [-v|--verbose] <tag>...",
+	NULL
+};
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 
@@ -88,23 +91,25 @@ 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;
+	int verbose = 0, had_error = 0;
 
 	git_config(git_default_config, NULL);
 
+	const struct option options[] = {
+		OPT__VERBOSE(&verbose),
+		OPT_END()
+	};
+
 	if (argc == 1)
-		usage(builtin_verify_tag_usage);
+		usage_with_options(builtin_verify_tag_usage, options);
 
-	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
-		verbose = 1;
-		i++;
-	}
+	argc = parse_options(argc, argv, options, builtin_verify_tag_usage, 0);
 
 	/* sometimes the program was terminated because this signal
 	 * was received in the process of writing the gpg input: */
 	signal(SIGPIPE, SIG_IGN);
-	while (i < argc)
-		if (verify_tag(argv[i++], verbose))
+	while (argc-- > 0)
+		if (verify_tag(*argv++, verbose))
 			had_error = 1;
 	return had_error;
 }
-- 
1.5.6.3

[PATCH 2/9] builtin-write-tree.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-write-tree.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 52a3c01..25f3d8a 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=<directory>/]",
+	NULL
+};
 
 int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 {
@@ -19,19 +22,19 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	const char *me = "git-write-tree";
 
 	git_config(git_default_config, NULL);
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--missing-ok"))
-			missing_ok = 1;
-		else if (!prefixcmp(arg, "--prefix="))
-			prefix = arg + 9;
-		else
-			usage(write_tree_usage);
-		argc--; argv++;
-	}
 
-	if (argc > 2)
-		die("too many options");
+	const struct option options[] = {
+		OPT_BOOLEAN(0, "missing-ok", &missing_ok,
+			    "disable existence check"),
+		OPT_STRING(0, "prefix", &prefix, "directory",
+			   "write a tree object for <directory>"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, write_tree_usage, 0);
+
+	if (argc > 0)
+		usage_with_options(write_tree_usage, options);
 
 	ret = write_cache_as_tree(sha1, missing_ok, prefix);
 	switch (ret) {
-- 
1.5.6.3

[PATCH 3/9] builtin-prune-packed.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-prune-packed.c |   38 ++++++++++++++++++--------------------
 1 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index 10cb8df..5866871 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -1,12 +1,15 @@
 #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] [-q]",
+	NULL
+};
 
 #define DRY_RUN 01
-#define VERBOSE 02
+#define QUIET 02
 
 static struct progress *progress;
 
@@ -43,7 +46,7 @@ void prune_packed_objects(int opts)
 	const char *dir = get_object_directory();
 	int len = strlen(dir);
 
-	if (opts == VERBOSE)
+	if (!opts)
 		progress = start_progress_delay("Removing duplicate objects",
 			256, 95, 2);
 
@@ -67,24 +70,19 @@ void prune_packed_objects(int opts)
 
 int cmd_prune_packed(int argc, const char **argv, const char *prefix)
 {
-	int i;
-	int opts = VERBOSE;
+	int opts = 0;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	const struct option options[] = {
+		OPT_BIT('n', "dry-run", &opts, "dry run", DRY_RUN),
+		OPT_BIT('q', "quiet", &opts, "be quiet", QUIET),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, prune_packed_usage, 0);
+
+	if (argc > 0)
+		usage_with_options(prune_packed_usage, options);
 
-		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.5.6.3

[PATCH 4/9] builtin-ls-tree.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-ls-tree.c |   92 +++++++++++++++++++++--------------------------------
 1 files changed, 36 insertions(+), 56 deletions(-)

diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index d25767a..a0b17aa 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c
@@ -9,6 +9,7 @@
 #include "commit.h"
 #include "quote.h"
 #include "builtin.h"
+#include "parse-options.h"
 
 static int line_termination = '\n';
 #define LS_RECURSIVE 1
@@ -22,8 +23,10 @@ static const char **pathspec;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
 
-static const char ls_tree_usage[] =
-	"git ls-tree [-d] [-r] [-t] [-l] [-z] [--name-only] [--name-status] [--full-name] [--abbrev[=<n>]] <tree-ish> [path...]";
+static const char * const ls_tree_usage[] = {
+	"git ls-tree [options] <tree-ish> [path...]",
+	NULL
+};
 
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
@@ -122,70 +125,47 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	unsigned char sha1[20];
 	struct tree *tree;
 
+	const struct option options[] = {
+		OPT_SET_INT('z', NULL, &line_termination,
+			    "\\0 line termination on output", 0),
+		OPT_BIT('r', NULL, &ls_options,
+			"recurse into sub-trees", LS_RECURSIVE),
+		OPT_BIT('d', NULL, &ls_options,
+			"show only the tree, not its children",
+			LS_TREE_ONLY),
+		OPT_BIT('t', NULL, &ls_options,
+			"show tree entries", LS_SHOW_TREES),
+		OPT_BIT('l', "long", &ls_options,
+			"show object size of blob (file) entries",
+			LS_SHOW_SIZE),
+		OPT_BIT(0, "name-only", &ls_options,
+			"list only filenames", LS_NAME_ONLY),
+		OPT_BIT(0, "name-status", &ls_options,
+			"same as --name-only", LS_NAME_ONLY),
+		OPT_SET_INT(0, "full-name", &chomp_prefix,
+			    "show the full path name", 0),
+		OPT__ABBREV(&abbrev),
+		OPT_END()
+	};
+
 	git_config(git_default_config, NULL);
 	ls_tree_prefix = prefix;
 	if (prefix && *prefix)
 		chomp_prefix = strlen(prefix);
-	while (1 < argc && argv[1][0] == '-') {
-		switch (argv[1][1]) {
-		case 'z':
-			line_termination = 0;
-			break;
-		case 'r':
-			ls_options |= LS_RECURSIVE;
-			break;
-		case 'd':
-			ls_options |= LS_TREE_ONLY;
-			break;
-		case 't':
-			ls_options |= LS_SHOW_TREES;
-			break;
-		case 'l':
-			ls_options |= LS_SHOW_SIZE;
-			break;
-		case '-':
-			if (!strcmp(argv[1]+2, "name-only") ||
-			    !strcmp(argv[1]+2, "name-status")) {
-				ls_options |= LS_NAME_ONLY;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "long")) {
-				ls_options |= LS_SHOW_SIZE;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "full-name")) {
-				chomp_prefix = 0;
-				break;
-			}
-			if (!prefixcmp(argv[1]+2, "abbrev=")) {
-				abbrev = strtoul(argv[1]+9, NULL, 10);
-				if (abbrev && abbrev < MINIMUM_ABBREV)
-					abbrev = MINIMUM_ABBREV;
-				else if (abbrev > 40)
-					abbrev = 40;
-				break;
-			}
-			if (!strcmp(argv[1]+2, "abbrev")) {
-				abbrev = DEFAULT_ABBREV;
-				break;
-			}
-			/* otherwise fallthru */
-		default:
-			usage(ls_tree_usage);
-		}
-		argc--; argv++;
-	}
+
+	argc = parse_options(argc, argv, options, ls_tree_usage, 0);
+
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
 	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
 		ls_options |= LS_SHOW_TREES;
 
-	if (argc < 2)
-		usage(ls_tree_usage);
-	if (get_sha1(argv[1], sha1))
-		die("Not a valid object name %s", argv[1]);
+	if (argc < 1)
+		usage_with_options(ls_tree_usage, options);
+	if (get_sha1(argv[0], sha1))
+		die("Not a valid object name %s", argv[0]);
 
-	pathspec = get_pathspec(prefix, argv + 2);
+	pathspec = get_pathspec(prefix, argv + 1);
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-- 
1.5.6.3

[PATCH 5/9] builtin-rev-list.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-rev-list.c |  132 +++++++++++++++++++++++++--------------------------
 1 files changed, 65 insertions(+), 67 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 893762c..9200b20 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -11,44 +11,16 @@
 #include "builtin.h"
 #include "log-tree.h"
 #include "graph.h"
+#include "parse-options.h"
 
 /* bits #0-15 in revision.h */
 
 #define COUNTED		(1u<<16)
 
-static const char rev_list_usage[] =
-"git rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
-"  limiting output:\n"
-"    --max-count=nr\n"
-"    --max-age=epoch\n"
-"    --min-age=epoch\n"
-"    --sparse\n"
-"    --no-merges\n"
-"    --remove-empty\n"
-"    --all\n"
-"    --branches\n"
-"    --tags\n"
-"    --remotes\n"
-"    --stdin\n"
-"    --quiet\n"
-"  ordering output:\n"
-"    --topo-order\n"
-"    --date-order\n"
-"    --reverse\n"
-"  formatting output:\n"
-"    --parents\n"
-"    --children\n"
-"    --objects | --objects-edge\n"
-"    --unpacked\n"
-"    --header | --pretty\n"
-"    --abbrev=nr | --no-abbrev\n"
-"    --abbrev-commit\n"
-"    --left-right\n"
-"  special purpose:\n"
-"    --bisect\n"
-"    --bisect-vars\n"
-"    --bisect-all"
-;
+static const char * const rev_list_usage[] = {
+	"git rev-list [OPTION] <commit-id>... [ -- paths... ]",
+	NULL
+};
 
 static struct rev_info revs;
 
@@ -575,15 +547,65 @@ static struct commit_list *find_bisection(struct commit_list *list,
 	return best;
 }
 
+static int parse_header_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct rev_info *t_revs = opt->value;
+	t_revs->verbose_header = unset ? 0 : 1;
+	return 0;
+}
+
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
-	int i;
 	int read_from_stdin = 0;
 	int bisect_show_vars = 0;
 	int bisect_find_all = 0;
 	int quiet = 0;
 
+	const struct option options[] = {
+		OPT_GROUP("limiting output:"),
+		OPT_ARGUMENT("max-count=nr", "limit number of commits output"),
+		OPT_ARGUMENT("max-age=epoch", "limit commits output by time"),
+		OPT_ARGUMENT("min-age=epoch", "limit commits output by time"),
+		OPT_ARGUMENT("sparse", ""),
+		OPT_ARGUMENT("no-merges", "do not print merges"),
+		OPT_ARGUMENT("remove-empty", "stop when a given path disappears from the tree"),
+		OPT_ARGUMENT("all", "all refs"),
+		OPT_ARGUMENT("branches", "show local branches"),
+		OPT_ARGUMENT("tags", "show tags"),
+		OPT_ARGUMENT("remotes", "show remote-tracking branches"),
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			    "read commits also from command line"),
+		OPT__QUIET(&quiet),
+		OPT_GROUP("ordering output:"),
+		OPT_ARGUMENT("topo-order", "show commits in topological order"),
+		OPT_ARGUMENT("date-order", "use date order, preserving topology"),
+		OPT_ARGUMENT("reverse", "output commits in reverse order"),
+		OPT_GROUP("formatting output:"),
+		OPT_ARGUMENT("parents", "print the parents of the commit"),
+		OPT_ARGUMENT("children", "print the children of the commit"),
+		OPT_ARGUMENT("objects", "print all objects"),
+		OPT_ARGUMENT("objects-edge", "similar to --objects, used by git-pack-objects"),
+		OPT_ARGUMENT("unpacked", "print objects not in packs"),
+		{ OPTION_CALLBACK, 0, "header", &revs, NULL,
+		  "use raw-format", PARSE_OPT_NOARG, parse_header_cb, 0 },
+		OPT_ARGUMENT("pretty", "print contents in a given format"),
+		OPT_BOOLEAN(0, "timestamp", &show_timestamp,
+			    "print the raw commit timestamp"),
+		OPT_ARGUMENT("abbrev-commit", "show short sha1"),
+		OPT_ARGUMENT("abbrev=nr", "number of digits used for short sha1"),
+		OPT_ARGUMENT("no-abbrev", "do not use short sha1"),
+		OPT_ARGUMENT("left-right", "mark side of symmetric diff"),
+		OPT_ARGUMENT("graph", "show an ASCII graph"),
+		OPT_GROUP("special purpose:"),
+		OPT_BOOLEAN(0, "bisect", &bisect_list, "useful for binary searches"),
+		OPT_BOOLEAN(0, "bisect-all", &bisect_find_all,
+			    "order commits by their distance from given commits"),
+		OPT_BOOLEAN(0, "bisect-vars", &bisect_show_vars,
+			    "like --bisect, but ready to be eval'ed"),
+		OPT_END()
+	};
+
 	git_config(git_default_config, NULL);
 	init_revisions(&revs, prefix);
 	revs.abbrev = 0;
@@ -591,40 +613,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	argc = setup_revisions(argc, argv, &revs, NULL);
 
 	quiet = DIFF_OPT_TST(&revs.diffopt, QUIET);
-	for (i = 1 ; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, options, rev_list_usage, 0);
 
-		if (!strcmp(arg, "--header")) {
-			revs.verbose_header = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--timestamp")) {
-			show_timestamp = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--bisect")) {
-			bisect_list = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--bisect-all")) {
-			bisect_list = 1;
-			bisect_find_all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--bisect-vars")) {
-			bisect_list = 1;
-			bisect_show_vars = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (read_from_stdin++)
-				die("--stdin given twice?");
-			read_revisions_from_stdin(&revs);
-			continue;
-		}
-		usage(rev_list_usage);
+	if (argc > 0)
+		usage_with_options(rev_list_usage, options);
+
+	if (bisect_find_all || bisect_show_vars)
+		bisect_list = 1;
+	if (read_from_stdin)
+		read_revisions_from_stdin(&revs);
 
-	}
 	if (revs.commit_format != CMIT_FMT_UNSPECIFIED) {
 		/* The command line has a --pretty  */
 		hdr_termination = '\n';
@@ -643,7 +641,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	     (!(revs.tag_objects||revs.tree_objects||revs.blob_objects) &&
 	      !revs.pending.nr)) ||
 	    revs.diff)
-		usage(rev_list_usage);
+		usage_with_options(rev_list_usage, options);
 
 	save_commit_buffer = revs.verbose_header || revs.grep_filter;
 	if (bisect_list)
-- 
1.5.6.3

[PATCH 6/9] builtin-init-db.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-init-db.c |   56 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..ea1bed7 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -353,8 +354,17 @@ static int guess_repository_type(const char *git_dir)
 	return 1;
 }
 
-static const char init_db_usage[] =
-"git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]]";
+static const char * const init_db_usage[] = {
+	"git init [-q | --quiet] [--bare] [--template=<dir>] [--shared[=<type>]]",
+	NULL
+};
+
+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+			       int unset)
+{
+	*(int *)(opt->value) = unset ? 0 : git_config_perm("arg", arg);
+	return 0;
+}
 
 /*
  * If you want to, you can share the DB area with any number of branches.
@@ -367,25 +377,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
-	int i;
-
-	for (i = 1; i < argc; i++, argv++) {
-		const char *arg = argv[1];
-		if (!prefixcmp(arg, "--template="))
-			template_dir = arg+11;
-		else if (!strcmp(arg, "--bare")) {
-			static char git_dir[PATH_MAX+1];
-			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
-						sizeof(git_dir)), 0);
-		} else if (!strcmp(arg, "--shared"))
-			shared_repository = PERM_GROUP;
-		else if (!prefixcmp(arg, "--shared="))
-			shared_repository = git_config_perm("arg", arg+9);
-		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
-			flags |= INIT_DB_QUIET;
-		else
-			usage(init_db_usage);
+	int bare = 0;
+
+	const struct option options[] = {
+		OPT_STRING(0, "template", &template_dir, "dir",
+			   "directory from which templates will be used"),
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
+		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
+		  "type", "type of shared repository",
+		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
+		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, init_db_usage, 0);
+
+	if (argc > 0)
+		usage_with_options(init_db_usage, options);
+
+	if (bare) {
+		static char git_dir[PATH_MAX+1];
+		is_bare_repository_cfg = 1;
+		setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
+					sizeof(git_dir)), 0);
 	}
 
 	/*
-- 
1.5.6.3

[PATCH 7/9] builtin-checkout-index.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-checkout-index.c |  146 +++++++++++++++++++++++++---------------------
 1 files changed, 79 insertions(+), 67 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 71ebabf..429c850 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,18 +154,76 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const checkout_cache_usage[] = {
+	"git checkout-index [options] [--] <file>...",
+	NULL
+};
+
+static int parse_state_force_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct checkout *t_state = opt->value;
+	t_state->force = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_state_quiet_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct checkout *t_state = opt->value;
+	t_state->quiet = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_state_no_create_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct checkout *t_state = opt->value;
+	t_state->not_new = 1;
+	return 0;
+}
+
+static int parse_state_index_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct checkout *t_state = opt->value;
+	t_state->refresh_cache = unset ? 0 : 1;
+	return 0;
+}
 
 static struct lock_file lock_file;
 
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	int newfd = -1;
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	char *stage = NULL;
+
+	const struct option options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			    "checks out all files in the index"),
+		{ OPTION_CALLBACK, 'f', "force", &state, NULL,
+		  "force overwrite of existing files",
+		  PARSE_OPT_NOARG, parse_state_force_cb, 0 },
+		{ OPTION_CALLBACK, 'q', "quiet", &state, NULL, "be quiet",
+		  PARSE_OPT_NOARG, parse_state_quiet_cb, 0 },
+		{ OPTION_CALLBACK, 'n', "no-create", &state, NULL,
+		  "do not checkout new files, refresh existing ones",
+		  PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+		  parse_state_no_create_cb, 0 },
+		{ OPTION_CALLBACK, 'u', "index", &state, NULL,
+		  "update stat information in the index",
+		  PARSE_OPT_NOARG, parse_state_index_cb, 0 },
+		OPT_SET_INT('z', NULL, &line_termination,
+			    "separate paths with NUL", 0),
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			    "read paths from stdin"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			    "write content to temporary files"),
+		OPT_STRING(0, "prefix", &state.base_dir, "string",
+			   "prepend <string> when creating files"),
+		OPT_STRING(0, "stage", &stage, "1|2|3|all",
+			   "copy out files from the named stage"),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,71 +233,24 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, options, checkout_cache_usage, 0);
 
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
+	if ((state.refresh_cache) && (newfd < 0))
+		newfd = hold_locked_index(&lock_file, 1);
+	if (state.base_dir)
+		state.base_dir_len = strlen(state.base_dir);
+
+	if (stage) {
+		if (!strcmp(stage, "all")) {
 			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
+			checkout_stage = CHECKOUT_ALL;
+		} else {
+			int ch = stage[0];
+			if ('1' <= ch && ch <= '3')
+				checkout_stage = stage[0] - '0';
+			else
+				die("stage should be between 1 and 3 or all");
 		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
 	}
 
 	if (state.base_dir_len || to_tempfile) {
@@ -253,8 +265,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
-		const char *arg = argv[i];
+	while (argc-- > 0) {
+		const char *arg = *argv++;
 		const char *p;
 
 		if (all)
-- 
1.5.6.3

[PATCH 8/9] builtin-fetch-pack.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-fetch-pack.c |  144 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 90 insertions(+), 54 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 273239a..701be41 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -9,6 +9,7 @@
 #include "fetch-pack.h"
 #include "remote.h"
 #include "run-command.h"
+#include "parse-options.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -17,8 +18,10 @@ static struct fetch_pack_args args = {
 	/* .uploadpack = */ "git-upload-pack",
 };
 
-static const char fetch_pack_usage[] =
-"git fetch-pack [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] [--no-progress] [-v] [<host>:]<directory> [<refs>...]";
+static const char * const fetch_pack_usage[] = {
+	"git fetch-pack [options] [<host>:]<directory> [<refs>...]",
+	NULL
+};
 
 #define COMPLETE	(1U << 0)
 #define COMMON		(1U << 1)
@@ -667,6 +670,56 @@ static void fetch_pack_setup(void)
 	did_setup = 1;
 }
 
+static int parse_opt_keep_pack_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->lock_pack = t_args->keep_pack;
+	t_args->keep_pack = 1;
+	return 0;
+}
+
+static int parse_opt_thin_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->use_thin_pack = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_opt_include_tag_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->include_tag = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_opt_no_progress_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->no_progress = 1;
+	return 0;
+}
+
+static int parse_opt_fetch_all_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->fetch_all = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_opt_quiet_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->quiet = unset ? 0 : 1;
+	return 0;
+}
+
+static int parse_opt_verbose_cb(const struct option *opt, const char *arg, int unset)
+{
+	struct fetch_pack_args *t_args = opt->value;
+	t_args->verbose = unset ? 0 : 1;
+	return 0;
+}
+
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, ret, nr_heads;
@@ -677,60 +730,43 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 
 	nr_heads = 0;
 	heads = NULL;
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
 
-		if (*arg == '-') {
-			if (!prefixcmp(arg, "--upload-pack=")) {
-				args.uploadpack = arg + 14;
-				continue;
-			}
-			if (!prefixcmp(arg, "--exec=")) {
-				args.uploadpack = arg + 7;
-				continue;
-			}
-			if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
-				args.quiet = 1;
-				continue;
-			}
-			if (!strcmp("--keep", arg) || !strcmp("-k", arg)) {
-				args.lock_pack = args.keep_pack;
-				args.keep_pack = 1;
-				continue;
-			}
-			if (!strcmp("--thin", arg)) {
-				args.use_thin_pack = 1;
-				continue;
-			}
-			if (!strcmp("--include-tag", arg)) {
-				args.include_tag = 1;
-				continue;
-			}
-			if (!strcmp("--all", arg)) {
-				args.fetch_all = 1;
-				continue;
-			}
-			if (!strcmp("-v", arg)) {
-				args.verbose = 1;
-				continue;
-			}
-			if (!prefixcmp(arg, "--depth=")) {
-				args.depth = strtol(arg + 8, NULL, 0);
-				continue;
-			}
-			if (!strcmp("--no-progress", arg)) {
-				args.no_progress = 1;
-				continue;
-			}
-			usage(fetch_pack_usage);
-		}
-		dest = (char *)arg;
-		heads = (char **)(argv + i + 1);
-		nr_heads = argc - i - 1;
-		break;
-	}
+	const struct option options[] = {
+		{ OPTION_CALLBACK, 0, "all", &args, NULL,
+		 "fetch all remote refs", PARSE_OPT_NOARG,
+		 parse_opt_fetch_all_cb },
+		OPT_STRING(0, "upload-pack", &args.uploadpack, "git-upload-pack",
+			   "specify path to git-upload-pack on remote"),
+		OPT_STRING(0, "exec", &args.uploadpack, "git-upload-pack",
+			   "same as --upload-pack <git-upload-pack>."),
+		{ OPTION_CALLBACK, 0, "no-progress", &args, NULL,
+		 "do not show the progress", PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+		 parse_opt_no_progress_cb },
+		{ OPTION_CALLBACK, 'q', "quiet", &args, NULL,
+		 "be quiet", PARSE_OPT_NOARG, parse_opt_quiet_cb },
+		{ OPTION_CALLBACK, 'v', "verbose", &args, NULL,
+		 "be verbose", PARSE_OPT_NOARG, parse_opt_verbose_cb },
+		OPT_INTEGER(0, "depth", &args.depth, "fetch chains not longer than <n>"),
+		{ OPTION_CALLBACK, 'k', "keep", &args, NULL,
+		 "create a single packfile of received data",
+		 PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_keep_pack_cb },
+		{ OPTION_CALLBACK, 0, "include-tag", &args, NULL,
+		 "download annotated tags too", PARSE_OPT_NOARG,
+		 parse_opt_include_tag_cb },
+		{ OPTION_CALLBACK, 0, "thin", &args, NULL,
+		 "minimize number of objects to be sent",
+		 PARSE_OPT_NOARG, parse_opt_thin_cb },
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, fetch_pack_usage, 0);
+
+	dest = (char *)argv[0];
+	heads = (char **)(argv + 1);
+	nr_heads = argc - 1;
+
 	if (!dest)
-		usage(fetch_pack_usage);
+		usage_with_options(fetch_pack_usage, options);
 
 	conn = git_connect(fd, (char *)dest, args.uploadpack,
 			   args.verbose ? CONNECT_VERBOSE : 0);
-- 
1.5.6.3

[PATCH 9/9] builtin-mailinfo.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-mailinfo.c |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index f974b9d..f1ed269 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -6,6 +6,7 @@
 #include "builtin.h"
 #include "utf8.h"
 #include "strbuf.h"
+#include "parse-options.h"
 
 static FILE *cmitmsg, *patchfile, *fin, *fout;
 
@@ -905,8 +906,10 @@ static int mailinfo(FILE *in, FILE *out, int ks, const char *encoding,
 	return 0;
 }
 
-static const char mailinfo_usage[] =
-	"git mailinfo [-k] [-u | --encoding=<encoding> | -n] msg patch <mail >info";
+static const char * const mailinfo_usage[] = {
+	"git mailinfo [-k] [-u | --encoding=<encoding> | -n] msg patch <mail >info",
+	NULL
+};
 
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
@@ -920,22 +923,22 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 	def_charset = (git_commit_encoding ? git_commit_encoding : "utf-8");
 	metainfo_charset = def_charset;
 
-	while (1 < argc && argv[1][0] == '-') {
-		if (!strcmp(argv[1], "-k"))
-			keep_subject = 1;
-		else if (!strcmp(argv[1], "-u"))
-			metainfo_charset = def_charset;
-		else if (!strcmp(argv[1], "-n"))
-			metainfo_charset = NULL;
-		else if (!prefixcmp(argv[1], "--encoding="))
-			metainfo_charset = argv[1] + 11;
-		else
-			usage(mailinfo_usage);
-		argc--; argv++;
-	}
+	const struct option options[] = {
+		OPT_BOOLEAN('k', NULL, &keep_subject,
+			    "keep subject, don't clean it up"),
+		OPT_SET_PTR('u', NULL, &metainfo_charset,
+			    "re-code in UTF-8", (intptr_t)def_charset),
+		OPT_SET_PTR('n', NULL, &metainfo_charset,
+			    "disable re-coding", (intptr_t)NULL),
+		OPT_STRING(0, "encoding", &metainfo_charset,
+			   "encoding", "override default encoding"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, mailinfo_usage, 0);
 
-	if (argc != 3)
-		usage(mailinfo_usage);
+	if (argc != 2)
+		usage_with_options(mailinfo_usage, options);
 
-	return !!mailinfo(stdin, stdout, keep_subject, metainfo_charset, argv[1], argv[2]);
+	return !!mailinfo(stdin, stdout, keep_subject, metainfo_charset, argv[0], argv[1]);
 }
-- 
1.5.6.3

Re: [PATCH 7/9] builtin-checkout-index.c: use parse_options()

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Michele Ballabio wrote:

> +		{ OPTION_CALLBACK, 'f', "force", &state, NULL,
> +		  "force overwrite of existing files",
> +		  PARSE_OPT_NOARG, parse_state_force_cb, 0 },

I wonder if this could not be written as

		OPT_BOOLEAN('f', "force", &state.force,
			"force overwrite of existing files"),

Ciao,
Dscho

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Olivier Marin]

Michele Ballabio a écrit :
> 
> +	const struct option options[] = {
> +		OPT_STRING(0, "template", &template_dir, "dir",
> +			   "directory from which templates will be used"),

Perhaps "path", "path to the template repository" to stay consistent with clone.

> +		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),

s/set up/setup/ and s/repo/repository/?

> +		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
> +		  "type", "type of shared repository",

What about "permissions", "setup a shared repository"?

> +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },

Are you sure the default value is really used here?

Also, perhaps we can play it safer by avoiding changing "share_repository"
directly.

$ git init -> shared_repository == PERM_UMASK
$ git init --shared --no-shared -> shared_repository == 0

It works because PERM_UMASK == 0, but it is a side effect. Don't you think?

> +		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),

OPT__QUIET(&quiet),

if (quiet)
	flags |= INIT_DB_QUIET;

to use the same quiet option everywhere?

Just my opinion,
Olivier.

Re: [PATCH 1/9] builtin-verify-tag.c: use parse_options()

[Olivier Marin]

Michele Ballabio a écrit :
> 
>  	if (argc == 1)
> -		usage(builtin_verify_tag_usage);
> +		usage_with_options(builtin_verify_tag_usage, options);

It seems this is broken since the C rewrite: "git verify-tag -v" just do
nothing instead of printing usage message.

Moving the if() after parse_options() call with s/argc == 1/argc == 0/
should do the trick.

> -	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
> -		verbose = 1;
> -		i++;
> -	}
> +	argc = parse_options(argc, argv, options, builtin_verify_tag_usage, 0);

Olivier.

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Johannes Schindelin]

Hi,

On Thu, 24 Jul 2008, Olivier Marin wrote:

> Michele Ballabio a ??rit :
> 
> > +		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
> 
> s/set up/setup/

No.  "setup" is a noun.

> > +		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
> > +		  "type", "type of shared repository",
> 
> What about "permissions", "setup a shared repository"?
> 
> > +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
> 
> Are you sure the default value is really used here?
> 
> Also, perhaps we can play it safer by avoiding changing "share_repository"
> directly.

I do not see how that would be any safer.

> $ git init -> shared_repository == PERM_UMASK
> $ git init --shared --no-shared -> shared_repository == 0
> 
> It works because PERM_UMASK == 0, but it is a side effect. Don't you think?

Then the callback is wrong, too.  I think, however, that it is by design, 
and correct.

We rely on shared_repository == 0 for non-shared repositories _almost 
everywhere_.

> > +		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
> 
> OPT__QUIET(&quiet),
> 
> if (quiet)
> 	flags |= INIT_DB_QUIET;
> 
> to use the same quiet option everywhere?

Why?  Doesn't make it more readable, I think.  I'd rather have 3 lines 
less.

Ciao,
Dscho

Re: [PATCH 1/9] builtin-verify-tag.c: use parse_options()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 751 bytes --]

Hi,

On Thu, 24 Jul 2008, Olivier Marin wrote:

> Michele Ballabio a écrit :
> > 
> >  	if (argc == 1)
> > -		usage(builtin_verify_tag_usage);
> > +		usage_with_options(builtin_verify_tag_usage, options);
> 
> It seems this is broken since the C rewrite: "git verify-tag -v" just do
> nothing instead of printing usage message.
> 
> Moving the if() after parse_options() call with s/argc == 1/argc == 0/
> should do the trick.

That would be a bugfix.  As such, it belongs into a different commit.  
Care to provide a patch?

> > -	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
> > -		verbose = 1;
> > -		i++;
> > -	}
> > +	argc = parse_options(argc, argv, options, builtin_verify_tag_usage, 0);

Why did you quote this?

Ciao,
Dscho

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Michele Ballabio]

On Thursday 24 July 2008, Olivier Marin wrote:
> Michele Ballabio a écrit :
> > 
> > +	const struct option options[] = {
> > +		OPT_STRING(0, "template", &template_dir, "dir",
> > +			   "directory from which templates will be used"),
> 
> Perhaps "path", "path to the template repository" to stay consistent with clone.

Ok.

> > +		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
> 
> s/set up/setup/ and s/repo/repository/?

I think "set up a bare repository" will be fine.

> > +		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
> > +		  "type", "type of shared repository",
> 
> What about "permissions", "setup a shared repository"?

Ok, but with s/setup/set up/.

> > +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
> 
> Are you sure the default value is really used here?

Yes. Perhaps I don't understand your question. Can you explain what you mean?

> Also, perhaps we can play it safer by avoiding changing "share_repository"
> directly.
> 
> $ git init -> shared_repository == PERM_UMASK
> $ git init --shared --no-shared -> shared_repository == 0
> 
> It works because PERM_UMASK == 0, but it is a side effect. Don't you think?

Would you like this better, with PARSE_OPT_NONEG?

+               { OPTION_CALLBACK, 0, "shared", &shared_repository,
+                 "permissions", "set up a shared repository",
+                 PARSE_OPT_OPTARG | PARSE_OPT_NONEG, parse_opt_shared_cb, PERM_GROUP },

Or do you prefer changing the callback like this:

+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+                              int unset)
+{
+       *(int *)(opt->value) = unset ? PERM_UMASK : git_config_perm("arg", arg);
+       return 0;
+}

> > +		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
> 
> OPT__QUIET(&quiet),
> 
> if (quiet)
> 	flags |= INIT_DB_QUIET;
> 
> to use the same quiet option everywhere?

I thought about it and decided against ;)
And it's one line vs four (counting "int quiet = 0;"). But I see your point.

Re: [PATCH 7/9] builtin-checkout-index.c: use parse_options()

[Michele Ballabio]

On Thursday 24 July 2008, Johannes Schindelin wrote:
> On Wed, 23 Jul 2008, Michele Ballabio wrote:
> 
> > +		{ OPTION_CALLBACK, 'f', "force", &state, NULL,
> > +		  "force overwrite of existing files",
> > +		  PARSE_OPT_NOARG, parse_state_force_cb, 0 },
> 
> I wonder if this could not be written as
> 
> 		OPT_BOOLEAN('f', "force", &state.force,
> 			"force overwrite of existing files"), 

I did it that way because 'force' is a bitfield.

Re: [PATCH 7/9] builtin-checkout-index.c: use parse_options()

[Sverre Rabbelier]

On Thu, Jul 24, 2008 at 10:08 PM, Michele Ballabio
<barra_cuda@katamail.com> wrote:
> On Thursday 24 July 2008, Johannes Schindelin wrote:
>> On Wed, 23 Jul 2008, Michele Ballabio wrote:
>>
>> > +           { OPTION_CALLBACK, 'f', "force", &state, NULL,
>> > +             "force overwrite of existing files",
>> > +             PARSE_OPT_NOARG, parse_state_force_cb, 0 },
>>
>> I wonder if this could not be written as
>>
>>               OPT_BOOLEAN('f', "force", &state.force,
>>                       "force overwrite of existing files"),
>
> I did it that way because 'force' is a bitfield.

I thought there is an OPT_BIT?

-- 
Cheers,

Sverre Rabbelier

[PATCH 6/9 - v2] builtin-init-db.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---

On Thursday 24 July 2008, Michele Ballabio wrote:
> +static int parse_opt_shared_cb(const struct option *opt, const char *arg,
> +                              int unset)
> +{
> +       *(int *)(opt->value) = unset ? PERM_UMASK : git_config_perm("arg", arg);
> +       return 0;
> +}
> 

Did it this way (and changed help strings).

 builtin-init-db.c |   57 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..42c2e20 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -353,8 +354,18 @@ static int guess_repository_type(const char *git_dir)
 	return 1;
 }
 
-static const char init_db_usage[] =
-"git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]]";
+static const char * const init_db_usage[] = {
+	"git init [-q | --quiet] [--bare] [--template=<dir>] [--shared[=<type>]]",
+	NULL
+};
+
+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+			       int unset)
+{
+	*(int *)(opt->value) = unset ? PERM_UMASK :
+				       git_config_perm("arg", arg);
+	return 0;
+}
 
 /*
  * If you want to, you can share the DB area with any number of branches.
@@ -367,25 +378,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
-	int i;
-
-	for (i = 1; i < argc; i++, argv++) {
-		const char *arg = argv[1];
-		if (!prefixcmp(arg, "--template="))
-			template_dir = arg+11;
-		else if (!strcmp(arg, "--bare")) {
-			static char git_dir[PATH_MAX+1];
-			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
-						sizeof(git_dir)), 0);
-		} else if (!strcmp(arg, "--shared"))
-			shared_repository = PERM_GROUP;
-		else if (!prefixcmp(arg, "--shared="))
-			shared_repository = git_config_perm("arg", arg+9);
-		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
-			flags |= INIT_DB_QUIET;
-		else
-			usage(init_db_usage);
+	int bare = 0;
+
+	const struct option options[] = {
+		OPT_STRING(0, "template", &template_dir, "path",
+			   "path to the template directory"),
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repository"),
+		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
+		  "permissions", "set up a shared repository",
+		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
+		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, init_db_usage, 0);
+
+	if (argc > 0)
+		usage_with_options(init_db_usage, options);
+
+	if (bare) {
+		static char git_dir[PATH_MAX+1];
+		is_bare_repository_cfg = 1;
+		setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
+					sizeof(git_dir)), 0);
 	}
 
 	/*
-- 
1.5.6.3

Re: [PATCH 7/9] builtin-checkout-index.c: use parse_options()

[René Scharfe]

Sverre Rabbelier schrieb:
> On Thu, Jul 24, 2008 at 10:08 PM, Michele Ballabio
> <barra_cuda@katamail.com> wrote:
>> On Thursday 24 July 2008, Johannes Schindelin wrote:
>>> On Wed, 23 Jul 2008, Michele Ballabio wrote:
>>>
>>>> +           { OPTION_CALLBACK, 'f', "force", &state, NULL,
>>>> +             "force overwrite of existing files",
>>>> +             PARSE_OPT_NOARG, parse_state_force_cb, 0 },
>>> I wonder if this could not be written as
>>>
>>>               OPT_BOOLEAN('f', "force", &state.force,
>>>                       "force overwrite of existing files"),
>> I did it that way because 'force' is a bitfield.
> 
> I thought there is an OPT_BIT?

OPT_BIT is for flags and bitmasks, not for bitfields.

Since you can't get the address of a bitfield member, a function that
wants to change its value needs to know its name.  Switching to bitmasks
would make the option parsing code look cleaner, but you'd have to
change all those bitfield accesses to explicit bitmask operations, e.g.:

	if (state.force)
		state.force = 0;

vs.

	if (state.flags & CHECKOUT_FORCE)
		state.flags &= ~CHECKOUT_FORCE;

In the case of struct checkout, though, we could simply make the
bitfield members full ints, because there are only a few instances of
this structure in memory at any given time.  Wasting a few bytes of RAM
in order to gain much simpler code is OK in this case, I think.
OPT_BOOLEAN looks a lot nicer than a callback.

René

Re: [PATCH 1/9] builtin-verify-tag.c: use parse_options()

[Olivier Marin]

Johannes Schindelin a écrit :
> 
> That would be a bugfix.  As such, it belongs into a different commit.  

I thought, for that kind of trivial bug that probably never hit anyone,
a line in the commit message was enough.

> Care to provide a patch?

OK, will do.

Olivier.

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Olivier Marin]

Johannes Schindelin a écrit :
>>
>>> +		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
>> s/set up/setup/
> 
> No.  "setup" is a noun.

Right, sorry.

> We rely on shared_repository == 0 for non-shared repositories _almost 
> everywhere_.

I think we rely on the fact that PERM_UMASK == 0 and not on the value
of shared_repository. Not the same thing.

That said, perhaps you are right: it is harmless.

>>> +		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
>> OPT__QUIET(&quiet),
>>
>> if (quiet)
>> 	flags |= INIT_DB_QUIET;
>>
>> to use the same quiet option everywhere?
> 
> Why?  Doesn't make it more readable, I think.  I'd rather have 3 lines 
> less.

Hum.

Olivier.

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Olivier Marin]

Michele Ballabio a écrit :
> 
>>> +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
>> Are you sure the default value is really used here?
> 
> Yes. Perhaps I don't understand your question. Can you explain what you mean?

If I read the code correctly in parse-options.c, with OPTION_CALLBACK, the
default value is not "automatically" used. You can use it in your callback
if you want, but because you don't, I think it's never used.

> Would you like this better, with PARSE_OPT_NONEG?

No, I'm fine with the negated option.

> Or do you prefer changing the callback like this:
> 
> +static int parse_opt_shared_cb(const struct option *opt, const char *arg,
> +                              int unset)
> +{
> +       *(int *)(opt->value) = unset ? PERM_UMASK : git_config_perm("arg", arg);
> +       return 0;
> +}

I think it's better but what I suggested is more something like:

static int parse_opt_shared_cb(const struct option *opt, const char *arg,
			       int unset)
{
	*(int *)(opt->value) = unset ? -1 : git_config_perm("arg", arg);
	return 0;
}

int shared = -1;

{ OPTION_CALLBACK, 0, "shared", &shared,
	"permissions", "setup as shared repository",
	PARSE_OPT_OPTARG, parse_perm_callback },

if (shared >= 0)
	shared_repository = shared;

This way we do not change shared_repository during parsing, so we do not
loose the initial value.

But it seems nobody care about this kind of details, so perhaps, you can
just ignore this suggestion.

Olivier.

[PATCH 7/9 - v2] builtin-checkout-index.c: use parse_options()

[Michele Ballabio]

This changes "struct checkout" (now it uses ints and not bitfields) to
simplify the parsing code.

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
On Friday 25 July 2008, René Scharfe wrote:
> In the case of struct checkout, though, we could simply make the
> bitfield members full ints, because there are only a few instances of
> this structure in memory at any given time. Wasting a few bytes of RAM
> in order to gain much simpler code is OK in this case, I think.
> OPT_BOOLEAN looks a lot nicer than a callback.

Yes. I only wanted the changes to be minimal, and only affect the option
parsing. In this sense, I still think the old patch is better. Here it is
the one you suggested (maybe Johannes suggested the same, but I didn't
understand :).

 builtin-checkout-index.c |  113 +++++++++++++++++++---------------------------
 cache.h                  |    8 ++--
 2 files changed, 50 insertions(+), 71 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 71ebabf..135348e 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,18 +154,43 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const checkout_cache_usage[] = {
+	"git checkout-index [options] [--] <file>...",
+	NULL
+};
 
 static struct lock_file lock_file;
 
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	int newfd = -1;
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	char *stage = NULL;
+
+	const struct option options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			    "checks out all files in the index"),
+		OPT_BOOLEAN('f', "force", &state.force,
+			    "force overwrite of existing files"),
+		OPT__QUIET(&state.quiet),
+		OPT_SET_INT('n', "no-create", &state.not_new,
+			"do not checkout new files, refresh existing ones", 1),
+		OPT_BOOLEAN('u', "index", &state.refresh_cache,
+			    "update stat information in the index"),
+		OPT_SET_INT('z', NULL, &line_termination,
+			    "separate paths with NUL", 0),
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			    "read paths from stdin"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			    "write content to temporary files"),
+		OPT_STRING(0, "prefix", &state.base_dir, "string",
+			   "prepend <string> when creating files"),
+		OPT_STRING(0, "stage", &stage, "1|2|3|all",
+			   "copy out files from the named stage"),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,71 +200,24 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
+	argc = parse_options(argc, argv, options, checkout_cache_usage, 0);
 
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
+	if ((state.refresh_cache) && (newfd < 0))
+		newfd = hold_locked_index(&lock_file, 1);
+	if (state.base_dir)
+		state.base_dir_len = strlen(state.base_dir);
+
+	if (stage) {
+		if (!strcmp(stage, "all")) {
 			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
+			checkout_stage = CHECKOUT_ALL;
+		} else {
+			int ch = stage[0];
+			if ('1' <= ch && ch <= '3')
+				checkout_stage = stage[0] - '0';
+			else
+				die("stage should be between 1 and 3 or all");
 		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
 	}
 
 	if (state.base_dir_len || to_tempfile) {
@@ -253,8 +232,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
-		const char *arg = argv[i];
+	while (argc-- > 0) {
+		const char *arg = *argv++;
 		const char *p;
 
 		if (all)
diff --git a/cache.h b/cache.h
index 38985aa..0bbe33b 100644
--- a/cache.h
+++ b/cache.h
@@ -618,10 +618,10 @@ extern const char *fmt_name(const char *name, const char *email);
 struct checkout {
 	const char *base_dir;
 	int base_dir_len;
-	unsigned force:1,
-		 quiet:1,
-		 not_new:1,
-		 refresh_cache:1;
+	int force;
+	int quiet;
+	int not_new;
+	int refresh_cache;
 };
 
 extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
-- 
1.5.6.3

[PATCH 6/9 - v3] builtin-init-db.c: use parse_options()

[Michele Ballabio]

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
On Friday 25 July 2008, Olivier Marin wrote:
> Michele Ballabio a écrit :
> > 
> >>> +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
> >> Are you sure the default value is really used here?
> > 
> > Yes. Perhaps I don't understand your question. Can you explain what you mean?
> 
> If I read the code correctly in parse-options.c, with OPTION_CALLBACK, the
> default value is not "automatically" used. You can use it in your callback
> if you want, but because you don't, I think it's never used.

Oh, you're right, but git_config_perm() handles NULL just fine, so I can
remove it. Done in this patch, thanks.

> what I suggested is more something like:
> 
> static int parse_opt_shared_cb(const struct option *opt, const char *arg,
> 			       int unset)
> {
> 	*(int *)(opt->value) = unset ? -1 : git_config_perm("arg", arg);
> 	return 0;
> }
> 
> int shared = -1;
> 
> { OPTION_CALLBACK, 0, "shared", &shared,
> 	"permissions", "setup as shared repository",
> 	PARSE_OPT_OPTARG, parse_perm_callback },
> 
> if (shared >= 0)
> 	shared_repository = shared;
> 
> This way we do not change shared_repository during parsing, so we do not
> loose the initial value.
> 
> But it seems nobody care about this kind of details, so perhaps, you can
> just ignore this suggestion.

I might be wrong, but shared_repository is initialized to PERM_UMASK and
does not change before parse_options() is called, so this is not much
useful.

 builtin-init-db.c |   57 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 38b4fcb..01b84a9 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -353,8 +354,18 @@ static int guess_repository_type(const char *git_dir)
 	return 1;
 }
 
-static const char init_db_usage[] =
-"git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]]";
+static const char * const init_db_usage[] = {
+	"git init [-q | --quiet] [--bare] [--template=<dir>] [--shared[=<type>]]",
+	NULL
+};
+
+static int parse_opt_shared_cb(const struct option *opt, const char *arg,
+			       int unset)
+{
+	*(int *)(opt->value) = unset ? PERM_UMASK :
+				       git_config_perm("arg", arg);
+	return 0;
+}
 
 /*
  * If you want to, you can share the DB area with any number of branches.
@@ -367,25 +378,29 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
-	int i;
-
-	for (i = 1; i < argc; i++, argv++) {
-		const char *arg = argv[1];
-		if (!prefixcmp(arg, "--template="))
-			template_dir = arg+11;
-		else if (!strcmp(arg, "--bare")) {
-			static char git_dir[PATH_MAX+1];
-			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
-						sizeof(git_dir)), 0);
-		} else if (!strcmp(arg, "--shared"))
-			shared_repository = PERM_GROUP;
-		else if (!prefixcmp(arg, "--shared="))
-			shared_repository = git_config_perm("arg", arg+9);
-		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
-			flags |= INIT_DB_QUIET;
-		else
-			usage(init_db_usage);
+	int bare = 0;
+
+	const struct option options[] = {
+		OPT_STRING(0, "template", &template_dir, "path",
+			   "path to the template directory"),
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repository"),
+		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
+		  "permissions", "set up a shared repository",
+		  PARSE_OPT_OPTARG, parse_opt_shared_cb },
+		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, init_db_usage, 0);
+
+	if (argc > 0)
+		usage_with_options(init_db_usage, options);
+
+	if (bare) {
+		static char git_dir[PATH_MAX+1];
+		is_bare_repository_cfg = 1;
+		setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
+					sizeof(git_dir)), 0);
 	}
 
 	/*
-- 
1.5.6.3

Re: [PATCH 1/9] builtin-verify-tag.c: use parse_options()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 321 bytes --]

Hi,

On Fri, 25 Jul 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> > 
> > That would be a bugfix.  As such, it belongs into a different commit.  
> 
> I thought, for that kind of trivial bug that probably never hit anyone, 
> a line in the commit message was enough.

So bisectability goes down the gutter?

Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 452 bytes --]

Hi,

On Fri, 25 Jul 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> 
> > We rely on shared_repository == 0 for non-shared repositories _almost 
> > everywhere_.
> 
> I think we rely on the fact that PERM_UMASK == 0 and not on the value of 
> shared_repository. Not the same thing.

Just look at all the cases where we ask for "if (shared_repository)".  
And then look where PERM_UMASK is assigned to.  It _is_ the same thing.

Hth,
Dscho

[PATCH] builtin-verify-tag: fix -v option parsing

[Olivier Marin]

From: Olivier Marin <dkr@freesurf.fr>

Since the C rewrite, "git verify-tag -v" just does nothing instead of
printing the usage message with an error. This patch fix the regression.

Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
 builtin-verify-tag.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index 7d837f0..729a159 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -92,14 +92,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	if (argc == 1)
-		usage(builtin_verify_tag_usage);
-
-	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
+	if (argc > 1 &&
+	    (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose"))) {
 		verbose = 1;
 		i++;
 	}
 
+	if (argc <= i)
+		usage(builtin_verify_tag_usage);
+
 	/* sometimes the program was terminated because this signal
 	 * was received in the process of writing the gpg input: */
 	signal(SIGPIPE, SIG_IGN);
-- 
1.6.0.rc0.79.gb0320

Re: [PATCH] builtin-verify-tag: fix -v option parsing

[Johannes Schindelin]

Hi,

On Mon, 28 Jul 2008, Olivier Marin wrote:

> From: Olivier Marin <dkr@freesurf.fr>
> 
> Since the C rewrite, "git verify-tag -v" just does nothing instead of 
> printing the usage message with an error. This patch fix the regression.

Maybe a better solution would be to convert (trivially) to 
parse-options...

Ciao,
Dscho

Re: [PATCH] builtin-verify-tag: fix -v option parsing

[Olivier Marin]

Johannes Schindelin a écrit :
>>
>> Since the C rewrite, "git verify-tag -v" just does nothing instead of 
>> printing the usage message with an error. This patch fix the regression.
> 
> Maybe a better solution would be to convert (trivially) to 
> parse-options...

I am very puzzled.

You first asked me to do a separate commit with just the fix and now
you seem to want the fix with the conversion...

What do you mean by "trivially"?

Olivier.

Re: [PATCH] builtin-verify-tag: fix -v option parsing

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 637 bytes --]

Hi,

On Mon, 28 Jul 2008, Olivier Marin wrote:

> Johannes Schindelin a écrit :
> >>
> >> Since the C rewrite, "git verify-tag -v" just does nothing instead of 
> >> printing the usage message with an error. This patch fix the 
> >> regression.
> > 
> > Maybe a better solution would be to convert (trivially) to 
> > parse-options...
> 
> I am very puzzled.
> 
> You first asked me to do a separate commit with just the fix and now you 
> seem to want the fix with the conversion...

Sorry.  It was not obvious to myself when I asked for a separate patch 
that the fix would fall out of the conversion to parse-options.

My fault,
Dscho