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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johan Herland <johan@xxxxxxxxxxx> writes:

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

> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]