Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

> For `fetch_pack_fsck_config()` to discern between errors and unhandled
> config variables, the return code when `git_config_path()` errors is
> changed to a different value also indicating success. This frees up the
> previous return code to now indicate the provided config variable
> was unhandled. The behavior remains functionally the same.

> @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,
>  		char *path ;
>  
>  		if (git_config_pathname(&path, var, value))
> -			return 1;
> +			return 0;

So, after getting complaint about a misconfiguration, the caller
used to be able to, if they wanted to, tell two cases apart: a
misconfigured variable was ignored here, and we happily parsed the
configuration.  Now they no longer can tell them apart, because we
return 0 for both cases.

> -		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
> -			fsck_msg_types.len ? ',' : '=', path);
> +		strbuf_addf(msg_types, "%cskiplist=%s",
> +			msg_types->len ? ',' : '=', path);
>  		free(path);
>  		return 0;
>  	}
> @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,
>  		if (!value)
>  			return config_error_nonbool(var);
>  		if (is_valid_msg_type(msg_id, value))
> -			strbuf_addf(&fsck_msg_types, "%c%s=%s",
> -				fsck_msg_types.len ? ',' : '=', msg_id, value);
> +			strbuf_addf(msg_types, "%c%s=%s",
> +				msg_types->len ? ',' : '=', msg_id, value);
>  		else
>  			warning("Skipping unknown msg id '%s'", msg_id);
>  		return 0;
>  	}
>  
> -	return git_default_config(var, value, ctx, cb);
> +	return 1;
> +}

And a 1 is returned to signal a "we didn't see what we care about".

> +static int fetch_pack_config_cb(const char *var, const char *value,
> +				const struct config_context *ctx, void *cb)
> +{
> +	int ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);
> +	if (ret > 0)
> +		return git_default_config(var, value, ctx, cb);
> +
> +	return ret;
>  }

And we catch that 1 and ask git_default_config() to use what we
didn't use.

OK.  If I were doing this patch, I would probably have chosen not to
change the value used to signal "a misconfiguration but that is not
too serious so we'll ignore after warning", picked another and new
value, like 2, to signal "the key is not something we care about",
which would mean fetch_pack_config_cb() would call "default" only
when it sees 2.  But with the current callers, and with the callers
after this series, it is not necessary.  If we need to accomodate
other new callers later, we can make such an update then.

This step, together with other patches, are nicely done.

Will queue.  Thanks.