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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 26, 2024 at 06:57:06PM -0600, Justin Tobler wrote:
> diff --git a/fetch-pack.c b/fetch-pack.c
> index fe1fb3c1b7..e7d4f6e6e2 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -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;
> +}
> +
> +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;
>  }
>  
>  static void fetch_pack_config(void)

Okay, this now reads a lot nicer. But I'm sceptical whether we should
return -1 for the case where the value wasn't handled because we now
cannot discern the case where the function returns an error from the
case where it simply didn't handle the value.

In fact, we cannot even use positive values right now as far as I can
see:

  - We return 0 on success.

  - We return 1 in case `git_config_pathname()` indicates an error.

  - We return -1 when there is no value with "fetch.fsck.".

I'd propose to have a look at whether the positive return value from the
second case is actually used anywhere. If not, we can refactor this case
so that we always return negative on errors. And then we could further
adapt the function to return positive in case it didn't handle the
value.

> diff --git a/fetch-pack.h b/fetch-pack.h
> index b5c579cdae..c667b6fbf3 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -106,4 +106,7 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);
>   */
>  int fetch_pack_fsck_objects(void);
>  
> +int fetch_pack_fsck_config(const char *var, const char *value,
> +			   struct strbuf *msg_types);

We should also add some docs here, at the least to document the error
codes.

Patrick