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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2011, 16:43 UTC
Message-ID: <7vtydlezfj.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1303776102-9085-5-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 18 quoted lines

> diff --git a/diff.c b/diff.c
> index 08aaa47..20fe02c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -45,6 +45,17 @@ static char diff_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_NORMAL,	/* FUNCINFO */
>  };
>  
> +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.

 . 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?

Show 13 quoted lines

> @@ -114,6 +125,44 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>  	return git_diff_basic_config(var, value, cb);
>  }
>  
> +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.