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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jun 23, 2008, 18:47 UTC
Message-ID: <alpine.LFD.1.10.0806231137070.2926@woody.linux-foundation.org>
In-Reply-To: <20080623183358.GA28941@sigill.intra.peff.net>

On Mon, 23 Jun 2008, Jeff King wrote:

Show 13 quoted lines

> 
> It's worse than that. We assume by default that the option has no
> argument, so the argument becomes a non-option parameter to the original
> command. Try (with current git-blame, but I think your patch doesn't
> change this):
> 
>   $ git blame -n 1 git.c
>   fatal: bad revision '1'
> 
>   $ 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.

Hint: it _still_ involves PARSE_OPT_CONTINUE_ON_UNKNOWN. It would fix both 
cases.

IOW, the whole builtin-blame.c option parsing *should* look like this:

	argc = parse_options(argc, argv, options, usage, PARSE_OPT_CONTINUE_ON_UNKNOWN);
	init_revisions(&revs, NULL);
	setup_revisions(argc, argv, &revs, NULL);

and it should just work.

But we should *also* have some way to do things like the code in builtin-ls-files.c which have a few options that don't work well with the current parse_options(). Yes, you can make all of them work with callbacks, but that often ends up requiring moving arguments around. There's no way to make a trivial conversion for 90% of the cases, and then leaving the 10% that need other changes.

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.

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.

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

			Linus