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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 9, 2019, 11:21 UTC
Message-ID: <nycvar.QRO.7.76.6.1911091211560.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq1rujndza.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Fri, 8 Nov 2019, Junio C Hamano wrote:

Show 32 quoted lines

> "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?

I changed the commit message to:

    built-in add -i: implement the main loop

    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.

    In contrast to the Perl version, in the built-in interactive `add`, we
    will keep the `list()` function (which only displays items) and the
    `list_and_choose()` function (which uses `list()` to display the items,
    and only takes care of the "and choose" part) separate.

    The `list_and_choose()` function, as implemented in
    `git-add--interactive.perl` knows a few more tricks than the function we
    introduce in this patch:

    - There is a flag to let the user select multiple items.

    - In multi-select mode, the list of items is prefixed with a marker
      indicating what items have been selected.

    - Initially, for each item a unique prefix is determined (if there
      exists any within the given parameters), and shown in the list, and
      accepted as a shortcut for the selection.

    These features will be implemented in the C version later.

    This patch does not add any new main loop command, of course, the
    built-in `git add -i` still only supports the `status` command. The
    remaining commands to follow over the course of the next commits.

    To accommodate for listing the commands in columns, preparing for the
    commands that will be implemented over the course of the next
    patches/patch series, we teach the `list()` function to do precisely
    that.

    Note that we only have a prompt ending in a single ">" at this stage;
    later commits will add commands that display a double ">>" to indicate
    that the user is in a different loop than the main one.

Show 19 quoted lines

> > 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.

True. I moved the declaration of `endp` into that three-line (now four-line) scope.

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

Thank you for your review! Dscho