RFC patchgitopt - command-line parsing enhancements

Here's my take at a new command-line option parser to reduce wear on my fingers. It handles both long and short options, permuting, automatic abbreviations, required arguments, optional arguments, and bundling.

It doesn't use getopt or argp, so it should be easily portable to non-GNU systems (and only uses git-isms trivially, so other programs can use it easily, too).

I've tested everything lightly but it seems to be working well. The unit test was very helpful throughout. More intensive testing, bugfixes and comments would be greatly appreciated.

It should be pretty easy to convert other git-* programs to use gitopt, and write shell wrappers for git.c like I did with am/checkout/commit. I'll probably do so in a few days once my fingers recover a bit, of course feedback and patches for bug reports/enhancements are encouraged, too.

1 - gitopt: a new command-line option parser for git
	This adds the parser, tests, and some simple changes to
	the git.c wrapper.

2 - update-index: convert to using gitopt
3 - ls-tree: convert to gitopt
4 - ls-files: convert to using gitopt
	These three are fairly straightforward conversions.
	I was somewhat disappointed that update-index didn't take
	kindly to permuting arguments, but it's also a good example
	how to disable permuting via gitopt: a == b

5 - gitopt: convert setup_revisions(), and diff_opt_parse()
	This is a fairly intrusive change that affects several
	important programs.  All the tests still pass, so it must be
	working :)

6 - commit: allow --pretty= args to be abbreviated
	This one isn't strictly dependent on gitopt, and can be
	trivially changed to work without the parent patches.

Eric Wong <normalperson@yhbt.net> writes:

Taken a superficial look at it.

Sounds nice, might be a tad too ambitious though. Looks intrusive at places.

And scary, especially the "eat" macros are very scary.

I have to think about it a bit.

Junio C Hamano <junkio@cox.net> wrote:

I wasn't overly happy with the addition of global variables to existing files and the way they're set (setup_revisions). But at least they're static. Of course, I'm not yet certain that I haven't introduced new bugs. All the tests pass, at least...

They look weird at first, but I think they help readability and maintainability once you get used to them. They let you focus on the important part of the function while hiding the boring parts from you. Quite elegant, imho.

Eric Wong <normalperson@yhbt.net> writes:

Sorry, there is no elegance to it as far as I can see. A macro invocation that creates a private function while it does not look like a function definition is already bad, you cannot have a comma in the stmt part, and the bare semicolons in the parenthesised text look insane.

If your patch were like this, it would have been a bit easier for me to understand what was going on during my first pass:

    static struct exec_args *ui_optparse
            (struct opt_spec *s, int ac, char **av, int *ac_p, int what)
    {
            struct exec_args *ea = one_arg(s, ac, av, ac_p);
            if (!ea) return NULL;
            switch (what) {
            case IGNORE_MISSING:
                    not_new = 1; break;
            case VERBOSE:
                    verbose = 1; break;
            case HELP:
                    usage(update_index_usage); break;
            }
            return nil_exec_args(ea);
    }

instead of

    gitopt_eat(opt_ignore_missing, not_new = 1;)
    gitopt_eat(opt_verbose, verbose = 1;)
    gitopt_eat(opt_h, usage(update_index_usage);)

Then, you would give an extra element in the table, and your argument parsing/splitting routine passes that one to the handler function:

    static const struct opt_spec update_index_ost[] = {
    ...
    { "ignore-missing", 0,	0, 0, ui_optparse, IGNORE_MISSING },
    { "verbose",	    0,	0, 0, ui_optparse, VERBOSE },
    { "help",	   'h',	0, 0, ui_optparse, HELP },
    { 0, 0 }

Another thing is I do not think we would want to make the argument parsing into callback style interface like you did. It actively encourages the option variables to be global (you could make it file scoped static but they are global nevertheless). If you can make it an iterator style, it would be a lot easier to see what is going on, I suspect. Then you would not even need the callback function pointers and small functions created by magic eat() macros.

Junio C Hamano <junkio@cox.net> wrote:

That sounds like a great idea, I'll work on it tonight.