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