Hi Jeff & Jeff,
On Thu, 18 Apr 2019, Jeff King wrote:
> On Thu, Apr 18, 2019 at 10:31:30AM -0400, Jeff Hostetler wrote:
>
> > Currently, neither function looks at any other k/v pairs, so
> > this is a bit of a moot point, but I'm wondering if this should
> > look like this:
> >
> > int add_config(...)
> > {
> > // give add-interactive.c a chance to look at k/v pair, but
> > // do not short-cut because we don't know yet whether we
> > // will be interactive or not yet.
> > (void)add_i_config(...);
> >
> > ...ignore_add_errors...
> > ...use_builtin_add_i...
> >
> > return git_default_config(...);
> > }
>
> 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.
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?
I guess I could invert the order, where `add_config()` would be called
as a fall-back from `add_i_config()`...
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`...
Ciao,
Dscho
