Re: [PATCH v2 2/2] clean: refector to the interactive part of clean

[Derrick Stolee <derrickstolee@github.com>]

On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:
> From: Nadav Goldstein <nadav.goldstein96@gmail.com>
> 
> 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