Re: [PATCH] log: convert to parse-options

Carlos Martín Nieto [off-list ref] writes:

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;