Re: [PATCH 3/6] Refactor --dirstat parsing; deprecate --cumulative and --dirstat-by-file

[Junio C Hamano <gitster@pobox.com>]

Johan Herland <johan@herland.net> writes:

>> Even better, probably they can be left to diff_opt_parse() without
>> calling this function, as you are deprecating them and do not have to
>> allow them to take the opt1,opt2,... form of parameter.
>
> I understand, but politely disagree: Patch 6/6 complicates the logic
> that DIFF_OPT_SET()/CLR() various bits in the diff options. I'd rather
> keep that logic in one place, than duplicate it into diff_opt_parse().

I've given a brief look at the v3.  Looks better than the previous one;
not using double is especailly a big and good thing.

There still are a few things I noticed, two of which I'd attempt to show
how to fix in the attached patch (on top of the whole 6 patch series, as I
don't have time to break it down and I know you are capable enough to do
so yourself).

 * We eradicated the use of C99_FORMAT at 28bd70d (unbreak and eliminate
   NO_C99_FORMAT, 2011-03-16) and "%td" at 31d713d (mktag: avoid %td in
   format string, 2011-03-16).

 * parse_dirstat_params() can die when it sees an input that it does not
   understand, instead of silently returning and indicating an error by
   its return value, even when it is called from the codepath to read the
   configuration files.  Dying upon an erroneous command line argument is
   fine and diagnosing it is preferred, but the configuration parser
   should ignore values that it does not understand (may want to warn) so
   that you can keep using older git (i.e. the version resulting from your
   patch) in a repository you usually use newer git that supports even
   more features with its --dirstat option.

 * Temporary memory allocation in your dirstat_opt() to handle commonly
   used shorthand stands out as a sore thumb.

 * The parsing implemented in dirstat_opt() is a bit too loose.  For
   example, we never accepted "-X=3" nor "--dirstat40" but I suspect your
   parser would.  Accepting the former might not be such a big deal, but
   not the latter.

The attached is not a complete fix-up, but addresses the last two issues,
and it also should be a good starting point for the second issue.

I tried not to fix style issues, but parse_dirstat_params() should follow

	if (...) {
            ... compound ...
	} else if (...) {
            ... compound ...
	} else if (...) {
	    ...

i.e. close brace just before the "else if" on the same line.

 diff.c |   94 +++++++++++++++++++++++++++++----------------------------------
 1 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/diff.c b/diff.c
index 9008e88..7c6a8d1 100644
--- a/diff.c
+++ b/diff.c
@@ -73,9 +73,14 @@ static int parse_diff_color_slot(const char *var, int ofs)
 #define PD_FMT "%td"
 #endif
 
-static void parse_dirstat_params(struct diff_options *options, const char *params)
+static int parse_dirstat_params(struct diff_options *options, const char *params,
+				int die_on_error)
 {
 	const char *p = params;
+	const char *unknown_param_error = "Unknown --dirstat parameter '%s'";
+	const char *missing_comma_error = "Missing comma separator at char " PD_FMT
+		" of '%s'";
+
 	while (*p) {
 		if (!prefixcmp(p, "changes")) {
 			p += 7;
@@ -109,19 +114,27 @@ static void parse_dirstat_params(struct diff_options *options, const char *param
 				options->dirstat_permille += *p - '0';
 				/* .. and ignore any further digits */
 				while (isdigit(*++p))
-					/* nothing */;
+					; /* nothing */
 			}
+		} else if (die_on_error) {
+			die(unknown_param_error, p);
+		} else {
+			return error(unknown_param_error, p);
 		}
-		else
-			die("Unknown --dirstat parameter '%s'", p);
 
-		if (*p) { /* more parameters, swallow separator */
-			if (*p != ',')
-				die("Missing comma separator at char " PD_FMT
-				    " of '%s'", p - params, params);
-			p++;
+		if (*p) {
+			/* more parameters, swallow separator */
+			if (*p == ',') {
+				p++;
+				continue;
+			}
+			if (die_on_error)
+				die(missing_comma_error, p - params, params);
+			else
+				return error(missing_comma_error, p - params, params);
 		}
 	}
+	return 0;
 }
 
 static int git_config_rename(const char *var, const char *value)
@@ -205,7 +218,7 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(var, "diff.dirstat")) {
 		default_diff_options.dirstat_permille = diff_dirstat_permille_default;
-		parse_dirstat_params(&default_diff_options, value);
+		(void) parse_dirstat_params(&default_diff_options, value, 0);
 		diff_dirstat_permille_default = default_diff_options.dirstat_permille;
 		return 0;
 	}
@@ -3252,45 +3265,14 @@ static int stat_opt(struct diff_options *options, const char **av)
 	return argcount;
 }
 
-/*
- * Parse dirstat-related options and any parameters given to those options.
- * Returns 1 if the option in 'arg' is a recognized dirstat-related option;
- * otherwise returns 0.
- */
-static int dirstat_opt(struct diff_options *options, const char *arg)
+static int parse_dirstat_opt(struct diff_options *options, const char *params)
 {
-	const char *p;
-	char *mangled = NULL;
-
-	if (!strcmp(arg, "--cumulative")) /* deprecated */
-		/* handle '--cumulative' like '--dirstat=cumulative' */
-		p = "cumulative";
-	else if (!strcmp(arg, "--dirstat-by-file") ||
-		 !prefixcmp(arg, "--dirstat-by-file=")) { /* deprecated */
-		/* handle '--dirstat-by-file=*' like '--dirstat=files,*' */
-		mangled = xstrdup(arg + 12);
-		memcpy(mangled, "files", 5);
-		if (mangled[5]) {
-			assert(mangled[5] == '=');
-			mangled[5] = ',';
-		}
-		p = mangled;
-	}
-	else if (!prefixcmp(arg, "-X"))
-		p = arg + 2;
-	else if (!prefixcmp(arg, "--dirstat"))
-		p = arg + 9;
-	else
-		return 0;
-
+	parse_dirstat_params(options, params, 1);
+	/*
+	 * The caller knows a dirstat-related option is given from the command
+	 * line; allow it to say "return this_function();"
+	 */
 	options->output_format |= DIFF_FORMAT_DIRSTAT;
-
-	if (*p)
-		if (*p == '=')
-			p++;
-		parse_dirstat_params(options, p);
-
-	free(mangled);
 	return 1;
 }
 
@@ -3313,10 +3295,20 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
 	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;
-	else if (!prefixcmp(arg, "-X") || !prefixcmp(arg, "--dirstat") ||
-		 !strcmp(arg, "--cumulative"))
-		/* -X, --dirstat[=<args>], --dirstat-by-file, or --cumulative */
-		return dirstat_opt(options, arg);
+	else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
+		return parse_dirstat_opt(options, "");
+	else if (!prefixcmp(arg, "-X"))
+		return parse_dirstat_opt(options, arg + 2);
+	else if (!prefixcmp(arg, "--dirstat="))
+		return parse_dirstat_opt(options, arg + 10);
+	else if (!strcmp(arg, "--cumulative"))
+		return parse_dirstat_opt(options, "cumulative");
+	else if (!strcmp(arg, "--dirstat-by-file"))
+		return parse_dirstat_opt(options, "files");
+	else if (!prefixcmp(arg, "--dirstat-by-file=")) {
+		parse_dirstat_opt(options, "files");
+		return parse_dirstat_opt(options, arg + 18);
+	}
 	else if (!strcmp(arg, "--check"))
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))