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

Commit Message

Linus Arver via GitGitGadget Nov. 4, 2019, 12:15 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 of items is prefixed with a marker indicating what items have
  been selected, if multi-selection is allowed.

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

These features will be implemented later, except the part where the user
can choose a command. At this stage, though, the built-in `git add -i`
still only supports the `status` command, with the remaining commands to
follow over the course of the next commits.

In addition, we also modify `list()` to support displaying the commands
in columns, even if there is currently only one.

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.

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.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 add-interactive.c | 135 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Nov. 8, 2019, 5:17 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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.

Johannes Schindelin Nov. 9, 2019, 11:21 a.m. UTC | #2

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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.

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

Patch

diff --git a/add-interactive.c b/add-interactive.c
index 174e07ce83..c6f7fbad36 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -46,6 +46,7 @@  static void init_add_i_state(struct add_i_state *s, struct repository *r)
 }
 
 struct list_options {
+	int columns;
 	const char *header;
 	void (*print_item)(int i, struct string_list_item *item, void *print_item_data);
 	void *print_item_data;
@@ -54,7 +55,7 @@  struct list_options {
 static void list(struct add_i_state *s, struct string_list *list,
 		 struct list_options *opts)
 {
-	int i;
+	int i, last_lf = 0;
 
 	if (!list->nr)
 		return;
@@ -65,8 +66,96 @@  static void list(struct add_i_state *s, struct string_list *list,
 
 	for (i = 0; i < list->nr; i++) {
 		opts->print_item(i, list->items + i, opts->print_item_data);
+
+		if ((opts->columns) && ((i + 1) % (opts->columns))) {
+			putchar('\t');
+			last_lf = 0;
+		}
+		else {
+			putchar('\n');
+			last_lf = 1;
+		}
+	}
+
+	if (!last_lf)
 		putchar('\n');
+}
+struct list_and_choose_options {
+	struct list_options list_opts;
+
+	const char *prompt;
+};
+
+#define LIST_AND_CHOOSE_ERROR (-1)
+#define LIST_AND_CHOOSE_QUIT  (-2)
+
+/*
+ * Returns the selected index.
+ *
+ * If an error occurred, returns `LIST_AND_CHOOSE_ERROR`. Upon EOF,
+ * `LIST_AND_CHOOSE_QUIT` is returned.
+ */
+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;
+
+		strbuf_reset(&input);
+
+		list(s, items, &opts->list_opts);
+
+		printf("%s%s", opts->prompt, "> ");
+		fflush(stdout);
+
+		if (strbuf_getline(&input, stdin) == EOF) {
+			putchar('\n');
+			res = LIST_AND_CHOOSE_QUIT;
+			break;
+		}
+		strbuf_trim(&input);
+
+		if (!input.len)
+			break;
+
+		p = input.buf;
+		for (;;) {
+			size_t sep = strcspn(p, " \t\r\n,");
+			ssize_t index = -1;
+
+			if (!sep) {
+				if (!*p)
+					break;
+				p++;
+				continue;
+			}
+
+			if (isdigit(*p)) {
+				index = strtoul(p, &endp, 10) - 1;
+				if (endp != p + sep)
+					index = -1;
+			}
+
+			p[sep] = '\0';
+			if (index < 0 || index >= items->nr)
+				printf(_("Huh (%s)?\n"), p);
+			else {
+				res = index;
+				break;
+			}
+
+			p += sep + 1;
+		}
+
+		if (res != LIST_AND_CHOOSE_ERROR)
+			break;
 	}
+
+	strbuf_release(&input);
+	return res;
 }
 
 struct adddel {
@@ -252,20 +341,48 @@  static int run_status(struct add_i_state *s, const struct pathspec *ps,
 	return 0;
 }
 
+typedef int (*command_t)(struct add_i_state *s, const struct pathspec *ps,
+			 struct string_list *files,
+			 struct list_options *opts);
+
+static void print_command_item(int i, struct string_list_item *item,
+			       void *print_command_item_data)
+{
+	printf(" %2d: %s", i + 1, item->string);
+}
+
 int run_add_i(struct repository *r, const struct pathspec *ps)
 {
 	struct add_i_state s = { NULL };
+	struct list_and_choose_options main_loop_opts = {
+		{ 4, N_("*** Commands ***"), print_command_item, NULL },
+		N_("What now")
+	};
+	struct {
+		const char *string;
+		command_t command;
+	} command_list[] = {
+		{ "status", run_status },
+	};
+	struct string_list commands = STRING_LIST_INIT_NODUP;
+
 	struct print_file_item_data print_file_item_data = {
 		"%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
 	};
 	struct list_options opts = {
-		NULL, print_file_item, &print_file_item_data
+		0, NULL, print_file_item, &print_file_item_data
 	};
 	struct strbuf header = STRBUF_INIT;
 	struct string_list files = STRING_LIST_INIT_DUP;
+	ssize_t i;
 	int res = 0;
 
+	for (i = 0; i < ARRAY_SIZE(command_list); i++)
+		string_list_append(&commands, command_list[i].string)
+			->util = command_list[i].command;
+
 	init_add_i_state(&s, r);
+
 	strbuf_addstr(&header, "      ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
@@ -279,11 +396,25 @@  int run_add_i(struct repository *r, const struct pathspec *ps)
 
 	res = run_status(&s, ps, &files, &opts);
 
+	for (;;) {
+		i = list_and_choose(&s, &commands, &main_loop_opts);
+		if (i == LIST_AND_CHOOSE_QUIT) {
+			printf(_("Bye.\n"));
+			res = 0;
+			break;
+		}
+		if (i != LIST_AND_CHOOSE_ERROR) {
+			command_t command = commands.items[i].util;
+			res = command(&s, ps, &files, &opts);
+		}
+	}
+
 	string_list_clear(&files, 1);
 	strbuf_release(&print_file_item_data.buf);
 	strbuf_release(&print_file_item_data.index);
 	strbuf_release(&print_file_item_data.worktree);
 	strbuf_release(&header);
+	string_list_clear(&commands, 0);
 
 	return res;
 }