Re: [RFC] Re: Convert 'git blame' to parse_options()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 23, 2008, 17:04 UTC
Message-ID: <alpine.DEB.1.00.0806231756340.6440@racer>
In-Reply-To: <alpine.LFD.1.10.0806230912230.2926@woody.linux-foundation.org>

Hi,

On Mon, 23 Jun 2008, Linus Torvalds wrote:

Show 5 quoted lines

> On Mon, 23 Jun 2008, Johannes Schindelin wrote:
> > 
> > Thinking about the recursive approach again, I came up with this POC:
> 
> "recursive" is pointless.

Nope, it is not.

To keep things _simple_ a callback is not good. Sure, you can work around the limitations of callbacks for aggregation, but the code change looks horrible. And the same holds true for the help message.

Just compare that to the recursive approach.

Okay, now for the "granted, your approach has merits" part.

> Look at cmd_apply() in builtin-apply.c. Notice how it currently 
> absolutely CANNOT sanely be turned into using "parse_options()", not 
> because it needs any "recursive" handling, but simply because it wants 
> to do *incremental* handling.

That is a totally independent issue from the one I discussed, namely sane handling of the diff (and rev) options.

> 	for (;;) {
> 		const char *arg;
> 		argc = parse_options(argc, argv,
> 			options, usage, PARSE_OPT_STOP_AT_UNKNOWN);

We do have PARSE_OPT_STOP_AT_NON_OPTION since a0ec9d25(parseopt: add flag to stop on first non option), which tries to solve a _similar_ problem, and it should be not hard at all to get PARSE_OPT_STOP_AT_UNKNOWN without changing the loop as you suggested.

Heck, we could just as easily introduce PARSE_OPT_IGNORE_UNKNOWN.

> Could you handle the "recursive" use of parse_options() in builtin-blame.c 
> by teaching it about recursion? Yes. But again, it's just _simpler_ to 
> just teach parse_options() to parse the things it knows about, and leave 
> the other things in place.

And here I disagree. You might not need a nice "--help" output, but most mortals do. And this is not easy with your approach.

In contrast, by using my approach of having an option_table for a bundle of common options, which just set variables in a certain struct, you can have a relatively painless migration, and you get all the benefits of parse-options.

But I guess the approach of whoever has more time to work on it will win... ;-)

Ciao, Dscho