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

[Jeff King <peff@peff.net>]

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

> >   $ 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.

> 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