Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: [PATCH 2/8] rm: support the --pathspec-from-file option
Date: Thu, 16 Jan 2020 16:09:19 +0000
Message-ID: <5611e3ae326bb7f61abf870e3b2851226b6af1d8.1579190965.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.530.git.1579190965.gitgitgadget@gmail.com>

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

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
-- 
gitgitgadget


  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 16:09 [PATCH 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` Alexandr Miloslavskiy via GitGitGadget [this message]
2020-01-16 16:09 ` [PATCH 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5611e3ae326bb7f61abf870e3b2851226b6af1d8.1579190965.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=alexandr.miloslavskiy@syntevo.com \
    --cc=git@vger.kernel.org \
    --cc=ungureanupaulsebastian@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git