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

> Johan Herland <johan@herland.net> writes:
> > +--dirstat[=<arg1,arg2,...>]::
> > +	Output the distribution of relative amount of changes for each
> > +	sub-directory. The behavior of `--dirstat` can be customized by
> > +	passing it a comma separated list of arguments. The defaults
> > +	are controlled by the `diff.dirstat` configuration variable (see
> > +	linkgit:git-config[1]). The following arguments are available:
>
> These "arguments" feel more like "options" (or "parameters"), no?  Your
> code in diff.c also calls it "opt".  The second line of the proposed log
> message has the same issue.

> > +--
> > +`changes`;;
> > +	Compute the dirstat numbers by counting the lines that have been
> > +	removed from the source, or added to the destination. This ignores
> > +	the amount of pure code movements within a file.  In other words,
> > +	rearranging lines in a file is not counted as much as other changes.
> > +	This is the default `--dirstat` behavior.
> 
> "default behavior when no option is given"?

> > +`cumulative`;;
> > +	Count changes in a child directory for the parent directory as well.
> > +	Note that when using `cumulative`, the sum of the percentages
> > +	reported may exceed 100%. The default (non-cumulative) behavior can
> > +	be specified with the `noncumulative` argument.
> 
> So the later one wins?  I.e. --dirstat=cumulative,noncumulative from the
> command line (which seems silly), or more importantly with
> 
>     [alias]
>     	dstat = diff --dirstat=cumulative
> 
> and you can say "git dstat --dirstat=noncumulative A..B"?

> > diff --git a/diff.c b/diff.c
> > index cfbfa92..08aaa47 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3144,6 +3144,72 @@ static int stat_opt(struct diff_options
> > *options, const char **av)
> > 
> >  	return argcount;
> >  
> >  }
> 
> /*
>  * Document what the return value from this function means here.
>  */
> > +static int dirstat_opt(struct diff_options *options, const char **av)
> 
> Do you have to pass "const char **av", or just "const char *arg"?

> > +{
> > +	const char *p, *arg = av[0];
> > +	char *mangled = NULL;
> > +	char sep = '=';
> > +
> > +	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 + 2);
> > +		memcpy(mangled, "--dirstat=files", 15);
> > +		if (mangled[15]) {
> > +			assert(mangled[15] == '=');
> > +			mangled[15] = ',';
> > +		}
> > +		arg = mangled;
> > +		p = mangled + 9;
> 
> I understand you wanted to reuse the while() loop below, but I do not
> think it is worth it.  Isn't it easier to read if you handled the above
> cases in their if/else body and return?
> 
> 	if (--cumulative) {
> 		options->output_format |= DIFF_FORMAT_DIRSTAT;
>         	DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
>                 return 1;
> 	}
>         if (--dirstat-by-file) {
> 		options->output_format |= DIFF_FORMAT_DIRSTAT;
> 		DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> 		return 1;
> 	}
> 	...
> 
> 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.

> > +	}
> > +	else if (!prefixcmp(arg, "-X"))
> > +		p = arg + 2;
> > +	else if (!prefixcmp(arg, "--dirstat"))
> > +		p = arg + 9;
> > +	else
> > +		return 0;
> > +
> > +	options->output_format |= DIFF_FORMAT_DIRSTAT;
> > +
> > +	while (*p) {
> > +		if (*p != sep)
> 
> What happens to "diff -X3 A..B"?

> > +		if (!prefixcmp(p, "changes")) {
> > +			p += 7;
> > +			DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
> > +		}
> > +		else if (!prefixcmp(p, "files")) {
> > +			p += 5;
> > +			DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
> > +		}
> > +		else if (!prefixcmp(p, "noncumulative")) {
> > +			p += 13;
> > +			DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
> > +		}
> > +		else if (!prefixcmp(p, "cumulative")) {
> > +			p += 10;
> > +			DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
> > +		}
> > +		else if (isdigit(*p)) {
> > +			char *end;
> > +			options->dirstat_percent = strtoul(p, &end, 10);
> > +			assert(end > p);
> > +			p = end;
> > +		}
> 
> That's a senseless assert(), isn't it?
> 
> You already know the first letter is a digit, so assert(p < end) will
> always be true.  You may want to check that this particular option is all
> digit by checking (*end == '\0' || *end == ',') but that is done at the
> beginning of this loop anyway, so I don't think there is anything to
> check here.