From: Slavica Djukic Date: Fri, 01 Mar 2019 11:20:57 GMT Subject: Re: [PATCH v5 04/10] add-interactive.c: implement list_and_choose Message-ID: <25f71e47-acad-e985-4f3f-ecde77f883d6@gmail.com> In-Reply-To: On 22-Feb-19 10:46 PM, Junio C Hamano wrote: > "Slavica Djukic via GitGitGadget" 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. >