Hi Jonathan,
Many thanks to provide so much precious advice and even a patch, I
think the patch is really helpful. Thanks.
After some thought, I think we should keep the syntax as simple,
see my following comments.
On Mon, May 10, 2010 at 5:31 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> Jonathan Nieder wrote:
> The -L arguments describe lines in some particular revision of the
> files, so how would arbitrary ‘rev-list’-style revision specifiers
> work here? They don’t: in ‘blame’, one "positive" revision is allowed
> and the rest must be negative. Good.
A great reminder, thanks!
> The modified proposal was, roughly:
>
> git blame [options, no -L among them] revs ((-L range)... filespec)...
> git blame [options, -L permitted] revs -- [filespec...]
> git blame [options, -L permitted] revs [filespec...]
>
> “...” means “one or more”. How to know whether the -L or revision
> arguments are encountered first? One approach is to abuse
> STOP_AT_NON_OPTION to catch the -L, revisions, and filespecs as they
> appear. Probably better would be to make -L an unknown option and
> rely on parse_revision_opt leaving a residue of any revisions it
> finds, so that after the first pass, the first syntax can be
> distinguished from the others by the first argument starting with "-L".
How if the user provide now <revs> argument at all. In that case, we
may encounter the '-L' firstly too. And then we need to check whether
the non option argument is revision specifier. I think this kind of
check is not good to appear in builtin/log.c
> Feel free to do something else entirely (including another syntax) if
> you prefer, of course.
Yeah, I want to make a simpler syntax for the line level browser. It is:
git log [options without -L] revs ((-Llines)... filespec)...
We can then parse the options by:
1. Make -L a unknown option and call parse_revision_opt to filter out
all options and leave out revs, -L and pathspec.
2. Parse the remain options with '-L' a known option and your
STOP_AT_NON_OPTION way to matching all line ranges with pathspec.
Error out when there are some pathspec which has no line ranges
specified. And this time, leaving out the revs and pathspec. And put a
'--' just before the first '-L'.
3. Call setup_revisions using the remain arguments.
> Here’s a patch that makes STOP_AT_NON_OPTION easier to abuse, without
> affecting current users. Maybe it would make it easier to play
> around.
>
> Good night,
> Jonathan
>
> parse-options.c | 3 ++-
> parse-options.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 8546d85..4e3532b 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -372,7 +372,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
> if (parse_nodash_opt(ctx, arg, options) == 0)
> continue;
> if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
> - break;
> + return PARSE_OPT_NON_OPTION;
> ctx->out[ctx->cpidx++] = ctx->argv[0];
> continue;
> }
> @@ -454,6 +454,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
> switch (parse_options_step(&ctx, options, usagestr)) {
> case PARSE_OPT_HELP:
> exit(129);
> + case PARSE_OPT_NON_OPTION:
> case PARSE_OPT_DONE:
> break;
> default: /* PARSE_OPT_UNKNOWN */
> diff --git a/parse-options.h b/parse-options.h
> index 7581e93..4773cf9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -160,6 +160,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
> enum {
> PARSE_OPT_HELP = -1,
> PARSE_OPT_DONE,
> + PARSE_OPT_NON_OPTION,
> PARSE_OPT_UNKNOWN,
> };
>
> --
> 1.7.1
>
>
Really thanks for this patch I will use it.
Regards!
Bo
--
My blog: http://blog.morebits.org
--
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