Re: [PATCH 4/6] Add config variable for specifying default --dirstat behavior

[Johan Herland <johan@herland.net>]

On Tuesday 26 April 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > +static void init_default_diff_options()
> > +{
> > +	static int initialized = 0;
> > +	if (initialized)
> > +		return;
> > +
> > +	default_diff_options.dirstat_percent = 3;
> > +
> > +	initialized = 1;
> > +}
> 
> This smells fishy on two counts.
> 
>  . The rest of the diff machinery is designed to be callable multiple
>    times by calling diff_setup(), and there should be no place for any
>    call-once function like this one.

True. I needed it because the hardcoded "options->dirstat_percent = 3" in 
diff_setup() would overwrite the "diff.dirstat=10" stored in 
default_diff_options.dirstat_percent. Instead, I needed the fallback "3" to 
be stored in default_diff_options.dirstat_percent before "diff.dirstat" was 
parsed.

>  . Why is dirstat-percent _so_ special that it is the only one that has
>    to be initialized this way, when the function name implies that this
>    is the central place to control the initialization of all diff
>    related options?

Once I added init_default_diff_options(), I did in fact try to move the 
other hardcoded diff options from diff_setup(). However, I ended up with a 
lot of test failures, so I quickly gave up on that.

In the upcoming re-roll, I have solved the problem in a different way (using 
a static variable to store the default dirstat percentage).

> > +static void dirstat_opt_args(struct diff_options *options, const char
> > *args) +{
> > +	const char *p = args;
> > +	while (*p) {
> > +		if (!prefixcmp(p, "changes")) {
> > + ...
> > +		}
> > +	}
> > +}
> 
> Please move this part to the previous patch in your reroll.  This helper
> is what the previous patch should have been written with.

Will do.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net