From: Bo Yang <struggleyb.nku@gmail.com>
Date: Tue, 10 Aug 2010 15:40:39 GMT
Subject: Re: [PATCH v4 05/18] Parse the -L options
Message-ID: <AANLkTi=pzsPpC=gM3UEBAaMq7PGJYafW8SKHunVzrOyP@mail.gmail.com>
In-Reply-To: <7v39ur8r56.fsf@alter.siamese.dyndns.org>

Hi Junio,

On Sat, Aug 7, 2010 at 3:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Bo Yang <struggleyb.nku@gmail.com> 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/