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

From: Jeff King <peff@peff.net>
Date: Jun 23, 2008, 19:53 UTC
Message-ID: <20080623195314.GA29569@sigill.intra.peff.net>
In-Reply-To: <alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org>

On Mon, Jun 23, 2008 at 11:47:49AM -0700, Linus Torvalds wrote:

Show 8 quoted lines

> >   $ git blame --default HEAD git.c
> >   fatal: cannot stat path HEAD: No such file or directory
> > 
> > Oops.
> 
> Oops. And then, how would you fix this most easily?
> 
> Be honest now.

Your statement is obviously loaded. I haven't seen _anything_ that fixes it except what I have already mentioned. But I'm sure you are going to complain that it isn't easy enough.

Show 5 quoted lines

> Example: many arguments cause multiple option variables to change. 
> parse_options() simply can't handle that well - you can do it with a 
> callback, but then you need to make the option variables global or make 
> them a structure or something. All of which just makes it nasty to do 
> partial conversions for the simple cases.

I'm not so sure. I assumed that most of the callbacks would simply take a "struct rev_list". So you would end up in builtin-blame.c with:

  ...
  OPT__REVISION(&my_rev_list),
  ...

in your options table. And if setup_revisions takes options that affect things that _aren't_ in that struct, then they probably ought to be.

> And I guarantee that just adding PARSE_OPT_{CONTINUE|STOP}_ON_UNKNOWN is 
> going to be the smallest patch, and make for the easiest usage case. It 
> may not be "pretty", but I can whip up a patch in five minutes.

I don't have a problem with STOP_ON_UNKNOWN, as I think it is a building block upon which sane things can be done (like linearly going through each parser and saying "did you want this?"). I think IGNORE/CONTINUE has a fundamental flaw.

> Or are we going to sit around discussing this for another five months?

Please! :)

Pierre was working on the approach I mentioned, but I think he is short on time. I will take a look at the conversion, but I have a few other fixes on my plate first.

In the meantime, I don't think your patch makes anything _worse_, since we already have these sorts of bugs in the current parsing code.

-Peff