Hi Junio,
On Sat, Aug 7, 2010 at 3:42 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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?
Yes, I think so. And if I change the logic to what you suggest, it
will also make the later 'move/copy detect' related argument parsing
easy. Because in move/copy detect, I should remove the 'remain path'
before feed it to setup_revisions. So, I hope I can make this change
along with the 'move/copy detect' change together, I hope this is
acceptable. :)
--
Regards!
Bo
----------------------------
My blog: http://blog.morebits.org
Why Git: http://www.whygitisbetterthanx.com/
--
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