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

Re: [PATCH v7 01/10] Add support for -i/--interactive to git-clean

From
Junio C Hamano <gitster@pobox.com>
Date
May 8, 2013, 16:57 UTC
Message-ID
<7vr4hhflym.fsf@alter.siamese.dyndns.org>
In-Reply-To
<ad26375dece854339d64dcb82c17f19f8edccf48.1368011946.git.worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
Show 18 quoted lines
> Show what would be done and the user must confirm before actually
> cleaning. In the confirmation dialog, the user has three choices:
>
>  * y/yes:  Start to do cleaning.
>  * n/no:   Nothing will be deleted.
>  * e/edit: Exclude items from deletion using ignore patterns.
>
> When the user chooses the edit mode, the user can input space-
> separated patterns (the same syntax as gitignore), and each clean
> candidate that matches with one of the patterns will be excluded
> from cleaning. When the user feels it's OK, presses ENTER and back
> to the confirmation dialog.
>
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Spelling-checked-by: Eric Sunshine <sunshine@sunshineco.com>
> Comments-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>

Listing everybody who has ever said anything in the review thread? I can understand that you may want to give credit to those who significantly helped, but please do not overdo it.

In any case, with the help of their inputs, you brought the patch into its final shape. Please sign-off at the _end_.

Show 49 quoted lines
> ---
>  Documentation/git-clean.txt |  15 +++-
>  builtin/clean.c             | 198 +++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 192 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> index bdc3a..f5572 100644
> --- a/Documentation/git-clean.txt
> +++ b/Documentation/git-clean.txt
> @@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree
>  SYNOPSIS
>  --------
>  [verse]
> -'git clean' [-d] [-f] [-n] [-q] [-e <pattern>] [-x | -X] [--] <path>...
> +'git clean' [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [-x | -X] [--] <path>...
>  
>  DESCRIPTION
>  -----------
> @@ -34,7 +34,18 @@ OPTIONS
>  -f::
>  --force::
>  	If the Git configuration variable clean.requireForce is not set
> -	to false, 'git clean' will refuse to run unless given -f or -n.
> +	to false, 'git clean' will refuse to run unless given -f, -n or
> +	-i.
> +
> +-i::
> +--interactive::
> +	Show what would be done and the user must confirm before actually
> +	cleaning. In the confirmation dialog, the user can choose to abort
> +	the cleaning, or enter into an edit mode. In the edit mode, the
> +	user can input space-separated patterns (the same syntax as
> +	gitignore), and each clean candidate that matches with one of the
> +	patterns will be excluded from cleaning. When the user feels it's
> +	OK, presses ENTER and back to the confirmation dialog.
>  
>  -n::
>  --dry-run::
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 04e39..49aab 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -15,9 +15,12 @@
>  #include "quote.h"
>  
>  static int force = -1; /* unset */
> +static int interactive;
> +static struct string_list del_list = STRING_LIST_INIT_DUP;
> +static const char **the_prefix;
Ehh, why?
Show 11 quoted lines
>  static const char *const builtin_clean_usage[] = {
> -	N_("git clean [-d] [-f] [-n] [-q] [-e <pattern>] [-x | -X] [--] <paths>..."),
> +	N_("git clean [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [-x | -X] [--] <paths>..."),
>  	NULL
>  };
>  
> @@ -142,6 +145,139 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  	return ret;
>  }
>  
> +static void edit_by_patterns_cmd()
static void edit_by_patterns_cmd(void)
Show 12 quoted lines
> +{
> +	struct dir_struct dir;
> +	struct strbuf confirm = STRBUF_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf **ignore_list;
> +	struct string_list_item *item;
> +	struct exclude_list *el;
> +	const char *qname;
> +	int changed = -1, i;
> +
> +	while (1) {
> +		/* dels list may become empty when we run string_list_remove_empty_items later */

An unnecessary and overlong comment. The message shown already tells the reader what is going on anyway, no?

Show 22 quoted lines
> +		if (!del_list.nr) {
> +			printf_ln(_("No more files to clean, exiting."));
> +			break;
> +		}
> +
> +		if (changed) {
> +			putchar('\n');
> +
> +			/* Display dels in "Would remove ..." format */
> +			for_each_string_list_item(item, &del_list) {
> +				qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
> +				printf(_(msg_would_remove), qname);
> +			}
> +			putchar('\n');
> +		}
> +
> +		printf(_("Input ignore patterns>> "));
> +		if (strbuf_getline(&confirm, stdin, '\n') != EOF) {
> +			strbuf_trim(&confirm);
> +		} else {
> +			putchar('\n');
> +			break;

Why break here? If we got nothing, wouldn't confirm.len be zero? If we did get something but the input got flushed without line-end, sending '\n' to the terminal may be justified, but in that case you would may have something useful, and asking confirm.len if it is empty would be the consistent way to check between two cases, no?

Show 29 quoted lines
> +		}
> +
> +		/* Quit edit mode */
> +		if (!confirm.len)
> +			break;
> +
> +		memset(&dir, 0, sizeof(dir));
> +		el = add_exclude_list(&dir, EXC_CMDL, "manual exclude");
> +		ignore_list = strbuf_split_max(&confirm, ' ', 0);
> +
> +		for (i = 0; ignore_list[i]; i++) {
> +			strbuf_trim(ignore_list[i]);
> +			if (!ignore_list[i]->len)
> +				continue;
> +
> +			add_exclude(ignore_list[i]->buf, "", 0, el, -(i+1));
> +		}
> +
> +		changed = 0;
> +		for_each_string_list_item(item, &del_list) {
> +			int dtype = DT_UNKNOWN;
> +			const char *qname;
> +
> +			qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
> +
> +			if (is_excluded(&dir, qname, &dtype)) {
> +				*item->string = '\0';
> +				changed++;
> +			}
A few points:
 * Pass prefix as a parameter to this function, just like how
   remove_dirs() gets called, and get rid of the_prefix.
 * The result of quote_* is designed to avoid ambiguities, by
   applying C-style quotes like HT => \t and adding "" pair around
   it as necessary.  I doubt feeding it to is_excluded() makes any
   sense.  You probably meant path_relative(), but I am not sure.
Show 95 quoted lines
> +		}
> +
> +		if (changed) {
> +			string_list_remove_empty_items(&del_list, 0);
> +		} else {
> +			printf_ln(_("WARNING: Cannot find items matched by: %s"), confirm.buf);
> +		}
> +
> +		strbuf_list_free(ignore_list);
> +		clear_directory(&dir);
> +	}
> +
> +	strbuf_release(&buf);
> +	strbuf_release(&confirm);
> +}
> +
> +static void interactive_main_loop()
> +{
> +	struct strbuf confirm = STRBUF_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct string_list_item *item;
> +	const char *qname;
> +
> +	/* dels list may become empty after return back from edit mode */
> +	while (del_list.nr) {
> +		printf_ln(Q_("Would remove the following item:",
> +			     "Would remove the following items:",
> +			     del_list.nr));
> +		putchar('\n');
> +
> +		/* Display dels in "Would remove ..." format */
> +		for_each_string_list_item(item, &del_list) {
> +			qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
> +			printf(_(msg_would_remove), qname);
> +		}
> +		putchar('\n');
> +
> +		/* Confirmation dialog */
> +		printf(_("Remove ([y]es/[n]o/[e]dit) ? "));
> +		if (strbuf_getline(&confirm, stdin, '\n') != EOF) {
> +			strbuf_trim(&confirm);
> +		} else {
> +			/* Ctrl-D is the same as "quit" */
> +			string_list_clear(&del_list, 0);
> +			putchar('\n');
> +			printf_ln("Bye.");
> +			break;
> +		}
> +
> +		if (confirm.len) {
> +			if (!strncasecmp(confirm.buf, "yes", confirm.len)) {
> +				break;
> +			} else if (!strncasecmp(confirm.buf, "no", confirm.len) ||
> +				   !strncasecmp(confirm.buf, "quit", confirm.len)) {
> +				string_list_clear(&del_list, 0);
> +				printf_ln("Bye.");
> +				break;
> +			} else if (!strncasecmp(confirm.buf, "edit", confirm.len)) {
> +				edit_by_patterns_cmd();
> +			} else {
> +				continue;
> +			}
> +		}
> +	}
> +
> +	strbuf_release(&buf);
> +	strbuf_release(&confirm);
> +}
> +
>  int cmd_clean(int argc, const char **argv, const char *prefix)
>  {
>  	int i, res;
> @@ -154,12 +290,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	struct strbuf buf = STRBUF_INIT;
>  	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
>  	struct exclude_list *el;
> +	struct string_list_item *item;
>  	const char *qname;
>  	char *seen = NULL;
>  	struct option options[] = {
>  		OPT__QUIET(&quiet, N_("do not print names of files removed")),
>  		OPT__DRY_RUN(&dry_run, N_("dry run")),
>  		OPT__FORCE(&force, N_("force")),
> +		OPT_BOOL('i', "interactive", &interactive, N_("interactive cleaning")),
>  		OPT_BOOLEAN('d', NULL, &remove_directories,
>  				N_("remove whole directories")),
>  		{ OPTION_CALLBACK, 'e', "exclude", &exclude_list, N_("pattern"),
> @@ -176,7 +314,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	else
>  		config_set = 1;
>  
> -	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
> +	the_prefix = &prefix;
> +	argc = parse_options(argc, argv, *the_prefix, options, builtin_clean_usage,
>  			     0);

None of these changes s/prefix/*the_prefix/ looks remotely justifiable. Does anybody change *the_prefix after it is set and if so how?

Show 62 quoted lines
>  	memset(&dir, 0, sizeof(dir));
> @@ -186,12 +326,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	if (ignored && ignored_only)
>  		die(_("-x and -X cannot be used together"));
>  
> -	if (!dry_run && !force) {
> +	if (interactive) {
> +		if (!isatty(0) || !isatty(1))
> +			die(_("interactive clean can not run without a valid tty; "
> +				  "refusing to clean"));
> +	} else if (!dry_run && !force) {
>  		if (config_set)
> -			die(_("clean.requireForce set to true and neither -n nor -f given; "
> +			die(_("clean.requireForce set to true and neither -i, -n nor -f given; "
>  				  "refusing to clean"));
>  		else
> -			die(_("clean.requireForce defaults to true and neither -n nor -f given; "
> +			die(_("clean.requireForce defaults to true and neither -i, -n nor -f given; "
>  				  "refusing to clean"));
>  	}
>  
> @@ -210,7 +354,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	for (i = 0; i < exclude_list.nr; i++)
>  		add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
>  
> -	pathspec = get_pathspec(prefix, argv);
> +	pathspec = get_pathspec(*the_prefix, argv);
>  
>  	fill_directory(&dir, pathspec);
>  
> @@ -257,26 +401,41 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  		}
>  
>  		if (S_ISDIR(st.st_mode)) {
> -			strbuf_addstr(&directory, ent->name);
> -			if (remove_directories || (matches == MATCHED_EXACTLY)) {
> -				if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
> -					errors++;
> -				if (gone && !quiet) {
> -					qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
> -					printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
> -				}
> -			}
> -			strbuf_reset(&directory);
> +			if (remove_directories || (matches == MATCHED_EXACTLY))
> +				string_list_append(&del_list, ent->name);
>  		} else {
>  			if (pathspec && !matches)
>  				continue;
> -			res = dry_run ? 0 : unlink(ent->name);
> +			string_list_append(&del_list, ent->name);
> +		}
> +	}
> +
> +	if (interactive && !dry_run && del_list.nr > 0)
> +		interactive_main_loop();
> +
> +	for_each_string_list_item(item, &del_list) {
> +		struct stat st;
> +
> +		if (lstat(item->string, &st))
> +			continue;
Ignoring errors silently?

With the "interactive" stuff, can you get into a situation where you originally propose to remove D and D/F but the user tells you to remove D (editing D/F away), or vice versa?

I think this patch should be in at least two parts:
 - Introduce the two-phase "collect in del_list, remove in a
   separate loop at the end" restructuring.
 - (optional, if you are feeling ambitious) Change the path that is
   stored in del_list relative to the prefix, so that all functions
   that operate on the string in the del_list do not have to do
   *_relative() thing.  Some functions may instead have to prepend
   prefix but if they are minority compared to the users of
   *_relative(), it may be an overall win from the readability's
   point of view.
 - Add the "interactively allow you to reduce the del_list" bit
   between the two phases.
Show 29 quoted lines
> +		if (S_ISDIR(st.st_mode)) {
> +			strbuf_addstr(&directory, item->string);
> +			if (remove_dirs(&directory, *the_prefix, rm_flags, dry_run, quiet, &gone))
> +				errors++;
> +			if (gone && !quiet) {
> +				qname = quote_path_relative(directory.buf, directory.len, &buf, *the_prefix);
> +				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
> +			}
> +			strbuf_reset(&directory);
> +		} else {
> +			res = dry_run ? 0 : unlink(item->string);
>  			if (res) {
> -				qname = quote_path_relative(ent->name, -1, &buf, prefix);
> +				qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
>  				warning(_(msg_warn_remove_failed), qname);
>  				errors++;
>  			} else if (!quiet) {
> -				qname = quote_path_relative(ent->name, -1, &buf, prefix);
> +				qname = quote_path_relative(item->string, -1, &buf, *the_prefix);
>  				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
>  			}
>  		}
> @@ -285,5 +444,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  
>  	strbuf_release(&directory);
>  	string_list_clear(&exclude_list, 0);
> +	string_list_clear(&del_list, 0);
>  	return (errors != 0);
>  }
Previous: Matthieu MoyNext: Jiang Xin
Message 19 of 41 in “interactive git clean”
  1. 00/10 interactive git cleanJiang Xin, May 8, 2013
  2. 01/10 Add support for -i/--interactive to git-cleanJiang Xin, May 8, 2013
  3. 02/10 Show items of interactive git-clean in columnsJiang Xin, May 8, 2013
  4. 03/10 Add colors to interactive git-cleanJiang Xin, May 8, 2013
  5. 04/10 git-clean: use a git-add-interactive compatible UIJiang Xin, May 8, 2013
  6. 05/10 git-clean: interactive cleaning by select numbersJiang Xin, May 8, 2013
  7. 06/10 git-clean: rm -i style interactive cleaningJiang Xin, May 8, 2013
  8. 07/10 git-clean: update document for interactive git-cleanJiang Xin, May 8, 2013
  9. 08/10 git-clean refactor: save some options in clean_flagsJiang Xin, May 8, 2013
  10. 09/10 git-clean refactor: wrap in scan_clean_candidatesJiang Xin, May 8, 2013
  11. 10/10 git-clean: change clean flags in interactive modeJiang Xin, May 8, 2013
  12. Junio C HamanoMay 8, 2013
  13. Matthieu MoyMay 12, 2013
  14. Jiang XinMay 13, 2013
  15. Junio C HamanoMay 13, 2013
  16. Matthieu MoyMay 13, 2013
  17. Junio C HamanoMay 8, 2013
  18. Matthieu MoyMay 12, 2013
  19. Junio C HamanoMay 8, 2013
  20. Jiang XinMay 9, 2013
  21. 00/12 Interactive git cleanJiang Xin, May 9, 2013
  22. 01/12 git-clean refactor: hold cleaning items in del_listJiang Xin, May 9, 2013
  23. 02/12 git-clean: add support for -i/--interactiveJiang Xin, May 9, 2013
  24. 03/12 git-clean: show items of del_list in columnsJiang Xin, May 9, 2013
  25. 04/12 git-clean: add colors to interactive git-cleanJiang Xin, May 9, 2013
  26. 05/12 git-clean: use a git-add-interactive compatible UIJiang Xin, May 9, 2013
  27. 06/12 git-clean: add filter by pattern interactive actionJiang Xin, May 9, 2013
  28. 07/12 git-clean: add select by numbers interactive actionJiang Xin, May 9, 2013
  29. 08/12 git-clean: add ask each interactive actionJiang Xin, May 9, 2013
  30. 09/12 git-clean refactor: save some options in clean_flagsJiang Xin, May 9, 2013
  31. 10/12 git-clean refactor: add wrapper scan_clean_candidatesJiang Xin, May 9, 2013
  32. 11/12 git-clean: add toggle flags interactive actionJiang Xin, May 9, 2013
  33. 12/12 git-clean: update document for interactive git-cleanJiang Xin, May 9, 2013
  34. Matthieu MoyMay 12, 2013
  35. Jiang XinMay 13, 2013
  36. Eric SunshineMay 8, 2013
  37. Eric SunshineMay 8, 2013
  38. Junio C HamanoMay 8, 2013
  39. Matthieu MoyMay 12, 2013
  40. Jiang XinMay 13, 2013
  41. Matthieu MoyMay 13, 2013

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.