On Sun, 2007-09-30 at 15:11 +0200, Jonas Fonseca wrote:
> Hello Kristian,
>
> I have some comments on your patch. Some of the "improvement" might have
> to wait until after your builtin-commit changes hits git.git. However,
> if we could agree on some of the general changes, I could start porting
> other of the main porcelain commands to use the option parser without
> depending on the state of the remaining builtin-commit series.
Hi Jonas,
That's sounds like a good plan. In fact, in you want to update the
patch with your changes (they all sound good) and start porting over
some of the other builtins feel free. I don't have much time follow up
on these comments right now, but I will get to it eventually - unless
you beat me to it of course ;) I will update builtin-commit.c to work
with whatever changes you introduce once I get around to updating that
patch.
> Kristian Høgsberg <krh@xxxxxxxxxx> wrote Thu, Sep 27, 2007:
> > Signed-off-by: Kristian Høgsberg <krh@xxxxxxxxxx>
> > ---
> > Makefile | 2 +-
> > parse-options.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > parse-options.h | 29 +++++++++++++++++++++
> > 3 files changed, 104 insertions(+), 1 deletions(-)
> > create mode 100644 parse-options.c
> > create mode 100644 parse-options.h
> >
> > diff --git a/Makefile b/Makefile
> > index 62bdac6..d90e959 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -310,7 +310,7 @@ LIB_OBJS = \
> > alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
> > color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
> > convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
> > - transport.o bundle.o
> > + transport.o bundle.o parse-options.o
> >
> > BUILTIN_OBJS = \
> > builtin-add.o \
> > diff --git a/parse-options.c b/parse-options.c
> > new file mode 100644
> > index 0000000..2fb30cd
> > --- /dev/null
> > +++ b/parse-options.c
> > @@ -0,0 +1,74 @@
> > +#include "git-compat-util.h"
> > +#include "parse-options.h"
> > +
> > +int parse_options(const char ***argv,
> > + struct option *options, int count,
> > + const char *usage_string)
> > +{
> > + const char *value, *eq;
> > + int i;
> > +
> > + if (**argv == NULL)
> > + return 0;
> > + if ((**argv)[0] != '-')
> > + return 0;
> > + if (!strcmp(**argv, "--"))
> > + return 0;
>
> I don't know if this is a bug, but you do not remove "--" from argv,
> which is later (in the patch that adds builtin-commit.c) passed to
> add_files_to_cache and then get_pathspec where it is not removed or
> detected either.
That's an oversight, good catch.
> > +
> > + value = NULL;
> > + for (i = 0; i < count; i++) {
> > + if ((**argv)[1] == '-') {
> > + if (!prefixcmp(options[i].long_name, **argv + 2)) {
> > + if (options[i].type != OPTION_BOOLEAN)
> > + value = *++(*argv);
> > + goto match;
> > + }
> > +
> > + eq = strchr(**argv + 2, '=');
> > + if (eq && options[i].type != OPTION_BOOLEAN &&
> > + !strncmp(**argv + 2,
> > + options[i].long_name, eq - **argv - 2)) {
> > + value = eq + 1;
> > + goto match;
> > + }
> > + }
> > +
> > + if ((**argv)[1] == options[i].short_name) {
> > + if ((**argv)[2] == '\0') {
> > + if (options[i].type != OPTION_BOOLEAN)
> > + value = *++(*argv);
> > + goto match;
> > + }
> > +
> > + if (options[i].type != OPTION_BOOLEAN) {
> > + value = **argv + 2;
> > + goto match;
> > + }
> > + }
> > + }
> > +
> > + usage(usage_string);
> > +
> > + match:
>
> I think the goto can be avoided by simply breaking out of the above loop
> when an option has been found and add ...
>
> > + switch (options[i].type) {
> case OPTION_LAST
> usage(usage_string);
> break;
>
> > + case OPTION_BOOLEAN:
> > + *(int *)options[i].value = 1;
> > + break;
Yeah, that looks nicer. I think the goto structure is a leftover from
when there was more logic between the loop and the switch. It's good to
get some fresh eyes on this code. Junio didn't like the OPTION_LAST
terminator, so I changed the interface to take a count. We can do
if (i == count)
usage();
else switch (options[i].type) {
...
}
of course.
> I've been looking at builtin-blame.c which IMO has some of the most
> obscure option parsing and maybe this can be changed to increment in
> order to support stuff like changing the meaning by passing the same arg
> multiple times (e.g. "-C -C -C") better.
That would be fine, yes.
> Blame option parsing also sports (enum) flags being masked together,
> this can of course be rewritten to a boolean option followed by
> masking when parse_options is done (to keep it sane).
Yup.
> > + case OPTION_STRING:
> > + if (value == NULL)
> > + die("option %s requires a value.", (*argv)[-1]);
>
> Maybe change this ...
>
> > + *(const char **)options[i].value = value;
> > + break;
> > + case OPTION_INTEGER:
> > + if (value == NULL)
> > + die("option %s requires a value.", (*argv)[-1]);
>
> ... and this to:
>
> if (!value) {
> error("option %s requires a value.", (*argv)[-1]);
> usage(usage_string);
> }
Sure, that's friendlier.
> > + *(int *)options[i].value = atoi(value);
> > + break;
> > + default:
> > + assert(0);
> > + }
> > +
> > + (*argv)++;
> > +
> > + return 1;
> > +}
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..39399c3
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,29 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > + OPTION_BOOLEAN,
> > + OPTION_STRING,
> > + OPTION_INTEGER,
> > + OPTION_LAST,
> > +};
> > +
> > +struct option {
> > + enum option_type type;
> > + const char *long_name;
> > + char short_name;
> > + void *value;
> > +};
>
> Space vs tab indentation.
>
> One of the last things I miss from Cogito is the nice abbreviated help
> messages that was available via '-h'. I don't know if it would be
> acceptable (at least for the main porcelain commands) to put this
> functionality into the option parser by adding a "description" member to
> struct option and have parse_options print a nice:
>
> <error message if any>
> <usage string>
> <option summary>
>
> on failure, or, if that is regarded as too verbose, simply when -h is
> detected.
Yeah, that might be nice. We can add it in a follow-on patch, if the
list agrees that it's a good thing, I guess.
> > +
> > +/* Parse the given options against the list of known options. The
> > + * order of the option structs matters, in that ambiguous
> > + * abbreviations (eg, --in could be short for --include or
> > + * --interactive) are matched by the first option that share the
> > + * prefix.
> > + */
>
> This prefix aware option parsing has not been ported over to the other
> builtins when they were lifted from shell code. It might be nice to have
> of course. Is it really needed?
I don't ever use it myself and I think it's more confusing than helpful.
I only added it to avoid introducing behavior changes in the port. I
don't have strong feelings either way.
> > +
> > +extern int parse_options(const char ***argv,
> > + struct option *options, int count,
> > + const char *usage_string);
>
> I think the interface could be improved a bit. For example, it doesn't
> need to count argument since the last entry in the options array is
> OPTION_LAST and thus the size can be detected that way.
Hehe, yeah, that's how I did it first. I don't have a strong preference
for terminator elements vs. ARRAY_SIZE(), but Junio prefers the
ARRAY_SIZE() approach, I guess. At this point I'm just trying the get
the patches upstream...
> Also, I think for this to be more usable for other built-in programs it
> shouldn't modify argv, but instead take both argc and argv (so we don't
> need to have code like "*++(*argv)" ;), parse _all_ options in one go,
> and return the index (of argv) for any remaining options.
>
> Then the following:
>
> while (parse_options(argv, commit_options, ARRAY_SIZE(commit_options),
> builtin_commit_usage))
> ;
>
> becomes:
>
> int i;
> ...
> i = parse_options(argc, argv, commit_options, builtin_commit_usage);
>
> This fits better with how option parsing is currently done. Take
> builtin-add for example:
>
> for (i = 1 ; i < argc ; i++) {
> const char *arg = argv[i];
> /* ... */
> }
> if (argc <= i)
> usage(builtin_rm_usage);
No objections, I think that looks better too.
> [ BTW, blame option parsing actually wants to know if "--" has been seen,
> but I think that can be worked around by simply checking argv[i - 1]
> after calling the option parser. ]
>
> > +
> > +#endif
>
> OK, I will stop these ramblings here. I hope the fact that I read your
> patch both back and forth and added comments in the process didn't make
> it too confusing.
Heh, that's what I do myself :)
thanks for the comments,
Kristian
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
