Bo Yang <struggleyb.nku@xxxxxxxxx> writes:
> 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.
> + 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.
> ...
> + 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?
--
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