[2/8] rm: support the --pathspec-from-file option
diff mbox series

Message ID 5611e3ae326bb7f61abf870e3b2851226b6af1d8.1579190965.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Support --pathspec-from-file in rm, stash
Related show

Commit Message

Elijah Newren via GitGitGadget Jan. 16, 2020, 4:09 p.m. UTC
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

Decisions taken for simplicity:
1) It is not allowed to pass pathspec in both args and file.

`if (!argc)` block was adapted to work with --pathspec-from-file. For
that, I also had to parse pathspec earlier. Now it happens before
`read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
sounds fine to me.

In case of empty pathspec, there is now a clear error message instead
of showing usage. As a consequence, exit code has also changed. Judging
from [1] it doesn't seem that showing usage in this case was important
(according to commit message, it was to avoid segfault), and it doesn't
fit into how other commands react to empty pathspec. Finally, the new
error message is easier to understand.

[1] Commit 7612a1ef ("git-rm: honor -n flag" 2006-06-09)

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 Documentation/git-rm.txt    | 17 +++++++-
 builtin/rm.c                | 28 ++++++++++---
 t/t3601-rm-pathspec-file.sh | 79 +++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+), 7 deletions(-)
 create mode 100755 t/t3601-rm-pathspec-file.sh

Comments

Junio C Hamano Jan. 21, 2020, 7:36 p.m. UTC | #1
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> Decisions taken for simplicity:
> 1) It is not allowed to pass pathspec in both args and file.
>
> `if (!argc)` block was adapted to work with --pathspec-from-file. For
> that, I also had to parse pathspec earlier. Now it happens before
> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
> sounds fine to me.

That is not an explanation nor justification.

> In case of empty pathspec, there is now a clear error message instead
> of showing usage.

Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
specified, nothing removed" and it makes perfect sense, but I am not
sure "git rm" that gives the same message is better than the output
by usage_with_options(builtin_rm_usage, builtin_rm_options).

> -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
> +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> +	  [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> +	  [--] [<pathspec>...]

OK.

> +--pathspec-from-file=<file>::
> +	Pathspec is passed in `<file>` instead of commandline args. If
> +	`<file>` is exactly `-` then standard input is used. Pathspec
> +	elements are separated by LF or CR/LF. Pathspec elements can be
> +	quoted as explained for the configuration variable `core.quotePath`
> +	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
> +	global `--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> +	separated with NUL character and all other characters are taken
> +	literally (including newlines and quotes).
> +

OK.

> diff --git a/builtin/rm.c b/builtin/rm.c
> index 19ce95a901..8e40795751 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -235,7 +235,8 @@ static int check_local_mod(struct object_id *head, int index_only)
>  }
>  
>  static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
> -static int ignore_unmatch = 0;
> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
> +static char *pathspec_from_file = NULL;

We may want to clean these "explicitly initialize to 0/NULL" up at
some point.  The clean-up itself would not be in the scope of this
patch, of course, but not making it worse is something this patch
can do to help.

> @@ -259,8 +262,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, builtin_rm_options,
>  			     builtin_rm_usage, 0);
> -	if (!argc)
> -		usage_with_options(builtin_rm_usage, builtin_rm_options);
> +
> +	parse_pathspec(&pathspec, 0,
> +		       PATHSPEC_PREFER_CWD,
> +		       prefix, argv);
> +
> +	if (pathspec_from_file) {
> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
> +
> +		parse_pathspec_file(&pathspec, 0,
> +				    PATHSPEC_PREFER_CWD,
> +				    prefix, pathspec_from_file, pathspec_file_nul);
> +	} else if (pathspec_file_nul) {
> +		die(_("--pathspec-file-nul requires --pathspec-from-file"));
> +	}
> +
> +	if (!pathspec.nr)
> +		die(_("Nothing specified, nothing removed"));

I wonder if doing these in this order instead would make more sense
without making unnecessary behaviour change.

    - parse the options (which would make pathspec_f_f available to
      us)

    - if pathspec_f_f is given, call parse_pathspec_file()

    - otherwise complain if pathspec_file_nul is set

    - otherwise check argc and give the usage_with_options()

I dunno.

Thanks.
Alexandr Miloslavskiy Feb. 10, 2020, 2:46 p.m. UTC | #2
Sorry for late reply, I was on vacation. Now I'm back and ready to 
continue :)

Thanks for your review!

On 21.01.2020 20:36, Junio C Hamano wrote:
>> Decisions taken for simplicity:
>> 1) It is not allowed to pass pathspec in both args and file.
>>
>> `if (!argc)` block was adapted to work with --pathspec-from-file. For
>> that, I also had to parse pathspec earlier. Now it happens before
>> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
>> sounds fine to me.
> 
> That is not an explanation nor justification.

I'm not exactly sure what are you suggesting. My best guess is that you 
want to remove "`if (!argc)` block was adapted" paragraph from commit 
message? I thought about it and it feels wrong to leave this change 
unexplained. Or are you suggesting to reword it? If so, please give a hint.

>> In case of empty pathspec, there is now a clear error message instead
>> of showing usage.
> 
> Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
> specified, nothing removed" and it makes perfect sense, but I am not
> sure "git rm" that gives the same message is better than the output
> by usage_with_options(builtin_rm_usage, builtin_rm_options).

What feels wrong to me is when I make a mistake and git just slams me 
with usage, and then it's up to me to figure what could be wrong. I 
myself struggled to find a mistake a couple times (in similar cases, not 
in this specific one) and didn't like the experience.

This could be a lot worse when there's no mistake, just the file was 
empty - but you already agreed that showing a new error message is 
reasonable with '--pathspec-from-file'.

Still, without '--pathspec-from-file', it should still be better to 
point to a specific error rather then "here's usage and try to find a 
difference". I have reworded the error message in V2 in hopes that it 
will be less controversial.

If you still don't like it, I will change it to only show the new error 
with '--pathspec-from-file'.

>> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
>> +static char *pathspec_from_file = NULL;
> 
> We may want to clean these "explicitly initialize to 0/NULL" up at
> some point.  The clean-up itself would not be in the scope of this
> patch, of course, but not making it worse is something this patch
> can do to help.

Changed in V2.
Junio C Hamano Feb. 10, 2020, 6:48 p.m. UTC | #3
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:

> Sorry for late reply, I was on vacation. Now I'm back and ready to
> continue :)
>
> Thanks for your review!
>
> On 21.01.2020 20:36, Junio C Hamano wrote:
>>> Decisions taken for simplicity:
>>> 1) It is not allowed to pass pathspec in both args and file.
>>>
>>> `if (!argc)` block was adapted to work with --pathspec-from-file. For
>>> that, I also had to parse pathspec earlier. Now it happens before
>>> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
>>> sounds fine to me.
>>
>> That is not an explanation nor justification.
>
> I'm not exactly sure what are you suggesting.

I expected that the proposed log message to explain and justify why
a change (in behaviour, in design, etc.) is made.  "There is this
limitation" is not a justification---"because of such and such
reasons, there is this limitation, otherwise such and such bad
things happen" is.
Alexandr Miloslavskiy Feb. 17, 2020, 5:25 p.m. UTC | #4
On 10.02.2020 19:48, Junio C Hamano wrote:
> I expected that the proposed log message to explain and justify why
> a change (in behaviour, in design, etc.) is made.  "There is this
> limitation" is not a justification---"because of such and such
> reasons, there is this limitation, otherwise such and such bad
> things happen" is.

I have rewritten the commit message in V3. Is that better?

Patch
diff mbox series

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index e02a08e5ef..ab750367fd 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -8,7 +8,9 @@  git-rm - Remove files from the working tree and from the index
 SYNOPSIS
 --------
 [verse]
-'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
+'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
+	  [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
+	  [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -73,6 +75,19 @@  For more details, see the 'pathspec' entry in linkgit:gitglossary[7].
 	`git rm` normally outputs one line (in the form of an `rm` command)
 	for each file removed. This option suppresses that output.
 
+--pathspec-from-file=<file>::
+	Pathspec is passed in `<file>` instead of commandline args. If
+	`<file>` is exactly `-` then standard input is used. Pathspec
+	elements are separated by LF or CR/LF. Pathspec elements can be
+	quoted as explained for the configuration variable `core.quotePath`
+	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
+	global `--literal-pathspecs`.
+
+--pathspec-file-nul::
+	Only meaningful with `--pathspec-from-file`. Pathspec elements are
+	separated with NUL character and all other characters are taken
+	literally (including newlines and quotes).
+
 
 REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM
 --------------------------------------------------------
diff --git a/builtin/rm.c b/builtin/rm.c
index 19ce95a901..8e40795751 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -235,7 +235,8 @@  static int check_local_mod(struct object_id *head, int index_only)
 }
 
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
-static int ignore_unmatch = 0;
+static int ignore_unmatch = 0, pathspec_file_nul = 0;
+static char *pathspec_from_file = NULL;
 
 static struct option builtin_rm_options[] = {
 	OPT__DRY_RUN(&show_only, N_("dry run")),
@@ -245,6 +246,8 @@  static struct option builtin_rm_options[] = {
 	OPT_BOOL('r', NULL,             &recursive,  N_("allow recursive removal")),
 	OPT_BOOL( 0 , "ignore-unmatch", &ignore_unmatch,
 				N_("exit with a zero status even if nothing matched")),
+	OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+	OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
 	OPT_END(),
 };
 
@@ -259,8 +262,24 @@  int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
 			     builtin_rm_usage, 0);
-	if (!argc)
-		usage_with_options(builtin_rm_usage, builtin_rm_options);
+
+	parse_pathspec(&pathspec, 0,
+		       PATHSPEC_PREFER_CWD,
+		       prefix, argv);
+
+	if (pathspec_from_file) {
+		if (pathspec.nr)
+			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
+
+		parse_pathspec_file(&pathspec, 0,
+				    PATHSPEC_PREFER_CWD,
+				    prefix, pathspec_from_file, pathspec_file_nul);
+	} else if (pathspec_file_nul) {
+		die(_("--pathspec-file-nul requires --pathspec-from-file"));
+	}
+
+	if (!pathspec.nr)
+		die(_("Nothing specified, nothing removed"));
 
 	if (!index_only)
 		setup_work_tree();
@@ -270,9 +289,6 @@  int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
-	parse_pathspec(&pathspec, 0,
-		       PATHSPEC_PREFER_CWD,
-		       prefix, argv);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL);
 
 	seen = xcalloc(pathspec.nr, 1);
diff --git a/t/t3601-rm-pathspec-file.sh b/t/t3601-rm-pathspec-file.sh
new file mode 100755
index 0000000000..0f69ae8478
--- /dev/null
+++ b/t/t3601-rm-pathspec-file.sh
@@ -0,0 +1,79 @@ 
+#!/bin/sh
+
+test_description='rm --pathspec-from-file'
+
+. ./test-lib.sh
+
+test_tick
+
+test_expect_success setup '
+	echo A >fileA.t &&
+	echo B >fileB.t &&
+	echo C >fileC.t &&
+	echo D >fileD.t &&
+	git add fileA.t fileB.t fileC.t fileD.t &&
+	git commit -m "files" &&
+	
+	git tag checkpoint
+'
+
+restore_checkpoint () {
+	git reset --hard checkpoint
+}
+
+verify_expect () {
+	git status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'simplest' '
+	restore_checkpoint &&
+
+	cat >expect <<-\EOF &&
+	D  fileA.t
+	EOF
+
+	echo fileA.t | git rm --pathspec-from-file=- &&
+	verify_expect
+'
+
+test_expect_success '--pathspec-file-nul' '
+	restore_checkpoint &&
+
+	cat >expect <<-\EOF &&
+	D  fileA.t
+	D  fileB.t
+	EOF
+
+	printf "fileA.t\0fileB.t\0" | git rm --pathspec-from-file=- --pathspec-file-nul &&
+	verify_expect
+'
+
+test_expect_success 'only touches what was listed' '
+	restore_checkpoint &&
+
+	cat >expect <<-\EOF &&
+	D  fileB.t
+	D  fileC.t
+	EOF
+
+	printf "fileB.t\nfileC.t\n" | git rm --pathspec-from-file=- &&
+	verify_expect
+'
+
+test_expect_success 'error conditions' '
+	restore_checkpoint &&
+	echo fileA.t >list &&
+
+	test_must_fail git rm --pathspec-from-file=list -- fileA.t 2>err &&
+	test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
+
+	test_must_fail git rm --pathspec-file-nul 2>err &&
+	test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
+	
+	>empty_list &&
+	test_must_fail git rm --pathspec-from-file=empty_list 2>err &&
+	test_i18ngrep -e "Nothing specified, nothing removed" err
+'
+
+test_done