On Mon, 23 Jun 2008, Jeff King wrote:
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
