[RFC] convert shortlog to use parse_options

[RFC] convert shortlog to use parse_options

[Shawn Bohrer]

I guess I should have searched the list _before_ creating these patches
since I just now stumbled upon some of the questions about how this
should be done for example:

http://kerneltrap.org/mailarchive/git/2008/3/1/1035344

>From my testing this seems to work fine, but I may have missed a use
case.  I actually created these patches because I was annoyed that:

git shortlog --author=bohrer -s HEAD

didn't work, and this also fixes that issue.

--
Shawn

[PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Shawn Bohrer]

This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents
parse_options() from erroring out when it finds an unknown option,
and leaves the original command and unknown options in argv.

This option is useful if the option parsing needs to be done in
multiple stages for example if the remaining options will be passed
to additional git commands.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 parse-options.c |   25 ++++++++++++++++++++-----
 parse-options.h |    5 +++--
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 8071711..2635e18 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -131,7 +131,8 @@ static int get_value(struct optparse_t *p,
 	}
 }
 
-static int parse_short_opt(struct optparse_t *p, const struct option *options)
+static int parse_short_opt(struct optparse_t *p, const struct option *options,
+			   int flags)
 {
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
@@ -139,11 +140,16 @@ static int parse_short_opt(struct optparse_t *p, const struct option *options)
 			return get_value(p, options, OPT_SHORT);
 		}
 	}
+
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {
+		p->out[p->cpidx++] = p->argv[0];
+		return 0;
+	}
 	return error("unknown switch `%c'", *p->opt);
 }
 
 static int parse_long_opt(struct optparse_t *p, const char *arg,
-                          const struct option *options)
+                          const struct option *options, int flags)
 {
 	const char *arg_end = strchr(arg, '=');
 	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
@@ -224,6 +230,11 @@ is_abbreviated:
 			abbrev_option->long_name);
 	if (abbrev_option)
 		return get_value(p, abbrev_option, abbrev_flags);
+
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN) {
+		p->out[p->cpidx++] = p->argv[0];
+		return 0;
+	}
 	return error("unknown option `%s'", arg);
 }
 
@@ -254,6 +265,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
                   const char * const usagestr[], int flags)
 {
 	struct optparse_t args = { argv + 1, argv, argc - 1, 0, NULL };
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN)
+		args.out =  argv + 1;
 
 	for (; args.argc; args.argc--, args.argv++) {
 		const char *arg = args.argv[0];
@@ -269,14 +282,14 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			args.opt = arg + 1;
 			if (*args.opt == 'h')
 				usage_with_options(usagestr, options);
-			if (parse_short_opt(&args, options) < 0)
+			if (parse_short_opt(&args, options, flags) < 0)
 				usage_with_options(usagestr, options);
 			if (args.opt)
 				check_typos(arg + 1, options);
 			while (args.opt) {
 				if (*args.opt == 'h')
 					usage_with_options(usagestr, options);
-				if (parse_short_opt(&args, options) < 0)
+				if (parse_short_opt(&args, options, flags) < 0)
 					usage_with_options(usagestr, options);
 			}
 			continue;
@@ -294,11 +307,13 @@ int parse_options(int argc, const char **argv, const struct option *options,
 			usage_with_options_internal(usagestr, options, 1);
 		if (!strcmp(arg + 2, "help"))
 			usage_with_options(usagestr, options);
-		if (parse_long_opt(&args, arg + 2, options))
+		if (parse_long_opt(&args, arg + 2, options, flags))
 			usage_with_options(usagestr, options);
 	}
 
 	memmove(args.out + args.cpidx, args.argv, args.argc * sizeof(*args.out));
+	if (flags & PARSE_OPT_NO_ERROR_ON_UNKNOWN)
+		++args.cpidx;
 	args.out[args.cpidx + args.argc] = NULL;
 	return args.cpidx + args.argc;
 }
diff --git a/parse-options.h b/parse-options.h
index 4ee443d..416ccdd 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -18,8 +18,9 @@ enum parse_opt_type {
 };
 
 enum parse_opt_flags {
-	PARSE_OPT_KEEP_DASHDASH = 1,
-	PARSE_OPT_STOP_AT_NON_OPTION = 2,
+	PARSE_OPT_KEEP_DASHDASH       = 1,
+	PARSE_OPT_STOP_AT_NON_OPTION  = 2,
+	PARSE_OPT_NO_ERROR_ON_UNKNOWN = 4
 };
 
 enum parse_opt_option_flags {
-- 
1.5.4.3

[PATCH 2/2] git shortlog: Modify to use parse_options

[Shawn Bohrer]

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 builtin-shortlog.c |   54 +++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index e6a2865..b1087b5 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -7,9 +7,12 @@
 #include "utf8.h"
 #include "mailmap.h"
 #include "shortlog.h"
+#include "parse-options.h"
 
-static const char shortlog_usage[] =
-"git-shortlog [-n] [-s] [-e] [-w] [<commit-id>... ]";
+static const char *const shortlog_usage[] = {
+	"git-shortlog [-n] [-s] [-e] [-w] [<commit-id>... ]",
+	NULL
+};
 
 static int compare_by_number(const void *a1, const void *a2)
 {
@@ -189,8 +192,6 @@ static const char wrap_arg_usage[] = "-w[<width>[,<indent1>[,<indent2>]]]";
 
 static void parse_wrap_args(const char *arg, int *in1, int *in2, int *wrap)
 {
-	arg += 2; /* skip -w */
-
 	*wrap = parse_uint(&arg, ',');
 	if (*wrap < 0)
 		die(wrap_arg_usage);
@@ -230,35 +231,38 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	struct shortlog log;
 	struct rev_info rev;
 	int nongit;
+	const char * wrap_options = NULL;
+	struct option options[] = {
+		OPT_BOOLEAN('n', "numbered", &log.sort_by_number,
+			    "sort by number"),
+		OPT_BOOLEAN('s', "summary", &log.summary,
+			    "only provide commit count summary"),
+		OPT_BOOLEAN('e', "email", &log.email,
+			    "show email address of author"),
+		{ OPTION_STRING, 'w', NULL, &wrap_options,
+		  "[<width>[,<indent1>[,<indent2>]]]", "linewrap the output",
+		  PARSE_OPT_OPTARG, NULL, (intptr_t)"-w" },
+		OPT_END()
+	};
 
 	prefix = setup_git_directory_gently(&nongit);
 	shortlog_init(&log);
 
-	/* since -n is a shadowed rev argument, parse our args first */
-	while (argc > 1) {
-		if (!strcmp(argv[1], "-n") || !strcmp(argv[1], "--numbered"))
-			log.sort_by_number = 1;
-		else if (!strcmp(argv[1], "-s") ||
-				!strcmp(argv[1], "--summary"))
-			log.summary = 1;
-		else if (!strcmp(argv[1], "-e") ||
-			 !strcmp(argv[1], "--email"))
-			log.email = 1;
-		else if (!prefixcmp(argv[1], "-w")) {
-			log.wrap_lines = 1;
-			parse_wrap_args(argv[1], &log.in1, &log.in2, &log.wrap);
-		}
-		else if (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help"))
-			usage(shortlog_usage);
-		else
-			break;
-		argv++;
-		argc--;
+	argc = parse_options(argc, argv, options, shortlog_usage,
+			     PARSE_OPT_NO_ERROR_ON_UNKNOWN);
+
+	if (wrap_options)
+	{
+		if (!prefixcmp(wrap_options, "-w"))
+			wrap_options += 2; /* skip -w */
+		log.wrap_lines = 1;
+		parse_wrap_args(wrap_options, &log.in1, &log.in2, &log.wrap);
 	}
+
 	init_revisions(&rev, prefix);
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	if (argc > 1)
-		die ("unrecognized argument: %s", argv[1]);
+		usage_with_options(shortlog_usage, options);
 
 	/* assume HEAD if from a tty */
 	if (!nongit && !rev.pending.nr && isatty(0))
-- 
1.5.4.3

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents
> parse_options() from erroring out when it finds an unknown option,
> and leaves the original command and unknown options in argv.

I have to say that this conceptually is broken.  How would you tell
without knowing what "--flag" is if the thing in argv[] after that is a
parameter to that option or the end of the options?

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Jeff King]

On Tue, Jun 17, 2008 at 08:21:50PM -0700, Junio C Hamano wrote:

> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> 
> > This adds the PARSE_OPT_NO_ERROR_ON_UNKNOWN flag which prevents
> > parse_options() from erroring out when it finds an unknown option,
> > and leaves the original command and unknown options in argv.
> 
> I have to say that this conceptually is broken.  How would you tell
> without knowing what "--flag" is if the thing in argv[] after that is a
> parameter to that option or the end of the options?

Agreed. I was just about to write the same thing. As it happens, I think
in the case of git-shortlog that there is not likely to be such a
parameter. The only three I see looking over setup_revisions are "-n"
(which is masked by shortlog anyway), "--default", and "-U" (which one
would never need with shortlog).

However I am still opposed to the concept, since its presence as a
parseopt flag implies that it isn't fundamentally broken.

I think the only right way to accomplish this is to convert the revision
and diff parameters into a parseopt-understandable format.

-Peff

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Jeff King]

On Tue, Jun 17, 2008 at 11:30:10PM -0400, Jeff King wrote:

> Agreed. I was just about to write the same thing. As it happens, I think
> in the case of git-shortlog that there is not likely to be such a
> parameter. The only three I see looking over setup_revisions are "-n"
> (which is masked by shortlog anyway), "--default", and "-U" (which one
> would never need with shortlog).

BTW, looking in my personal repo, I have the start of the exact same
patch (except I called it PARSE_OPT_STOP_AT_UNKNOWN). I think I
abandoned it when I realized the fundamental flaw with the approach, but
I guess I never got it to the point of sharing with the list.

-Peff

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think the only right way to accomplish this is to convert the revision
> and diff parameters into a parseopt-understandable format.

Not necessarily.  You could structure individual option parsers like how
diff option parsers are done.  You iterate over argv[], feed diff option
parser the current index into argv[] and ask if it is an option diff
understands, have diff eat the option (and possibly its parameter) to
advance the index, or allow diff option to say "I do not understand this",
and then handle it yourself or hand it to other parsers.

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Johannes Schindelin]

Hi,

On Tue, 17 Jun 2008, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think the only right way to accomplish this is to convert the revision
> > and diff parameters into a parseopt-understandable format.
> 
> Not necessarily.  You could structure individual option parsers like how 
> diff option parsers are done.  You iterate over argv[], feed diff option 
> parser the current index into argv[] and ask if it is an option diff 
> understands, have diff eat the option (and possibly its parameter) to 
> advance the index, or allow diff option to say "I do not understand 
> this", and then handle it yourself or hand it to other parsers.

AFAIR Pierre tried a few ways, and settled with a macro to introduce the 
diff options into a caller's options.

IOW it would look something like this:

static struct option builtin_what_options[] = {
	[... options specific to this command ...]
	DIFF__OPT(&diff_options)
};

Ciao,
Dscho

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 17 Jun 2008, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > I think the only right way to accomplish this is to convert the revision
>> > and diff parameters into a parseopt-understandable format.
>> 
>> Not necessarily.  You could structure individual option parsers like how 
>> diff option parsers are done.  You iterate over argv[], feed diff option 
>> parser the current index into argv[] and ask if it is an option diff 
>> understands, have diff eat the option (and possibly its parameter) to 
>> advance the index, or allow diff option to say "I do not understand 
>> this", and then handle it yourself or hand it to other parsers.
>
> AFAIR Pierre tried a few ways, and settled with a macro to introduce the 
> diff options into a caller's options.
>
> IOW it would look something like this:
>
> static struct option builtin_what_options[] = {
> 	[... options specific to this command ...]
> 	DIFF__OPT(&diff_options)
> };

I think that is the more painful approach Jeff mentioned, and my comment
was to show that it is not the only way.

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Shawn Bohrer]

On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 17 Jun 2008, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > I think the only right way to accomplish this is to convert the revision
> >> > and diff parameters into a parseopt-understandable format.
> >> 
> >> Not necessarily.  You could structure individual option parsers like how 
> >> diff option parsers are done.  You iterate over argv[], feed diff option 
> >> parser the current index into argv[] and ask if it is an option diff 
> >> understands, have diff eat the option (and possibly its parameter) to 
> >> advance the index, or allow diff option to say "I do not understand 
> >> this", and then handle it yourself or hand it to other parsers.
> >
> > AFAIR Pierre tried a few ways, and settled with a macro to introduce the 
> > diff options into a caller's options.
> >
> > IOW it would look something like this:
> >
> > static struct option builtin_what_options[] = {
> > 	[... options specific to this command ...]
> > 	DIFF__OPT(&diff_options)
> > };
> 
> I think that is the more painful approach Jeff mentioned, and my comment
> was to show that it is not the only way.
> 

It seems to me that you could implement Jeff's
PARSE_OPT_STOP_AT_UNKNOWN, and then if multiple option parsers are
needed you would simply loop over parse_options for each of the
commands, waiting for argc to stop changing.  Of course Jeff's flag
would also need to stop parse_options from eating the first argument.
Is this sort of what you are suggesting Junio?

--
Shawn

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Johannes Schindelin]

Hi,

On Thu, 19 Jun 2008, Shawn Bohrer wrote:

> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Tue, 17 Jun 2008, Junio C Hamano wrote:
> > >
> > >> Jeff King <peff@peff.net> writes:
> > >> 
> > >> > I think the only right way to accomplish this is to convert the 
> > >> > revision and diff parameters into a parseopt-understandable 
> > >> > format.
> > >> 
> > >> Not necessarily.  You could structure individual option parsers 
> > >> like how diff option parsers are done.  You iterate over argv[], 
> > >> feed diff option parser the current index into argv[] and ask if it 
> > >> is an option diff understands, have diff eat the option (and 
> > >> possibly its parameter) to advance the index, or allow diff option 
> > >> to say "I do not understand this", and then handle it yourself or 
> > >> hand it to other parsers.
> > >
> > > AFAIR Pierre tried a few ways, and settled with a macro to introduce 
> > > the diff options into a caller's options.
> > >
> > > IOW it would look something like this:
> > >
> > > static struct option builtin_what_options[] = {
> > > 	[... options specific to this command ...]
> > > 	DIFF__OPT(&diff_options)
> > > };
> > 
> > I think that is the more painful approach Jeff mentioned, and my 
> > comment was to show that it is not the only way.
> > 
> 
> It seems to me that you could implement Jeff's 
> PARSE_OPT_STOP_AT_UNKNOWN, and then if multiple option parsers are 
> needed you would simply loop over parse_options for each of the 
> commands, waiting for argc to stop changing.  Of course Jeff's flag 
> would also need to stop parse_options from eating the first argument. Is 
> this sort of what you are suggesting Junio?

I believe not.  I think that Junio prefers some callback that can handle a 
whole bunch of options (as opposed to the callback we can have now, to 
handle arguments for a specific option).

However, I am not sure what that would buy us over the approach Pierre 
settled.  Junio, maybe you thought that the option parsing macro would 
live in parse-options.h?  It was supposed to live in diff.h and 
revision.h, respectively.

Ciao,
Dscho

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 19 Jun 2008, Shawn Bohrer wrote:
>
>> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:
> I believe not.  I think that Junio prefers some callback that can handle a 
> whole bunch of options (as opposed to the callback we can have now, to 
> handle arguments for a specific option).

Sorry, no.  I do not want callbacks.  I've been saying that parser
cascading is easier if you use an incremental interface like diff option
parser does.

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Jeff King]

On Mon, Jun 23, 2008 at 12:55:00PM -0700, Junio C Hamano wrote:

> >> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:
> > I believe not.  I think that Junio prefers some callback that can handle a 
> > whole bunch of options (as opposed to the callback we can have now, to 
> > handle arguments for a specific option).
> 
> Sorry, no.  I do not want callbacks.  I've been saying that parser
> cascading is easier if you use an incremental interface like diff option
> parser does.

Now I'm confused: my understanding is that the diff option parser just
leaves unrecognized stuff in argv. But isn't that what a
PARSE_OPTIONS_IGNORE_UNKNOWN flag would do, and isn't that wrong?

-Peff

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Now I'm confused: my understanding is that the diff option parser just
> leaves unrecognized stuff in argv. But isn't that what a
> PARSE_OPTIONS_IGNORE_UNKNOWN flag would do, and isn't that wrong?

I was thinking more about the way how the lower level diff_opt_parse()
works by letting the caller to handle things that it itself does not know
how.

But I say this because I am not interested in "-a -b -c <=> -abc" and
haven't thought about how you would go about parsing something like that
sanely with partial knowledge.

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Johannes Schindelin]

Hi,

On Mon, 23 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 19 Jun 2008, Shawn Bohrer wrote:
> >
> >> On Wed, Jun 18, 2008 at 11:52:42AM -0700, Junio C Hamano wrote:
> >
> > I believe not.  I think that Junio prefers some callback that can 
> > handle a whole bunch of options (as opposed to the callback we can 
> > have now, to handle arguments for a specific option).
> 
> Sorry, no.  I do not want callbacks.  I've been saying that parser 
> cascading is easier if you use an incremental interface like diff option 
> parser does.

Sorry, I misunderstood.  At least you clarified it for me now.

Thanks,
Dscho

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Wed, Jun 18, 2008 at 05:13:02AM +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > I think the only right way to accomplish this is to convert the revision
> > and diff parameters into a parseopt-understandable format.
> 
> Not necessarily.  You could structure individual option parsers like how
> diff option parsers are done.  You iterate over argv[], feed diff option
> parser the current index into argv[] and ask if it is an option diff
> understands, have diff eat the option (and possibly its parameter) to
> advance the index, or allow diff option to say "I do not understand this",
> and then handle it yourself or hand it to other parsers.

  If you do that, you need to relocate pars option structures, and we
decided some time ago that it wasn't a good idea. Note that "recursing"
is not really trivial, because with flags aggregation and stuff like
that, things that look like an option can also be a value in the context
of an other option parser.

  That's why we settled for the other way Dscho pointed. But for that, I
need to work on it more than what I really have time to nowadays, and
moreover, it needs some things (the setup_revisions split and the log
traversal bits change) to be merged.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Wed, Jun 18, 2008 at 05:13:02AM +0000, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> > I think the only right way to accomplish this is to convert the revision
>> > and diff parameters into a parseopt-understandable format.
>> 
>> Not necessarily.  You could structure individual option parsers like how
>> diff option parsers are done.  You iterate over argv[], feed diff option
>> parser the current index into argv[] and ask if it is an option diff
>> understands, have diff eat the option (and possibly its parameter) to
>> advance the index, or allow diff option to say "I do not understand this",
>> and then handle it yourself or hand it to other parsers.
>
> If you do that, you need to relocate pars option structures,...
> ... Note that "recursing"
> is not really trivial, because with flags aggregation and stuff like
> that, things that look like an option can also be a value in the context
> of an other option parser.

Note that I was just saying "not necessarily" in response to "the only
right way" to point out it is not the _only_ way.

Parse-options has been done in a tablish way and it would involve cost to
modify it in a way I outlined (even if such a rewrite would make chaining
different set of option parsers easier, as each parser needs to handle
only what it knows about and handling aggregation and stuff would become
trivial).  I do not know if it is worth the cost, and I am not married to
the option parser structure that diff and revision part uses.