Re: [PATCH 01/11] Start to implement a built-in version of `git add --interactive`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 30, 2019, 23:40 UTC
Message-ID: <nycvar.QRO.7.76.6.1904301935400.45@tvgsbejvaqbjf.bet>
In-Reply-To: <20190418160633.GB8054@sigill.intra.peff.net>

Hi Jeff & Jeff,

On Thu, 18 Apr 2019, Jeff King wrote:

Show 27 quoted lines

> 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