Re: [PATCH v5 04/10] add-interactive.c: implement list_and_choose

[Slavica Djukic <slavicadj.ip2018@gmail.com>]

On 22-Feb-19 10:46 PM, Junio C Hamano wrote:
> "Slavica Djukic via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +#define HEADER_INDENT "      "
>> +
>>   enum collection_phase {
>>   	WORKTREE,
>>   	INDEX
>> @@ -27,6 +29,61 @@ struct collection_status {
>>   	struct hashmap file_map;
>>   };
>>   
>> +struct list_and_choose_options {
>> +	int column_n;
>> +	unsigned singleton:1;
>> +	unsigned list_flat:1;
>> +	unsigned list_only:1;
>> +	unsigned list_only_file_names:1;
>> +	unsigned immediate:1;
>> +	char *header;
>> +	const char *prompt;
> Makes a reader wonder if "header" can also be const (not to be taken
> as a suggestion to bend backwards to make it so).
>
>> +	void (*on_eof_fn)(void);
>> +};
>> +
>> +struct choice {
>> +	struct hashmap_entry e;
>> +	char type;
> If this is for choosing among the member of union, possible value(s)
> for the type member and which value corresponds to which union
> member must be documented somewhere, perhaps as a comment around
> here.
>
>> +	union {
>> +		void (*command_fn)(void);
>> +		struct {
>> +			struct {
>> +				uintmax_t added, deleted;
>> +			} index, worktree;
>> +		} file;
>> +	} u;
>> +	size_t prefix_length;
>> +	const char *name;
>> +};
>> +
>> +struct choices {
>> +	struct choice **choices;
> In general, do not name an array in plural.  An exception is when
> the code mostly refers to the array as a whole.
>
> When most accesses are to individual elements, then it would be a
> big win to be able to see choice[2] and pronounce it "the second
> choice" (you do not say "the second choices").
>
>> +	size_t alloc, nr;
>> +};
>> +#define CHOICES_INIT { NULL, 0, 0 }
>> +
>> +static int use_color = -1;
>> +enum color_add_i {
>> +	COLOR_PROMPT,
>> +	COLOR_HEADER,
>> +	COLOR_HELP,
>> +	COLOR_ERROR
>> +};
>> +
>> +static char colors[][COLOR_MAXLEN] = {
> Do not be overly selfish to assume that this will stay to be the
> only color pallette in this file.  If this is the color palette for
> list_and_choose, then have it in its name, e.g. list_and_choose_color[]
> or something like that.
>
>> +	GIT_COLOR_BOLD_BLUE, /* Prompt */
>> +	GIT_COLOR_BOLD,      /* Header */
>> +	GIT_COLOR_BOLD_RED,  /* Help */
>> +	GIT_COLOR_RESET      /* Reset */
>> +};
> Is the above list of values and comments correct?
>
> Doesn't each member of enum correspond to each element in
> list_and_choose_color[][COLOR_MAXLEN] array?  It does not exactly
> match my intuition to have help text in red and error messages in
> plain color.


I noticed I didn't add colors in corresponding commits, but list is 
correct -- later
on in patch series there is

GIT_COLOR_BOLD_RED, /* Error*/

added so that error messages are shown in red.


Help text is also in red following up what is happening in 
git-add--interactive.perl.

>> @@ -186,3 +243,73 @@ static struct collection_status *list_modified(struct repository *r, const char
>>   	free(files);
>>   	return s;
>>   }
>> +
>> +static struct choices *list_and_choose(struct choices *data,
>> +				       struct list_and_choose_options *opts)
>> +{
>> +	if (!data)
>> +		return NULL;
>> +
>> +	while (1) {
>> +		int last_lf = 0;
>> +
>> +		if (opts->header) {
>> +			const char *header_color = get_color(COLOR_HEADER);
>> +			if (!opts->list_flat)
>> +				printf(HEADER_INDENT);
> I won't complain if this is sufficient for the application, but the
> above would not allow different level of indentation depending on
> what header is being shown.  It may make sense to get rid of list_flat
> boolean and instead allow a new "const char *header_indent" member
> in the opts structure supplied by the caller.
>
> Don't use printf() when you _know_ you want to show a simple string
> without any % interpolation.  fputs(HEADER_INDENT, stdout) would suffice.
>
>> +			color_fprintf_ln(stdout, header_color, "%s", opts->header);
>> +		}
>> +
>> +		for (int i = 0; i < data->nr; i++) {
> We do not say "for (int i" (see a previous review).
>
>> +			struct choice *c = data->choices[i];
>> +			char *print;
>> +			const char *modified_fmt = _("%12s %12s %s");
>> +			char worktree_changes[50];
>> +			char index_changes[50];
>> +			char print_buf[100];
> It appears that many of these variables are only needed inside "we
> are showing 'f' and not just names" block.  Can their scope be
> narrowed?


Yes, I will change this.


>
>> +			print = (char *)c->name;
> Yuck.  Stuff c->name into print_buf[] instead and get rid of "print"
> pointer.
>
>> +			if ((data->choices[i]->type == 'f') && (!opts->list_only_file_names)) {
>> +				uintmax_t worktree_added = c->u.file.worktree.added;
>> +				uintmax_t worktree_deleted = c->u.file.worktree.deleted;
>> +				uintmax_t index_added = c->u.file.index.added;
>> +				uintmax_t index_deleted = c->u.file.index.deleted;
>> +
>> +				if (worktree_added || worktree_deleted)
>> +					snprintf(worktree_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX,
>> +						 worktree_added, worktree_deleted);
>> +				else
>> +					snprintf(worktree_changes, 50, "%s", _("nothing"));
>> +				if (index_added || index_deleted)
>> +					snprintf(index_changes, 50, "+%"PRIuMAX"/-%"PRIuMAX,
>> +						 index_added, index_deleted);
>> +				else
>> +					snprintf(index_changes, 50, "%s", _("unchanged"));
>> +
>> +				snprintf(print_buf, 100, modified_fmt, index_changes,
>> +					 worktree_changes, print);
> All of the above look overly repetitious; a helper function that
> takes a pointer to "struct { uintmax_t a, d; }" and populates
> changes[] buffer would cut them down by half, but other than that
> I do not see a room for drastic improvement here X-<.
>
> Oh, it would greatly help to use two strbuf for wt/ix_changes that
> are defined outside the loop that is strbuf_reset() after each
> iteration and use things like strbuf_addf().
>
>> +				print = xmalloc(strlen(print_buf) + 1);
>> +				snprintf(print, 100, "%s", print_buf);
> Likewise.
>
>> +			}
>> +
>> +			printf(" %2d: %s", i + 1, print);
>> +			if ((opts->list_flat) && ((i + 1) % (opts->column_n))) {
>> +				printf("\t");
>> +				last_lf = 0;
>> +			}
>> +			else {
>> +				printf("\n");
>> +				last_lf = 1;
>> +			}
>> +
>> +		}
>> +
>> +		if (!last_lf)
>> +			printf("\n");
>> +
>> +		return NULL;
>> +	}
>> +}
> This obviously only lists but does not let you choose at this step
> in the series, but that is OK.
>
> But I see a deeper problem with the design of this helper.  The
> things this helper can list is quite limited.
>
> The original was designed so that the shown strings are prepared by
> the caller and this helper is solely responsible for showing the
> choices, giving prompt, and accepting choice (in various abbreviated
> forms), all _WITHOUT_ having to know the meaning of what is in the
> list.  It gave us a much better separation of labor and
> responsibility between the caller and the callee, I would think.
>

Thanks for pointing this out. I talked to my mentor and I'm now working on
making list_and_choose more "type-independent".

I didn't reply to all suggestions in this message, but I did apply them 
in the code.

>