"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
The very real downside of the approach we have to take here is that the
test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the
conversion is complete (the `--helper` approach would have let it pass,
even at each of the incremental conversion steps).
That actually is an issue of the quality of the tests you add for
this series. If they are organized along the arc of run_add_i()
gaining features, the invocation of "git add -i" in the tests can
selectively use USE_BUILTIN=true to verify what has been rewritten
so far.
In any case, I think I've seen the patches in this part of the
metaseries enough and I think they are quickly stabilizing. Let's
see if others can find and raise issues and otherwise start cooking
in 'next' sometime next week.
Thanks.
Hi Junio,
On Thu, 14 Nov 2019, Junio C Hamano wrote:
any case, I think I've seen the patches in this part of the metaseries
enough and I think they are quickly stabilizing. Let's see if others
can find and raise issues and otherwise start cooking in 'next'
sometime next week.
While reviewing the next patch series in this arc, I *just* noticed a
buffer overrun: in the main loop, `path + sep` might point to the
trailing `NUL`, but we assign `p += sep + 1;` at the end (which is only
correct when `path + sep` points at whitespace).
The fix is already pushed up into gitgitgadget/git#170, and the relevant
part of the range-diff reads like this:
@@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l
+ index = -1;
+ }
+
-+ p[sep] = '\0';
++ if (p[sep])
++ p[sep++] = '\0';
+ if (index < 0 || index >= items->nr)
+ printf(_("Huh (%s)?\n"), p);
+ else {
@@ add-interactive.c: static void list(struct add_i_state *s, struct string_list *l
+ break;
+ }
+
-+ p += sep + 1;
++ p += sep;
+ }
+
+ if (res != LIST_AND_CHOOSE_ERROR)
I plan on waiting for the PR build to finish, and maybe wait until
tomorrow just in case any further suggestion rolls in, then submit the
hopefully final iteration.
And yes, I think it is a good time to start cooking this in `next`, I,
for one, am prone to overlook anything crucial in those patches because
I have stared at them so often.
Ciao,
Dscho