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

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