git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] log: convert to parse-options

From
=?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= <cmn@elego.de>
Date
Apr 19, 2011, 12:33 UTC
Message-ID
<20110419123325.GA10814@bee.lab.cmartin.tk>
In-Reply-To
<7v7hawiww7.fsf@alter.siamese.dyndns.org>

Use parse-options in cmd_log_init instead of manually iterating through them. This makes the code a bit cleaner but more importantly allows us to catch the "--quiet" option which causes some of the log-related commands to misbehave as it would otherwise get passed on to the diff.

Also take this opportinity to add 'whatchanged' to the help output.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
Carlos Martín Nieto <cmn@elego.de> writes:
Show 19 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 =3D3D NULL;
>>
>>  static int default_show_root =3D3D 1;
>>  static int decoration_style;
>> +static int decoration_given =3D3D 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;
>>+     }
Show 7 quoted lines
> 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.
Ok, I've changed this to use the version in your fixup patch.
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);
Show 6 quoted lines
> 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.
>

git_config_long doesn't die with an error (though git_config_int does, if git_config_long isn't successful) but returns 0 if it can't parse the value. git_config_maybe_bool notices this and returns -1, which parse_decoration_style interprets as "was not boolean" and tries to match "short" or "full". If it can't, then it returns -1 and decoration_callback dies with the error message.

Be it as it may, gt_config_long doesn't output any error message at all, so what we pass is irrelevant, unless we want to future-proof it because we don't trust the git_config_maybe_bool call to let us handle the error ourselves in the future.

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.
Done.
>> +     if (help)
>> +             usage(builtin_log_usage);
> I think parse_options() handles -h and --help itself, so there is no
> longer need for this.
Indeed, removed.

As the help is generated from the option struct, I've changed the comment a bit to me more helpful and I've added 'whatchanged' to the usage message, as it looks bad if 'git whatchanged -h' doesn't tell you about itself.

I'm not completely sure what "show source" is meant to be, I think it's the source of merges, which could be added to that explanation, I guess.

 Documentation/git-format-patch.txt |    5 ++-
 builtin/log.c                      |   77 ++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 31 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 9a15d69..d1b0861 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,12 +25,16 @@ static const char *default_date_mode = NULL;
 
 static int default_show_root = 1;
 static int decoration_style;
+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>...\n"
+	"   or: git whatchanged [options] <object>...",
+	NULL
+};
 
 static int parse_decoration_style(const char *var, const char *value)
 {
@@ -49,12 +53,44 @@ static int parse_decoration_style(const char *var, const char *value)
 	return -1;
 }
 
+static int decorate_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		decoration_style = 0;
+	else if (arg)
+		decoration_style = parse_decoration_style("decorate", arg);
+	else
+		decoration_style = DECORATE_SHORT_REFS;
+
+	if (decoration_style < 0)
+		die("invalid --decorate option: %s", arg);
+
+	decoration_given = 1;
+
+	return 0;
+}
+
 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 dummy, source;
+
+	/*
+	 * The 'quiet' option is a dummy no-op to stop it from propagating
+	 * to the diff option parsing.
+	 */
+	const struct option builtin_log_options[] = {
+		OPT_BOOLEAN(0, "quiet", &dummy, "no-op, provided for compatability"),
+		OPT_BOOLEAN(0, "source", &source, "show source"),
+		{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, "decoration options (default: short)",
+		  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);
 
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
@@ -69,14 +105,12 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	/*
-	 * Check for -h before setup_revisions(), or "git log -h" will
-	 * fail when run without a git directory.
-	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
+	/* Any arguments at this point are not recognized */
+	if (argc > 1)
+		die("unrecognized argument: %s", argv[1]);
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 
@@ -92,26 +126,9 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 		if (rev->diffopt.nr_paths != 1)
 			usage("git logs can only follow renames on one pathname at a time");
 	}
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--decorate")) {
-			decoration_style = DECORATE_SHORT_REFS;
-			decoration_given = 1;
-		} else if (!prefixcmp(arg, "--decorate=")) {
-			const char *v = skip_prefix(arg, "--decorate=");
-			decoration_style = parse_decoration_style(arg, v);
-			if (decoration_style < 0)
-				die("invalid --decorate option: %s", arg);
-			decoration_given = 1;
-		} else if (!strcmp(arg, "--no-decorate")) {
-			decoration_style = 0;
-		} else if (!strcmp(arg, "--source")) {
-			rev->show_source = 1;
-		} else if (!strcmp(arg, "-h")) {
-			usage(builtin_log_usage);
-		} else
-			die("unrecognized argument: %s", arg);
-	}
+
+	if (source)
+		rev->show_source = 1;
 
 	/*
 	 * defeat log.decorate configuration interacting with --pretty=raw

-- 
1.7.4.2.437.g4fc7e.dirty
Previous: Junio C HamanoNext: Jeff King
Message 15 of 17 in “Bug in "git diff --quiet" handling.”
  1. Paul GortmakerApr 11, 2011
  2. Junio C HamanoApr 11, 2011
  3. format-patch: don't pass on the --quiet flagCarlos Martín Nieto, Apr 12, 2011
  4. format-patch: document --quiet optionCarlos Martín Nieto, Apr 12, 2011
  5. Junio C HamanoApr 12, 2011
  6. Junio C HamanoApr 12, 2011
  7. Carlos Martín NietoApr 13, 2011
  8. Junio C HamanoApr 12, 2011
  9. Carlos Martín NietoApr 13, 2011
  10. whatchanged: always show the headerCarlos Martín Nieto, Apr 13, 2011
  11. Junio C HamanoApr 13, 2011
  12. Carlos Martín NietoApr 13, 2011
  13. log: convert to parse-optionsCarlos Martín Nieto, Apr 14, 2011
  14. Junio C HamanoApr 14, 2011
  15. log: convert to parse-options=?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?=, Apr 19, 2011
  16. Jeff KingApr 20, 2011
  17. Carlos Martín NietoApr 20, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.