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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Dec 03, 2024 at 08:23:11AM -0600, Justin Tobler wrote:
> On 24/12/03 10:34AM, Patrick Steinhardt wrote:
> > On Thu, Nov 28, 2024 at 12:25:44PM +0900, Junio C Hamano wrote:
> > > 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.
> > 
> > I wonder why we want to ignore these errors though. Grepping through the
> > codebase surfaces that all other callsites of `git_config_pathname()`
> > return its return value directly, which means that we'll die in case the
> > path name cannot be parsed. Shouldn't we do the same here, or is there a
> > good reason why we need to ignore it other than "We used to do it like
> > that"? In other words, I would have the below diff.
> 
> In both "fsck.c:git_fsck_config()" and
> "receive-pack:receive_pack_config()", the `git_config_pathname()`
> callsites are set to return 1 on error. The main reason for ignoring the
> error was to keep it consistent with the other fsck operations usage.
> Outside of that, I also wasn't quite sure why specifically
> "fsck.skipList" parsing is not as strict.

Okay, so your refactorings are simply keeping the status quo and are
consistent with what we do elsewhere where we parse the same config key.

> I do think being consistent would be nice, but I don't feel super
> strongly either way. I'm open to changing it in another version if that
> is best. :)

I think the current version of this patch series is fine as-is then. It
might be a good idea to adapt this in a follow-up series, unless there
is a good reason not to.

Patrick