Carlos MartÃn Nieto <cmn@xxxxxxxx> writes:
> 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.
> +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.
> + 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.
> 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;
--
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