Re: [PATCH v4 05/18] Parse the -L options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 6, 2010, 19:42 UTC
- Message-ID
- <7v39ur8r56.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1281024717-7855-6-git-send-email-struggleyb.nku@gmail.com>
Bo Yang <struggleyb.nku@gmail.com> writes:
Show 10 quoted lines
> static void cmd_log_init(int argc, const char **argv, const char *prefix,
> struct rev_info *rev, struct setup_revision_opt *opt)
> {
> int i;
> int decoration_given = 0;
> struct userformat_want w;
> + const char *path = NULL, *pathspec = NULL;
> + static struct diff_line_range *range = NULL, *r = NULL;
> + static struct parse_opt_ctx_t ctx;
> + static struct line_opt_callback_data line_cb = {&range, &ctx, NULL};Do these have to be static? cmd_log_init() may be near the top of the call chain and has less reason to be reentrant, but it feels somewhat wrong if we are placing something that should live on stack in BSS.
Show 16 quoted lines
> + static const struct option options[] = {
> + OPT_CALLBACK('L', NULL, &line_cb, "n,m", "Process only line range n,m, counting from 1", log_line_range_callback),
> + OPT_END()
> + };
> + ...
> + parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
> + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);
> + for (;;) {
> + switch (parse_options_step(&ctx, options, log_opt_usage)) {
> + case PARSE_OPT_HELP:
> + exit(129);
> + case PARSE_OPT_DONE:
> + goto parse_done;
> + case PARSE_OPT_NON_OPTION:
> + ... do the extra path thing ...
> + pathspec = prefix_path(prefix, prefix ? strlen(prefix) : 0, path);Please do not call it "pathspec", as this is a specific path in a commit. "pathspec" is a pattern to be matched to zero or more paths.
Show 9 quoted lines
> ... > + parse_options_next(&ctx, 1); > + continue; > + case PARSE_OPT_UNKNOWN: > + parse_options_next(&ctx, 1); > + continue; > + } > + parse_revision_opt(rev, &ctx, options, log_opt_usage); > + }
Hmm, so the strategy is that you first run the command line through a pass of parse-options that is aware only of "-L" syntax, eat whatever it recognizes, and give remainder to the setup_revisions().
While I agree with that strategy in general, I think this implementation is ugly. It may be even wrong. For example, can a user specify a path that begins with a dash with this parser?
My gut feeling is that the capturing of the (optional) second argument given to -L is better done inside your callback.
Now, the current callback interface does not give you access to ctx so you may need to invent a new type of "more powerful callback API" that gives you access to the ctx as well, but if you did so, you should be able to do something like:
static int log_line_range_callback(...)
{
arg = parse_options_current(ctx);
... make sure it is a line range, e.g. "10,20"
parse_options_next(ctx); /* consume it */
path = parse_options_current(ctx); /* peek the second position */
if (does it look like a path?) {
... associate path with the range in arg
parse_options_next(ctx); /* consume it */
} else if (have we already got another range earlier?) {
... use the previous path with the range in arg
} else {
die("-L range not followed by path");
}
}no? In the above illustration, I am assuming that the "more powerful" one allows the callback to control even parsing of the first argument, i.e. parse-options does not call get_arg() before calling you back.
And "does it look like a path?" logic could say something like "If it is in the index, it is a path, even if it begins with a dash", or "If it is prefixed with ./, then it is always a path but we strip that dot-slash out", and somesuch, to make the heuristic of "do we have the optional second parameter?" better than "we do not allow a path that begins with a dash". After all, the callback for -L knows better than the generic "parse-options" infrastructure what to expect for the optional argument at the second position.
And if you do that, I suspect that you also can lose the "clear up the last range" hack after the loop is done, no?