threads / rfc / 14008
Subject: [RFC] convert shortlog to use parse_options
Shawn BohrerI 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
Shawn Bohrer[PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Shawn Bohrer[PATCH 2/2] git shortlog: Modify to use parse_options
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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?
Jeff KingRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Jeff KingRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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.
Johannes SchindelinRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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.
Shawn BohrerRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Johannes SchindelinRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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.
Jeff KingRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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.
Johannes SchindelinRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Pierre HabouzitRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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
Junio C HamanoRe: [PATCH 1/2] parse_options: Add flag to prevent errors for further processing
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.