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@xxxxxxxxx> 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. 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. :) -Justin
