Re: [PATCH v6 1/9] Start to implement a built-in version of `git add --interactive`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 14, 2019, 15:07 UTC
Message-ID: <nycvar.QRO.7.76.6.1911141602350.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqimnn9p9p.fsf@gitster-ct.c.googlers.com>

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