Skip to contentSkip to conversation

Extend use of parse_options()

PATCH In review Michele Ballabio 5 participants 31 messages Jun 15, 2016

lore
Share & tools
lore

Michele Ballabio

author
#0 lore raw

cover Extend use of parse_options()

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(-)
author
#1 lore raw

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

Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-verify-tag.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)
builtin-verify-tag.c +15 -10
index 7d837f0..590038b 100644
@@ -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;
 }
Signature · 1 line
1.5.6.3
#2 lore raw

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

builtin-verify-tag.c static int verify_tag(const char *name, int verbose) · line 104

Quoted 4 lines · Michele Ballabio
builtin-verify-tag.c +1 -1
 	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.

builtin-verify-tag.c static int verify_tag(const char *name, int verbose) · line 106

Quoted 5 lines
builtin-verify-tag.c +1 -4
-	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
-		verbose = 1;
-		i++;
-	}
+	argc = parse_options(argc, argv, options, builtin_verify_tag_usage, 0);

Olivier.

#3 lore raw

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

Hi,

Quoted 11 lines · Olivier Marin

builtin-verify-tag.c static int verify_tag(const char *name, int verbose) · line 104

Quoted 4 lines · Michele Ballabio
builtin-verify-tag.c +1 -1
 	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?

Quoted 5 lines

builtin-verify-tag.c static int verify_tag(const char *name, int verbose) · line 106

Quoted 5 lines
builtin-verify-tag.c +1 -4
-	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

#4 lore raw

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

Quoted 2 lines · Johannes Schindelin

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.

Quoted 1 line

Care to provide a patch?

OK, will do.

Olivier.

#5 lore raw

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

Hi,

Quoted 6 lines · Olivier Marin
Quoted 2 lines · Johannes Schindelin

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?

#6 lore raw

PATCH 1/1 builtin-verify-tag: fix -v option parsing

In reply to Olivier Marin

From: Olivier Marin <[email protected]>

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 <[email protected]>
---
 builtin-verify-tag.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
builtin-verify-tag.c +5 -4
index 7d837f0..729a159 100644
@@ -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);
Signature · 1 line
1.6.0.rc0.79.gb0320
#7 lore raw

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

Hi,

Quoted 4 lines · Olivier Marin

From: Olivier Marin <[email protected]>

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

#8 lore raw

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

Quoted 6 lines · Johannes Schindelin
Quoted 3 lines

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.

#9 lore raw

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

Hi,

Quoted 6 lines · Olivier Marin
Quoted 5 lines · Johannes Schindelin
Quoted 4 lines

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.

Quoted 7 lines
Quoted 2 lines

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

author
#10 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-write-tree.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)
builtin-write-tree.c +17 -14
index 52a3c01..25f3d8a 100644
@@ -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) {
Signature · 1 line
1.5.6.3
author
#11 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-prune-packed.c |   38 ++++++++++++++++++--------------------
 1 files changed, 18 insertions(+), 20 deletions(-)
builtin-prune-packed.c +18 -20
index 10cb8df..5866871 100644
@@ -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;
 }
Signature · 1 line
1.5.6.3
author
#12 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-ls-tree.c |   92 +++++++++++++++++++++--------------------------------
 1 files changed, 36 insertions(+), 56 deletions(-)
builtin-ls-tree.c +36 -56
index d25767a..a0b17aa 100644
@@ -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");
Signature · 1 line
1.5.6.3
author
#13 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-rev-list.c |  132 +++++++++++++++++++++++++--------------------------
 1 files changed, 65 insertions(+), 67 deletions(-)
builtin-rev-list.c +65 -67
index 893762c..9200b20 100644
@@ -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)
Signature · 1 line
1.5.6.3
author
#14 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-init-db.c |   56 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 35 insertions(+), 21 deletions(-)
builtin-init-db.c +35 -21
index 38b4fcb..ea1bed7 100644
@@ -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);
 	}
 
 	/*
Signature · 1 line
1.5.6.3
#15 lore raw

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

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 384

Quoted 4 lines · Michele Ballabio
builtin-init-db.c +3 -0
+	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.

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 385

Quoted 1 line
builtin-init-db.c +1 -0
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),

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

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 387

Quoted 2 lines
builtin-init-db.c +2 -0
+		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
+		  "type", "type of shared repository",

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

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 388

Quoted 1 line
builtin-init-db.c +1 -0
+		  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?

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 389

Quoted 1 line
builtin-init-db.c +1 -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?

Just my opinion,
Olivier.

#16 lore raw

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

Hi,

Quoted 5 lines · Olivier Marin

Michele Ballabio a ??rit :

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 385

Quoted 1 line
builtin-init-db.c +1 -0
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),

s/set up/setup/

No. "setup" is a noun.

Quoted 11 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 387

Quoted 2 lines
builtin-init-db.c +2 -0
+		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
+		  "type", "type of shared repository",

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

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 388

Quoted 1 line
builtin-init-db.c +1 -0
+		  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.

Quoted 4 lines
$ 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_.

Quoted 8 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 389

Quoted 1 line
builtin-init-db.c +1 -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?

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

Ciao,
Dscho

#17 lore raw

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

Quoted 5 lines · Johannes Schindelin
Quoted 3 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 385

Quoted 2 lines
builtin-init-db.c +1 -0
+		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),

s/set up/setup/

No. "setup" is a noun.

Right, sorry.

Quoted 2 lines

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.

Quoted 10 lines
Quoted 7 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 389

Quoted 1 line
builtin-init-db.c +1 -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?

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

Hum.

Olivier.

#18 lore raw

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

Hi,

Quoted 7 lines · Olivier Marin
Quoted 2 lines · Johannes Schindelin

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

author
#19 lore raw

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

In reply to Olivier Marin
Quoted 7 lines · Olivier Marin

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 384

Quoted 4 lines · Michele Ballabio
builtin-init-db.c +3 -0
+	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.

Quoted 3 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 385

Quoted 1 line
builtin-init-db.c +1 -0
+		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.

Quoted 4 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 387

Quoted 2 lines
builtin-init-db.c +2 -0
+		{ 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/.

Quoted 3 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 388

Quoted 1 line
builtin-init-db.c +1 -0
+		  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?

Quoted 7 lines

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;
+}

Quoted 8 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 389

Quoted 1 line
builtin-init-db.c +1 -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.

author
#20 lore raw

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

Signed-off-by Michele Ballabio <[email protected]>

---

Review

Quoted 7 lines · Michele Ballabio
(quoted diff) +6 -0
+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(-)
builtin-init-db.c +36 -21
index 38b4fcb..42c2e20 100644
@@ -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);
 	}
 
 	/*
Signature · 1 line
1.5.6.3
#21 lore raw

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

In reply to Michele Ballabio
Quoted 5 lines · Michele Ballabio
Quoted 3 lines

builtin-init-db.c int cmd_init_db(int argc, const char **argv, const char *prefix) · line 388

Quoted 2 lines
builtin-init-db.c +1 -0
+		  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.

Quoted 1 line

Would you like this better, with PARSE_OPT_NONEG?

No, I'm fine with the negated option.

builtin-init-db.c static int guess_repository_type(const char *git_dir) · line 367

Quoted 8 lines

Or do you prefer changing the callback like this:

builtin-init-db.c +6 -0
+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.

author
#22 lore raw

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

Signed-off-by Michele Ballabio <[email protected]>

---

Quoted 10 lines · Olivier Marin

Michele Ballabio a écrit :

Quoted 5 lines
Quoted 3 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+		  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.

Quoted 15 lines

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 },

Quoted 8 lines

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(-)
builtin-init-db.c +36 -21
index 38b4fcb..01b84a9 100644
@@ -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);
 	}
 
 	/*
Signature · 1 line
1.5.6.3
author
#23 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-checkout-index.c |  146 +++++++++++++++++++++++++---------------------
 1 files changed, 79 insertions(+), 67 deletions(-)
builtin-checkout-index.c +79 -67
index 71ebabf..429c850 100644
@@ -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)
Signature · 1 line
1.5.6.3
#24 lore raw

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

Hi,

builtin-checkout-index.c static void checkout_all(const char *prefix, int prefix_length) · line 205

Quoted 3 lines · Michele Ballabio
builtin-checkout-index.c +3 -0
+		{ 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

author
#25 lore raw

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

Quoted 10 lines · Johannes Schindelin

builtin-checkout-index.c static void checkout_all(const char *prefix, int prefix_length) · line 205

Quoted 3 lines · Michele Ballabio
builtin-checkout-index.c +3 -0
+		{ 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.

#26 lore raw

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

Quoted 7 lines · Michele Ballabio
Quoted 6 lines · Johannes Schindelin
Quoted 3 lines · Michele Ballabio
builtin-checkout-index.c +3 -0
+           { OPTION_CALLBACK, 'f', "force", &state, NULL,
+             "force overwrite of existing files",
+             PARSE_OPT_NOARG, parse_state_force_cb, 0 },
Quoted 6 lines
Quoted 4 lines

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?

Signature · 3 lines
Cheers,

Sverre Rabbelier
#27 lore raw

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

Quoted 8 lines · Sverre Rabbelier
Quoted 6 lines · Michele Ballabio
Quoted 5 lines · Johannes Schindelin
Quoted 3 lines · Michele Ballabio
builtin-checkout-index.c +3 -0
+           { OPTION_CALLBACK, 'f', "force", &state, NULL,
+             "force overwrite of existing files",
+             PARSE_OPT_NOARG, parse_state_force_cb, 0 },
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines

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é

author
#28 lore raw

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

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

Signed-off-by Michele Ballabio <[email protected]>

---

Quoted 5 lines · René Scharfe

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(-)
builtin-checkout-index.c +46 -67
index 71ebabf..135348e 100644
@@ -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)
cache.h +4 -4
index 38985aa..0bbe33b 100644
@@ -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);
Signature · 1 line
1.5.6.3
author
#29 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-fetch-pack.c |  144 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 90 insertions(+), 54 deletions(-)
builtin-fetch-pack.c +90 -54
index 273239a..701be41 100644
@@ -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);
Signature · 1 line
1.5.6.3
author
#30 lore raw

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

In reply to Michele Ballabio
Signed-off-by Michele Ballabio <[email protected]>
---
 builtin-mailinfo.c |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)
builtin-mailinfo.c +21 -18
index f974b9d..f1ed269 100644
@@ -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]);
 }
Signature · 1 line
1.5.6.3