threads / patch / 19388

patch, 3 partsshow-branch: Fix die message in parse_reflog_param()

Subject: [PATCH 1/3] show-branch: Fix die message in parse_reflog_param()

## tl;dr

9 messages between May 17, 2009 and May 19, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Stephen Boyd· May 17, 2009, 10:47 UTC · lore

Commit 76a44c5 (show-branch --reflog: show the reflog message at the top, 2007-01-19) introduced parse_reflog_param(). The die() call was incorrectly passed arg + 9, when it should have been passed arg.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-show-branch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin-show-branch.c +1 −1
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 828e6f8..c3afabb 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -576,7 +576,7 @@ static void parse_reflog_param(const char *arg, int *cnt, const char **base)
 	if (*ep == ',')
 		*base = ep + 1;
 	else if (*ep)
-		die("unrecognized reflog param '%s'", arg + 9);
+		die("unrecognized reflog param '%s'", arg);
 	else
 		*base = NULL;
 	if (*cnt <= 0)
-- 
1.6.3.1.30.g55524
Stephen Boyd· May 17, 2009, 10:47 UTC · re: Stephen Boyd · lore

[PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

If argh is complicated, i.e. the option takes more than one argument, don't add the brackets around argh in the usage message.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
    This is used in the next patch to for --reflog[=<n>[,<base>]]
    Maybe someone can think of a better name?
 parse-options.c |   26 +++++++++++++++++---------
 parse-options.h |    4 ++++
 2 files changed, 21 insertions(+), 9 deletions(-)
Show changes to 2 files +21 −9

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index cf71bcf..cd9ab4f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -361,6 +361,20 @@ int parse_options(int argc, const char **argv, const struct option *options,
 	return parse_options_end(&ctx);
 }
 
+static int usage_argh(const struct option *opts, int pos)
+{
+	const char *s;
+	int custom = opts->flags & PARSE_OPT_CUSTOM_ARGH;
+	if (opts->flags & PARSE_OPT_OPTARG)
+		if (opts->long_name)
+			s = custom ? "[=%s]" : "[=<%s>]";
+		else
+			s = custom ? "[%s]" : "[<%s>]";
+	else
+		s = custom ? " %s" : " <%s>";
+	return pos + fprintf(stderr, s, opts->argh);
+}
+
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
@@ -421,15 +435,9 @@ int usage_with_options_internal(const char * const *usagestr,
 				break;
 			/* FALLTHROUGH */
 		case OPTION_STRING:
-			if (opts->argh) {
-				if (opts->flags & PARSE_OPT_OPTARG)
-					if (opts->long_name)
-						pos += fprintf(stderr, "[=<%s>]", opts->argh);
-					else
-						pos += fprintf(stderr, "[<%s>]", opts->argh);
-				else
-					pos += fprintf(stderr, " <%s>", opts->argh);
-			} else {
+			if (opts->argh)
+				pos += usage_argh(opts, pos);
+			else {
 				if (opts->flags & PARSE_OPT_OPTARG)
 					if (opts->long_name)
 						pos += fprintf(stderr, "[=...]");
diff --git a/parse-options.h b/parse-options.h
index b54eec1..671a635 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -31,6 +31,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_NONEG   = 4,
 	PARSE_OPT_HIDDEN  = 8,
 	PARSE_OPT_LASTARG_DEFAULT = 16,
+	PARSE_OPT_CUSTOM_ARGH = 64,
 };
 
 struct option;
@@ -66,6 +67,9 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *   PARSE_OPT_NONEG: says that this option cannot be negated
  *   PARSE_OPT_HIDDEN this option is skipped in the default usage, showed in
  *                    the long one.
+ *   PARSE_OPT_CUSTOM_ARGH: says that argh shouldn't be enclosed in brackets
+ *			    (i.e. '<argh>') in the help message.
+ *			    Useful for options with multiple parameters.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK.
-- 
1.6.3.1.30.g55524
Stephen Boyd· May 17, 2009, 10:47 UTC · re: Stephen Boyd · lore

[PATCH 3/3] show-branch: migrate to parse-options API

Also die instead of calling usage with an error string.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin-show-branch.c |  117 +++++++++++++++++++++++++------------------------
 1 files changed, 60 insertions(+), 57 deletions(-)
Show changes to builtin-show-branch.c +60 −57
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index c3afabb..e9acd4c 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -2,11 +2,13 @@
 #include "commit.h"
 #include "refs.h"
 #include "builtin.h"
+#include "parse-options.h"
 
-static const char show_branch_usage[] =
-"git show-branch [--sparse] [--current] [--all] [--remotes] [--topo-order] [--more=count | --list | --independent | --merge-base ] [--topics] [<refs>...] | --reflog[=n[,b]] <branch>";
-static const char show_branch_usage_reflog[] =
-"--reflog is incompatible with --all, --remotes, --independent or --merge-base";
+static const char* show_branch_usage[] = {
+    "git show-branch [--sparse] [--current] [--all] [--remotes] [--topo-order] [--more=count | --list | --independent | --merge-base] [--topics] [<refs>...]",
+    "--reflog[=n[,b]] [--list] <branch>",
+    NULL
+};
 
 static int default_num;
 static int default_alloc;
@@ -569,18 +571,25 @@ static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
 	return 0;
 }
 
-static void parse_reflog_param(const char *arg, int *cnt, const char **base)
+static int reflog = 0;
+
+static int parse_reflog_param(const struct option *opt, const char *arg,
+			      int unset)
 {
 	char *ep;
-	*cnt = strtoul(arg, &ep, 10);
+	const char **base = (const char **)opt->value;
+	if (!arg)
+		arg = "";
+	reflog = strtoul(arg, &ep, 10);
 	if (*ep == ',')
 		*base = ep + 1;
 	else if (*ep)
 		die("unrecognized reflog param '%s'", arg);
 	else
 		*base = NULL;
-	if (*cnt <= 0)
-		*cnt = DEFAULT_REFLOG;
+	if (reflog <= 0)
+		reflog = DEFAULT_REFLOG;
+	return 0;
 }
 
 int cmd_show_branch(int ac, const char **av, const char *prefix)
@@ -606,8 +615,42 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	int head_at = -1;
 	int topics = 0;
 	int dense = 1;
-	int reflog = 0;
 	const char *reflog_base = NULL;
+	struct option builtin_show_branch_options[] = {
+		OPT_BOOLEAN('a', "all", &all_heads,
+			    "show remote-tracking and local branches"),
+		OPT_BOOLEAN('r', "remotes", &all_remotes,
+			    "show remote-tracking branches"),
+		{ OPTION_INTEGER, 0, "more", &extra, "n",
+			    "show <n> more commits after the common ancestor",
+			    PARSE_OPT_OPTARG | PARSE_OPT_LASTARG_DEFAULT,
+			    NULL, (intptr_t)1 },
+		OPT_SET_INT(0, "list", &extra, "synonym to more=-1", -1),
+		OPT_BOOLEAN(0, "no-name", &no_name, "suppress naming strings"),
+		OPT_BOOLEAN(0, "current", &with_current_branch,
+			    "include the current branch"),
+		OPT_BOOLEAN(0, "sha1-name", &sha1_name,
+			    "name commits with their object names"),
+		OPT_BOOLEAN(0, "merge-base", &merge_base,
+			    "act like git merge-base -a"),
+		OPT_BOOLEAN(0, "independent", &independent,
+			    "show refs unreachable from any other ref"),
+		OPT_BOOLEAN(0, "topo-order", &lifo,
+			    "show commits in topological order"),
+		OPT_BOOLEAN(0, "topics", &topics,
+			    "show only commits not on the first branch"),
+		OPT_SET_INT(0, "sparse", &dense,
+			    "show merges reachable from only one tip", 0),
+		OPT_SET_INT(0, "date-order", &lifo,
+			    "show commits where no parent comes before its "
+			    "children", 0),
+		{ OPTION_CALLBACK, 'g', "reflog", &reflog_base, "<n>[,<base>]",
+			    "show <n> most recent ref-log entries starting at "
+			    "base",
+			    PARSE_OPT_OPTARG | PARSE_OPT_CUSTOM_ARGH,
+			    parse_reflog_param },
+		OPT_END()
+	};
 
 	git_config(git_show_branch_config, NULL);
 
@@ -617,59 +660,18 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		av = default_arg - 1; /* ick; we would not address av[0] */
 	}
 
-	while (1 < ac && av[1][0] == '-') {
-		const char *arg = av[1];
-		if (!strcmp(arg, "--")) {
-			ac--; av++;
-			break;
-		}
-		else if (!strcmp(arg, "--all") || !strcmp(arg, "-a"))
-			all_heads = all_remotes = 1;
-		else if (!strcmp(arg, "--remotes") || !strcmp(arg, "-r"))
-			all_remotes = 1;
-		else if (!strcmp(arg, "--more"))
-			extra = 1;
-		else if (!strcmp(arg, "--list"))
-			extra = -1;
-		else if (!strcmp(arg, "--no-name"))
-			no_name = 1;
-		else if (!strcmp(arg, "--current"))
-			with_current_branch = 1;
-		else if (!strcmp(arg, "--sha1-name"))
-			sha1_name = 1;
-		else if (!prefixcmp(arg, "--more="))
-			extra = atoi(arg + 7);
-		else if (!strcmp(arg, "--merge-base"))
-			merge_base = 1;
-		else if (!strcmp(arg, "--independent"))
-			independent = 1;
-		else if (!strcmp(arg, "--topo-order"))
-			lifo = 1;
-		else if (!strcmp(arg, "--topics"))
-			topics = 1;
-		else if (!strcmp(arg, "--sparse"))
-			dense = 0;
-		else if (!strcmp(arg, "--date-order"))
-			lifo = 0;
-		else if (!strcmp(arg, "--reflog") || !strcmp(arg, "-g")) {
-			reflog = DEFAULT_REFLOG;
-		}
-		else if (!prefixcmp(arg, "--reflog="))
-			parse_reflog_param(arg + 9, &reflog, &reflog_base);
-		else if (!prefixcmp(arg, "-g="))
-			parse_reflog_param(arg + 3, &reflog, &reflog_base);
-		else
-			usage(show_branch_usage);
-		ac--; av++;
-	}
-	ac--; av++;
+	ac = parse_options(ac, av, builtin_show_branch_options,
+			   show_branch_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+	if (all_heads)
+		all_remotes = 1;
 
 	if (extra || reflog) {
 		/* "listing" mode is incompatible with
 		 * independent nor merge-base modes.
 		 */
 		if (independent || merge_base)
-			usage(show_branch_usage);
+			usage_with_options(show_branch_usage,
+					   builtin_show_branch_options);
 		if (reflog && ((0 < extra) || all_heads || all_remotes))
 			/*
 			 * Asking for --more in reflog mode does not
@@ -677,7 +679,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			 *
 			 * Also --all and --remotes do not make sense either.
 			 */
-			usage(show_branch_usage_reflog);
+			die("--reflog is incompatible with --all, --remotes, "
+			    "--independent or --merge-base");
 	}
 
 	/* If nothing is specified, show all branches by default */
-- 
1.6.3.1.30.g55524
Junio C Hamano· May 18, 2009, 06:14 UTC · re: Stephen Boyd · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Stephen Boyd <bebarino@gmail.com> writes:
> If argh is complicated, i.e. the option takes more than one argument,
> don't add the brackets around argh in the usage message.
I think later user wants a bit more descriptive explanation, like...
	Usually, argh element in struct option points at a placeholder
	value (e.g. "val"), and this is used to show
	    --option=<val>
	by enclosing the string inside of angle brackets.
	When the option takes something more complex (e.g. optional part
	separated by comma), you would want to produce a help that looks
	like
	    --option=<val1>[,<val2>]
	In such a case, the caller can pass a string to argh with
	placeholders already enclosed in necessary angle brackets
	(e.g. "<val1>[,<val2>]") and set this option.
Please update Documentation/technical/api-parse-options.txt as well.
Stephen Boyd· May 18, 2009, 07:08 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Junio C Hamano wrote:
Show 22 quoted lines
> Stephen Boyd <bebarino@gmail.com> writes:
>> If argh is complicated, i.e. the option takes more than one argument,
>> don't add the brackets around argh in the usage message.
>
> I think later user wants a bit more descriptive explanation, like...
>
> 	Usually, argh element in struct option points at a placeholder
> 	value (e.g. "val"), and this is used to show
>
> 	    --option=<val>
>
> 	by enclosing the string inside of angle brackets.
>
> 	When the option takes something more complex (e.g. optional part
> 	separated by comma), you would want to produce a help that looks
> 	like
>
> 	    --option=<val1>[,<val2>]
>
> 	In such a case, the caller can pass a string to argh with
> 	placeholders already enclosed in necessary angle brackets
> 	(e.g. "<val1>[,<val2>]") and set this option.

This description sounds nice ;) Is PARSE_OPT_CUSTOM_ARGH a good name? I was thinking maybe PARSE_OPT_MULTARGS is better?

> Please update Documentation/technical/api-parse-options.txt as well.
There's no documentation on the flags yet, but I suppose I could add that.
Junio C Hamano· May 18, 2009, 07:57 UTC · re: Stephen Boyd · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Stephen Boyd <bebarino@gmail.com> writes:
> This description sounds nice ;) Is PARSE_OPT_CUSTOM_ARGH a good name? I
> was thinking maybe PARSE_OPT_MULTARGS is better?

You are asking a wrong person. I am terrible at naming things; think "rerere" ;-).

But I think custom-arghelp is much better than multargs. You are asking "use this help for argument value _literally_", so another possibility perhaps is PARSE_OPT_ARGHELP_LITERAL.

After all, there probably are many other valid reasons why you may not want "s/.*/<&>/" blindly applied to your string. One reason may be because the string describes multiple arguments, but I suspect that it is not the only one. It is better to name the option after what it does, than naming it after one sample reason why you might want to use that option.

>> Please update Documentation/technical/api-parse-options.txt as well.
>
> There's no documentation on the flags yet, but I suppose I could add that.
Thanks.
Stephen Boyd· May 18, 2009, 08:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Junio C Hamano wrote:
Show 13 quoted lines
> You are asking a wrong person.  I am terrible at naming things; think
> "rerere" ;-).
>
> But I think custom-arghelp is much better than multargs.  You are asking
> "use this help for argument value _literally_", so another possibility
> perhaps is PARSE_OPT_ARGHELP_LITERAL.
>
> After all, there probably are many other valid reasons why you may not
> want "s/.*/<&>/" blindly applied to your string.  One reason may be
> because the string describes multiple arguments, but I suspect that it is
> not the only one.  It is better to name the option after what it does,
> than naming it after one sample reason why you might want to use that
> option.

Ok. I think PARSE_OPT_LITERAL_ARGH might be good; until someone comes up with a better one of course.

Jakub Narebski· May 18, 2009, 22:52 UTC · re: Stephen Boyd · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Stephen Boyd <bebarino@gmail.com> writes:
> Junio C Hamano wrote:
Show 16 quoted lines
> > You are asking a wrong person.  I am terrible at naming things; think
> > "rerere" ;-).
> >
> > But I think custom-arghelp is much better than multargs.  You are asking
> > "use this help for argument value _literally_", so another possibility
> > perhaps is PARSE_OPT_ARGHELP_LITERAL.
> >
> > After all, there probably are many other valid reasons why you may not
> > want "s/.*/<&>/" blindly applied to your string.  One reason may be
> > because the string describes multiple arguments, but I suspect that it is
> > not the only one.  It is better to name the option after what it does,
> > than naming it after one sample reason why you might want to use that
> > option.
> 
> Ok. I think PARSE_OPT_LITERAL_ARGH might be good; until someone comes up
> with a better one of course.
Well, 'ARGH' sounds a bit strange. I think I'd prefer ARGHELP. :-)
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Sverre Rabbelier· May 19, 2009, 05:09 UTC · re: Jakub Narebski · lore

Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's

Heya,
On Tue, May 19, 2009 at 00:52, Jakub Narebski <jnareb@gmail.com> wrote:
> Well, 'ARGH' sounds a bit strange. I think I'd prefer ARGHELP. :-)
Yarr, it be reminding me of a patch from ye ol' mentor summit!
-- 
Cheers,

Sverre Rabbelier

← back to recent threads