Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 21/07/14 12:44, Tanay Abhra wrote:
> Use `git_config_get_*()` family instead of `git_config()` to take advantage of
> the config-set API which provides a cleaner control flow.
> 
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> Consider this as a proof of concept as the others callers have to be rewritten
> as well.
> I think that it is not so buggy as it passes all the tests.
> After the first six patches in the series which you have already seen there are
> five or four left which can rewritten without touching git_default_config().
> 
> Thus, this rewrite will serve as the base for rewriting other git_config()
> callers which pass control to git_default_config() at the end of the function.
> Also there are more than thirty direct callers to git_default_config()
> (i.e git_config(git_default_config, NULL)), so this rewrite solves them
> in one sweep.
> 
> Slight behaviour change, config_error_nonbool() has been replaced with
> die("Missing value for '%s'", var);.
> The original code also alerted the file name and the line number which we lose here.
> 
> Cheers,
> Tanay Abhra.
> 
>  advice.c |  18 ++--
>  advice.h |   2 +-
>  cache.h  |   2 +-
>  config.c | 287 ++++++++++++++++++++-------------------------------------------
>  ident.c  |  15 ++--
>  5 files changed, 104 insertions(+), 220 deletions(-)
> 

[snip]


> diff --git a/config.c b/config.c
> index fe9f399..72196a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -666,88 +666,47 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
>  	return 0;
>  }
> 
> -static int git_default_core_config(const char *var, const char *value)
> +static void git_default_core_config(void)
>  {
> +	const char *value = NULL;
>  	/* This needs a better name */
> -	if (!strcmp(var, "core.filemode")) {
> -		trust_executable_bit = git_config_bool(var, value);
> -		return 0;
> -	}
> -	if (!strcmp(var, "core.trustctime")) {
> -		trust_ctime = git_config_bool(var, value);
> -		return 0;
> -	}
> -	if (!strcmp(var, "core.checkstat")) {
> +	git_config_get_bool("core.filemode", &trust_executable_bit);
> +	git_config_get_bool("core.trustctime", &trust_ctime);
> +
> +	if (!git_config_get_value("core.checkstat", &value)) {
>  		if (!strcasecmp(value, "default"))
>  			check_stat = 1;
>  		else if (!strcasecmp(value, "minimal"))
>  			check_stat = 0;
>  	}
> 
> -	if (!strcmp(var, "core.quotepath")) {
> -		quote_path_fully = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.symlinks")) {
> -		has_symlinks = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.ignorecase")) {
> -		ignore_case = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.attributesfile"))
> -		return git_config_pathname(&git_attributes_file, var, value);
> -
> -	if (!strcmp(var, "core.bare")) {
> -		is_bare_repository_cfg = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.ignorestat")) {
> -		assume_unchanged = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.prefersymlinkrefs")) {
> -		prefer_symlink_refs = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.logallrefupdates")) {
> -		log_all_ref_updates = git_config_bool(var, value);
> -		return 0;
> -	}
> +	git_config_get_bool("core.quotepath", &quote_path_fully);
> +	git_config_get_bool("core.symlinks", &has_symlinks);
> +	git_config_get_bool("core.ignorecase", &ignore_case);
> +	git_config_get_pathname("core.attributesfile", &git_attributes_file);
> +	git_config_get_bool("core.bare", &is_bare_repository_cfg);
> +	git_config_get_bool("core.ignorestat", &assume_unchanged);
> +	git_config_get_bool("core.prefersymlinkrefs", &prefer_symlink_refs);
> +	git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
> +	git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);
> 
> -	if (!strcmp(var, "core.warnambiguousrefs")) {
> -		warn_ambiguous_refs = git_config_bool(var, value);
> -		return 0;
> +	int abbrev;

declaration after statement.

> +	if (!git_config_get_int("core.abbrev", &abbrev)) {
> +		if (abbrev >= minimum_abbrev && abbrev <= 40)
> +			default_abbrev = abbrev;
>  	}
> 
> -	if (!strcmp(var, "core.abbrev")) {
> -		int abbrev = git_config_int(var, value);
> -		if (abbrev < minimum_abbrev || abbrev > 40)
> -			return -1;
> -		default_abbrev = abbrev;
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.loosecompression")) {
> -		int level = git_config_int(var, value);
> +	int level;

ditto.

> +	if (!git_config_get_int("core.loosecompression", &level)) {
>  		if (level == -1)
>  			level = Z_DEFAULT_COMPRESSION;
>  		else if (level < 0 || level > Z_BEST_COMPRESSION)
>  			die("bad zlib compression level %d", level);
>  		zlib_compression_level = level;
>  		zlib_compression_seen = 1;
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.compression")) {
> -		int level = git_config_int(var, value);
> +	if (!git_config_get_int("core.compression", &level)) {
>  		if (level == -1)
>  			level = Z_DEFAULT_COMPRESSION;
>  		else if (level < 0 || level > Z_BEST_COMPRESSION)
> @@ -756,57 +715,39 @@ static int git_default_core_config(const char *var, const char *value)
>  		core_compression_seen = 1;
>  		if (!zlib_compression_seen)
>  			zlib_compression_level = level;
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.packedgitwindowsize")) {
> +	if (!git_config_get_ulong("core.packedgitwindowsize", (long unsigned int*)&packed_git_window_size)) {
>  		int pgsz_x2 = getpagesize() * 2;
> -		packed_git_window_size = git_config_ulong(var, value);
> 
>  		/* This value must be multiple of (pagesize * 2) */
>  		packed_git_window_size /= pgsz_x2;
>  		if (packed_git_window_size < 1)
>  			packed_git_window_size = 1;
>  		packed_git_window_size *= pgsz_x2;
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.bigfilethreshold")) {
> -		big_file_threshold = git_config_ulong(var, value);
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.packedgitlimit")) {
> -		packed_git_limit = git_config_ulong(var, value);
> -		return 0;
> -	}
> +	git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
> +	git_config_get_ulong("core.packedgitlimit", (long unsigned int*)&packed_git_limit);
> +	git_config_get_ulong("core.deltabasecachelimit", (long unsigned int*)&delta_base_cache_limit);
> 
> -	if (!strcmp(var, "core.deltabasecachelimit")) {
> -		delta_base_cache_limit = git_config_ulong(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.autocrlf")) {
> +	if (!git_config_get_value("core.autocrlf", &value)) {
>  		if (value && !strcasecmp(value, "input")) {
>  			if (core_eol == EOL_CRLF)
> -				return error("core.autocrlf=input conflicts with core.eol=crlf");
> +				die("core.autocrlf=input conflicts with core.eol=crlf");
>  			auto_crlf = AUTO_CRLF_INPUT;
> -			return 0;
> -		}
> -		auto_crlf = git_config_bool(var, value);
> -		return 0;
> +		} else
> +			auto_crlf = git_config_bool("core.autocrlf", value);
>  	}
> 
> -	if (!strcmp(var, "core.safecrlf")) {
> -		if (value && !strcasecmp(value, "warn")) {
> +	if (!git_config_get_value("core.safecrlf", &value)) {
> +		if (value && !strcasecmp(value, "warn"))
>  			safe_crlf = SAFE_CRLF_WARN;
> -			return 0;
> -		}
> -		safe_crlf = git_config_bool(var, value);
> -		return 0;
> +		else
> +			safe_crlf = git_config_bool("core.safecrlf", value);
>  	}
> 
> -	if (!strcmp(var, "core.eol")) {
> +	if (!git_config_get_value("core.eol", &value)) {
>  		if (value && !strcasecmp(value, "lf"))
>  			core_eol = EOL_LF;
>  		else if (value && !strcasecmp(value, "crlf"))
> @@ -816,108 +757,74 @@ static int git_default_core_config(const char *var, const char *value)
>  		else
>  			core_eol = EOL_UNSET;
>  		if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
> -			return error("core.autocrlf=input conflicts with core.eol=crlf");
> -		return 0;
> +			die("core.autocrlf=input conflicts with core.eol=crlf");
>  	}
> 
> -	if (!strcmp(var, "core.notesref")) {
> -		notes_ref_name = xstrdup(value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.pager"))
> -		return git_config_string(&pager_program, var, value);
> -
> -	if (!strcmp(var, "core.editor"))
> -		return git_config_string(&editor_program, var, value);
> +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);
> +	git_config_get_string("core.pager", &pager_program);
> +	git_config_get_string("core.editor", &editor_program);
> 
> -	if (!strcmp(var, "core.commentchar")) {
> -		const char *comment;
> -		int ret = git_config_string(&comment, var, value);
> -		if (ret)
> -			return ret;
> -		else if (!strcasecmp(comment, "auto"))
> +	const char *comment;

ditto. I gave up here.

ATB,
Ramsay Jones