Re: [PATCH] log: convert to parse-options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 14, 2011, 17:08 UTC
- Message-ID
- <7v7hawiww7.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1302791310-19640-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
Show 9 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c > index 9a15d69..5316be3 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -25,6 +25,7 @@ static const char *default_date_mode = NULL; > > static int default_show_root = 1; > static int decoration_style; > +static int decoration_given = 0;
We prefer to leave zero-initialization of statics to the linker, similarly to how we initialize decoration_style to zero in the line above.
Show 6 quoted lines
> +static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +{
> + if (unset) {
> + decoration_style = 0;
> + return 0;
> + }This is not a new issue, but I do not think the original code did not mean to keep decoration_given unmodified when --no-decorate was given from the command line. The variable is about "did we get any --decorate related options from the command line to override whatever log.decorate variable says?", and when we saw --no-decorate, we did get such an override from the command line. We should consistently set _given variable to 1 here.
It is immaterial that it happens not to matter to the current user of the variable that sets decoration_style to zero. The next user of _given may want to do other things.
Show 8 quoted lines
> + if (arg == NULL) {
> + decoration_style = DECORATE_SHORT_REFS;
> + decoration_given = 1;
> + return 0;
> + }
> +
> + /* First arg is irrelevant, as it just tries to parse arg */
> + decoration_style = parse_decoration_style("decorate", arg);It is used in the error message in git_config_long() in the callchain from here, primarily meant to report which configuration variable had a bad value, so it is in no way irrelevant. We need to say that a bad value comes not from a configuration but from the command line; get_color() in builtin/config.c passes "command line" for this exact reason.
Show 21 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;
> + int help, quiet, source;
> +
> + const struct option builtin_log_options[] = {
> + OPT_BOOLEAN(0, "h", &help, "show help"),
> + OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"),
> + OPT_BOOLEAN(0, "source", &source, "show source"),
> + { OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options",
> + PARSE_OPT_OPTARG, decorate_callback},
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, builtin_log_options,
> + builtin_log_usage,
> + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
> + PARSE_OPT_KEEP_DASHDASH);The 5th parameter is an array of strings terminated with a NULL element.
> + if (help) > + usage(builtin_log_usage);
I think parse_options() handles -h and --help itself, so there is no longer need for this.
How about this fix-up patch on top of your version?
builtin/log.c | 36 ++++++++++++++---------------------- 1 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c index 5316be3..80766a9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -25,13 +25,15 @@ static const char *default_date_mode = NULL; static int default_show_root = 1; static int decoration_style; -static int decoration_given = 0; +static int decoration_given; static const char *fmt_patch_subject_prefix = "PATCH"; static const char *fmt_pretty; -static const char * const builtin_log_usage = +static const char * const builtin_log_usage[] = { "git log [<options>] [<since>..<until>] [[--] <path>...]\n" - " or: git show [options] <object>..."; + " or: git show [options] <object>...", + NULL +}; static int parse_decoration_style(const char *var, const char *value) { @@ -52,19 +54,13 @@ static int parse_decoration_style(const char *var, const char *value) static int decorate_callback(const struct option *opt, const char *arg, int unset) { - if (unset) { + if (unset) decoration_style = 0; - return 0; - } - - if (arg == NULL) { + else if (arg) + decoration_style = parse_decoration_style("command line", arg); + else decoration_style = DECORATE_SHORT_REFS; - decoration_given = 1; - return 0; - } - /* First arg is irrelevant, as it just tries to parse arg */ - decoration_style = parse_decoration_style("decorate", arg); if (decoration_style < 0) die("invalid --decorate option: %s", arg); @@ -77,10 +73,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, struct rev_info *rev, struct setup_revision_opt *opt) { struct userformat_want w; - int help, quiet, source; + int quiet, source; const struct option builtin_log_options[] = { - OPT_BOOLEAN(0, "h", &help, "show help"), OPT_BOOLEAN(0, "quiet", &quiet, "supress diff output"), OPT_BOOLEAN(0, "source", &source, "show source"), { OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decorate options", @@ -88,13 +83,10 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, OPT_END() }; - argc = parse_options(argc, argv, prefix, builtin_log_options, - builtin_log_usage, - PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN | - PARSE_OPT_KEEP_DASHDASH); - - if (help) - usage(builtin_log_usage); + argc = parse_options(argc, argv, prefix, + builtin_log_options, builtin_log_usage, + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN | + PARSE_OPT_KEEP_DASHDASH); rev->abbrev = DEFAULT_ABBREV; rev->commit_format = CMIT_FMT_DEFAULT;