On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote: > From: Nadav Goldstein <nadav.goldstein96@xxxxxxxxx> > > moved the code that responsible for presenting the menu options > (interactive flag) to be handles by the add-menu lib that was added > in previous commit. Please see Documentation/SubmittingPatches, specifically the "present tense" section [1]. [1] https://github.com/git/git/blob/f9b95943b68b6b8ca5a6072f50a08411c6449b55/Documentation/SubmittingPatches#L174-L179 > -static int clean_use_color = -1; > -static char clean_colors[][COLOR_MAXLEN] = { > - [CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED, > - [CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD, > - [CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED, > - [CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL, > - [CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE, > - [CLEAN_COLOR_RESET] = GIT_COLOR_RESET, > -}; > - > - > -static const char *clean_get_color(enum color_clean ix) > +static const char *clean_get_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color) I'm surprised to see changes to add-menu.c in this patch. I would expect that add-menu.c was written in its correct form in the first patch and then this patch could focus entirely on deleting matching code from builtin/clean.c and calling the API exposed in add-menu.h. Perhaps this was just an interactive rebase issue? Fixed up the wrong commit? That happens to me a lot. There are also a lot of places that refer to "clean" when you want this API to be abstracted away from 'git clean'. > \ No newline at end of file > diff --git a/add-menu.h b/add-menu.h > index 52e5ccb1800..64f1cbdab9f 100644 > --- a/add-menu.h > +++ b/add-menu.h > @@ -1,3 +1,7 @@ > +#include "color.h" > + > +typedef char clean_color_settings[][COLOR_MAXLEN]; This typedef also shouldn't reference 'git clean'. > + > #define MENU_OPTS_SINGLETON 01 > #define MENU_OPTS_IMMEDIATE 02 > #define MENU_OPTS_LIST_ONLY 04 > @@ -35,7 +39,7 @@ struct menu_stuff { > void *stuff; > }; > > -void clean_print_color(enum color_clean ix); > +void clean_print_color(enum color_clean ix, clean_color_settings *clean_colors, int *clean_use_color); > > /* > * Implement a git-add-interactive compatible UI, which is borrowed > @@ -48,4 +52,4 @@ void clean_print_color(enum color_clean ix); > * - The array ends with EOF. > * - If user pressed CTRL-D (i.e. EOF), no selection returned. > */ > -int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, void (*prompt_help_cmd)(int)); > \ No newline at end of file > +int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff, clean_color_settings *clean_colors, int *clean_use_color, void (*prompt_help_cmd)(int)); There are a lot of instances where your lines are much too wide. Documentation/CodingGuidelines has a lot of style requirements, including trying to stay to an 80-character width. There are a lot of examples in the codebase that show how to break a method prototype across multiple lines with tasteful vertical alignment. > diff --git a/builtin/clean.c b/builtin/clean.c > index 5466636e666..ef220869851 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -19,6 +19,7 @@ > #include "pathspec.h" > #include "help.h" > #include "prompt.h" > +#include "add-menu.h" > > static int force = -1; /* unset */ > static int interactive; > @@ -39,14 +40,6 @@ static const char *msg_warn_lstat_failed = N_("could not lstat %s\n"); > static const char *msg_skip_cwd = N_("Refusing to remove current working directory\n"); > static const char *msg_would_skip_cwd = N_("Would refuse to remove current working directory\n"); > > -enum color_clean { > - CLEAN_COLOR_RESET = 0, > - CLEAN_COLOR_PLAIN = 1, > - CLEAN_COLOR_PROMPT = 2, > - CLEAN_COLOR_HEADER = 3, > - CLEAN_COLOR_HELP = 4, > - CLEAN_COLOR_ERROR = 5 > -}; This removal doesn't seem valuable. I think this color set should remain in the builtin, especially because it's being used further down. Alternatively, the names can be renamed to "MENU_COLOR_..." to apply to all commands in the add-menu.h API. > static const char *color_interactive_slots[] = { > [CLEAN_COLOR_ERROR] = "error", > @@ -58,7 +51,7 @@ static const char *color_interactive_slots[] = { > }; > > static int clean_use_color = -1; > -static char clean_colors[][COLOR_MAXLEN] = { > +static clean_color_settings clean_colors = { > [CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED, > [CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD, > [CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED, > @@ -67,36 +60,8 @@ static char clean_colors[][COLOR_MAXLEN] = { > [CLEAN_COLOR_RESET] = GIT_COLOR_RESET, > }; > > -#define MENU_OPTS_SINGLETON 01 > -#define MENU_OPTS_IMMEDIATE 02 > -#define MENU_OPTS_LIST_ONLY 04 > - > -struct menu_opts { > - const char *header; > - const char *prompt; > - int flags; > -}; > - Generally, the remainder of this patch is primarily deletions. Although, we could make it be completely deletions if the API method names are changed (not to start with "clean_" and then all these calls are modified in one go: > static void prompt_help_cmd(int singleton) > { > - clean_print_color(CLEAN_COLOR_HELP); > + clean_print_color(CLEAN_COLOR_HELP, &clean_colors, &clean_use_color); Here, we would have something like "menu_print_color()" instead. To avoid adding too much deletion noise when making these important changes, we can add MAYBE_UNUSED to all of the static methods that become unreachable. After a patch that does that refactor, a diff that only deletes lines (does not add any) would be very easy to verify. Thanks, -Stolee
