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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2024, 03:25 UTC
Message-ID: <xmqqed2wox8n.fsf@gitster.g>
In-Reply-To: <20241127233312.27710-4-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 23 quoted lines

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

Show 9 quoted lines

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