git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Derrick Stolee <derrickstolee@github.com>
Date
May 23, 2022, 19:45 UTC
Message-ID
<a28047a1-4db2-1638-8b7e-62fcf0b57983@github.com>
In-Reply-To
<7271a285d18604bd77251d586201d31eba1287da.1653286345.git.gitgitgadget@gmail.com>
On 5/23/22 2:12 AM, Nadav Goldstein via GitGitGadget wrote:
Show 5 quoted lines
> 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
Show 13 quoted lines
> -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'.

Show 9 quoted lines
> \ 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'.
Show 20 quoted lines
> +
>  #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.

Show 24 quoted lines
> 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.

Show 25 quoted lines
>  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

Previous: Nadav Goldstein via GitGitGadgetNext: Derrick Stolee
Message 8 of 15 in “stash: added safety flag for stash clear subcommand”
  1. stash: added safety flag for stash clear subcommandNadav Goldstein via GitGitGadget, May 15, 2022
  2. Junio C HamanoMay 16, 2022
  3. 0/2 stash clear: added safety flag for stash clear subcommandNadav Goldstein via GitGitGadget, May 23, 2022
  4. 1/2 add-menu: added add-menu to lib objectsNadav Goldstein via GitGitGadget, May 23, 2022
  5. Derrick StoleeMay 23, 2022
  6. Junio C HamanoMay 23, 2022
  7. 2/2 clean: refector to the interactive part of cleanNadav Goldstein via GitGitGadget, May 23, 2022
  8. Derrick StoleeMay 23, 2022
  9. Derrick StoleeMay 23, 2022
  10. Introduced force flag to the git stash clear subcommand.Nadav Goldstein via GitGitGadget, Jun 20, 2023
  11. Junio C HamanoJun 20, 2023
  12. Nadav GoldsteinJun 20, 2023
  13. Junio C HamanoJun 20, 2023
  14. Eric SunshineJun 20, 2023
  15. Nadav GoldsteinJun 20, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.