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

9 messages from 2009-05-17 to 2009-05-19. Participants: Stephen Boyd, Junio C Hamano, Jakub Narebski, Sverre Rabbelier.
Thread: https://gitlist.dev/t/19388

## Stephen Boyd, 2009-05-17 10:47

Subject: [PATCH 1/3] show-branch: Fix die message in parse_reflog_param()
Message-ID: <1242557224-8411-1-git-send-email-bebarino@gmail.com>
URL: https://gitlist.dev/e/1242557224-8411-1-git-send-email-bebarino%40gmail.com

```
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(-)

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, 2009-05-17 10:47

Subject: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <1242557224-8411-2-git-send-email-bebarino@gmail.com>
URL: https://gitlist.dev/e/1242557224-8411-2-git-send-email-bebarino%40gmail.com
In-Reply-To: <1242557224-8411-1-git-send-email-bebarino@gmail.com>

```
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(-)

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, 2009-05-17 10:47

Subject: [PATCH 3/3] show-branch: migrate to parse-options API
Message-ID: <1242557224-8411-3-git-send-email-bebarino@gmail.com>
URL: https://gitlist.dev/e/1242557224-8411-3-git-send-email-bebarino%40gmail.com
In-Reply-To: <1242557224-8411-2-git-send-email-bebarino@gmail.com>

```
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(-)

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, 2009-05-18 06:14

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <7vd4a7ey4h.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vd4a7ey4h.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1242557224-8411-2-git-send-email-bebarino@gmail.com>

```
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, 2009-05-18 07:08

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <4A11096B.8020208@gmail.com>
URL: https://gitlist.dev/e/4A11096B.8020208%40gmail.com
In-Reply-To: <7vd4a7ey4h.fsf@alter.siamese.dyndns.org>

```
Junio C Hamano wrote:
> 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, 2009-05-18 07:57

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <7vmy9aetc8.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vmy9aetc8.fsf%40alter.siamese.dyndns.org
In-Reply-To: <4A11096B.8020208@gmail.com>

```
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, 2009-05-18 08:06

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <4A1116FF.8030008@gmail.com>
URL: https://gitlist.dev/e/4A1116FF.8030008%40gmail.com
In-Reply-To: <7vmy9aetc8.fsf@alter.siamese.dyndns.org>

```
Junio C Hamano wrote:
> 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, 2009-05-18 22:52

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <m3vdnyja8i.fsf@localhost.localdomain>
URL: https://gitlist.dev/e/m3vdnyja8i.fsf%40localhost.localdomain
In-Reply-To: <4A1116FF.8030008@gmail.com>

```
Stephen Boyd <bebarino@gmail.com> writes:
> Junio C Hamano wrote:

> > 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, 2009-05-19 05:09

Subject: Re: [PATCH 2/3] parse-options: add PARSE_OPT_CUSTOM_ARGH for complicated argh's
Message-ID: <fabb9a1e0905182209r2c6ae99fh491a35337abbe548@mail.gmail.com>
URL: https://gitlist.dev/e/fabb9a1e0905182209r2c6ae99fh491a35337abbe548%40mail.gmail.com
In-Reply-To: <m3vdnyja8i.fsf@localhost.localdomain>

```
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

```