On Tue, Apr 30, 2019 at 07:40:06PM -0400, Johannes Schindelin wrote: > > Yeah, I agree this split seems a bit more natural. It is worth > > propagating errors from add_i_config(), though, like: > > > > if (add_i_config(var, value, data)) > > return -1; > > > > so that any key-specific errors (e.g., config_error_nonbool) stop the > > parsing in the usual way. > > The only problem there is that `add_i_config()` (like all the other > `git_config()` callbacks) does not report whether it consumed the > key/value pair or not. I tried to avoid deviating from the standard > practice to avoid calling `git_default_config()` when we already consumed > the config setting. I don't think it's worth worrying too much about that. We wouldn't match the keys in multiple places anyway (and even if we did, it would arguably be the right thing to give every callback a chance to see them). The only thing it does is short-circuit the rest of the checks that we know won't match. But that doesn't really change the performance substantially; the worst case is already that we have to hit every possible strcmp(). And most of our config code does not worry about this, and is OK with branching (it just needs to propagate errors, as above). For some more discussion, see 6680a0874f (drop odd return value semantics from userdiff_config, 2012-02-07). All that said... > And I also tried pretty hard to *not* bleed any internal state of > `add-interactive` into `builtin/add`, as I wanted the new code to be as > libified as possible (in a nearby thread, somebody wished for a new `-p` > mode that would essentially be a combined `git stash -p` and `git add -p`, > and with properly libified code such a beast is a lot more feasible). > > Any idea how to deal with that? The most lib-ified thing is to just use the configset code. I.e., wherever you need the config, just load it on demand via git_config_get_int or whatever. > Or I invent a new convention where `add_i_config()` returns 1 when it > consumed the key/value pair. But that would set a precedent that is > inconsistent with the entire existing code base, something I am > uncomfortable to do for the sake of `add -i`... Yes, don't do that. :) That was the same thing we finally got rid of for userdiff_config(). -Peff
