Re: [PATCH v5 5/9] built-in add -i: implement the main loop

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The reason why we did not start with the main loop to begin with is that
> it is the first user of `list_and_choose()`, which uses the `list()`
> function that we conveniently introduced for use by the `status`
> command.
>
> Apart from the "and choose" part, there are more differences between the
> way the `status` command calls the `list_and_choose()` function in the
> Perl version of `git add -i` compared to the other callers of said
> function. The most important ones:
>
> - The list is not only shown, but the user is also asked to make a
>   choice, possibly selecting multiple entries.

The list_and_choose() we have here shows and lets users choose and
returns the choice, but the above makes it sound as if it only shows
and the caller is responsible for asking the end-user input.  Is
this description outdated or something?

Perl allows us to return multiple choices, where it is a bit hard to
express it in C (perhaps because we are passing in an array of
structs to be shown as choices, list_and_choose could set a bit in
these structs to signal "this one, that one and that other one was
chosen", returning how many are chosen in total, to extend the
version here to bring it to feature-parity?).  So at this step, it
only lets the user one choice (or abort or ask for help).  Isn't the
lack of multiple choice the only difference this bullet item wants
to highlight?

> The Perl script `git-add--interactive.perl` mixed the purposes of the
> "list" and the "and choose" part into the same function. In the C
> version, we will keep them separate instead, calling the `list()`
> function from the `list_and_choose()` function.

That makes sense.

> +static ssize_t list_and_choose(struct add_i_state *s, struct string_list *items,
> +			       struct list_and_choose_options *opts)
> +{
> +	struct strbuf input = STRBUF_INIT;
> +	ssize_t res = LIST_AND_CHOOSE_ERROR;
> +
> +	for (;;) {
> +		char *p, *endp;

The scope of endp looks way too wide in this function, isn't it?
Even in the final state of the series, it only gets used to parse
an integer input using strtoul, inside a block of three lines.

Other than that, the code at this step was a pleasant read overall.

Thanks.