diff mbox series

checkout, restore: make pathspec recursive

Message ID 7c5144d8-24cd-0d50-a7c6-ff77160d503d@web.de (mailing list archive)
State New, archived
Headers show
Series checkout, restore: make pathspec recursive | expand

Commit Message

René Scharfe Aug. 22, 2020, 8:57 a.m. UTC 
The pathspec given to git checkout and git restore is used with both
tree_entry_interesting (via read_tree_recursive) and match_pathspec
(via ce_path_match).  The latter effectively only supports recursive
matching regardless of the value of the pathspec flag "recursive",
which is unset here.

That causes different match results for pathspecs with wildcards, and
can lead checkout and restore in no-overlay mode to remove entries
instead of modifying them.  Enable recursive matching for both checkout
and restore to make matching consistent.

Setting the flag in checkout_main() technically also affects git switch,
but since that command doesn't accept pathspecs at all this has no
actual consequence.

Reported-by: Sergii Shkarnikov <sergii.shkarnikov@globallogic.com>
Initial-test-by: Sergii Shkarnikov <sergii.shkarnikov@globallogic.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/checkout.c               |  2 ++
 t/t2025-checkout-no-overlay.sh   | 12 ++++++++++++
 t/t2072-restore-pathspec-file.sh | 19 ++++++++++++++++---
 3 files changed, 30 insertions(+), 3 deletions(-)

--
2.28.0

Comments

Jeff King Aug. 24, 2020, 8:21 p.m. UTC | #1 
On Sat, Aug 22, 2020 at 10:57:59AM +0200, René Scharfe wrote:

> The pathspec given to git checkout and git restore is used with both
> tree_entry_interesting (via read_tree_recursive) and match_pathspec
> (via ce_path_match).  The latter effectively only supports recursive
> matching regardless of the value of the pathspec flag "recursive",
> which is unset here.
> 
> That causes different match results for pathspecs with wildcards, and
> can lead checkout and restore in no-overlay mode to remove entries
> instead of modifying them.  Enable recursive matching for both checkout
> and restore to make matching consistent.
> 
> Setting the flag in checkout_main() technically also affects git switch,
> but since that command doesn't accept pathspecs at all this has no
> actual consequence.

Thanks, I think this is the thing to do (and the code change looks
obviously correct ;) ).

This may cause a user-visible behavior change even in overlay mode
(because we'll now match some tree entries that we wouldn't before). But
I think it's an improvement, because it makes:

  git checkout <tree> -- <paths>

consistent with:

  git checkout -- <paths>

I don't know if you want to call that out more directly in the commit
message.

-Peff
diff mbox series

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 28371954912..a65b4bf0b71 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1707,6 +1707,8 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
 	}

+	opts->pathspec.recursive = 1;
+
 	if (opts->pathspec.nr) {
 		if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
index 76330cb5ab7..fa9e0987063 100755
--- a/t/t2025-checkout-no-overlay.sh
+++ b/t/t2025-checkout-no-overlay.sh
@@ -44,4 +44,16 @@  test_expect_success '--no-overlay --theirs with D/F conflict deletes file' '
 	test_path_is_missing file1
 '

+test_expect_success 'wildcard pathspec matches file in subdirectory' '
+	git reset --hard &&
+	mkdir subdir &&
+	test_commit file3-1 subdir/file3 &&
+	test_commit file3-2 subdir/file3 &&
+
+	git checkout --no-overlay file3-1 "*file3" &&
+	echo file3-1 >expect &&
+	test_path_is_file subdir/file3 &&
+	test_cmp expect subdir/file3
+'
+
 test_done
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index 0d47946e8a9..b48345bf95f 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -9,18 +9,21 @@  test_tick
 test_expect_success setup '
 	test_commit file0 &&

+	mkdir dir1 &&
+	echo 1 >dir1/file &&
 	echo 1 >fileA.t &&
 	echo 1 >fileB.t &&
 	echo 1 >fileC.t &&
 	echo 1 >fileD.t &&
-	git add fileA.t fileB.t fileC.t fileD.t &&
+	git add dir1 fileA.t fileB.t fileC.t fileD.t &&
 	git commit -m "files 1" &&

+	echo 2 >dir1/file &&
 	echo 2 >fileA.t &&
 	echo 2 >fileB.t &&
 	echo 2 >fileC.t &&
 	echo 2 >fileD.t &&
-	git add fileA.t fileB.t fileC.t fileD.t &&
+	git add dir1 fileA.t fileB.t fileC.t fileD.t &&
 	git commit -m "files 2" &&

 	git tag checkpoint
@@ -31,7 +34,7 @@  restore_checkpoint () {
 }

 verify_expect () {
-	git status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual &&
+	git status --porcelain --untracked-files=no -- dir1 fileA.t fileB.t fileC.t fileD.t >actual &&
 	test_cmp expect actual
 }

@@ -161,4 +164,14 @@  test_expect_success 'error conditions' '
 	test_i18ngrep -e "you must specify path(s) to restore" err
 '

+test_expect_success 'wildcard pathspec matches file in subdirectory' '
+	restore_checkpoint &&
+
+	echo "*file" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+	cat >expect <<-\EOF &&
+	 M dir1/file
+	EOF
+	verify_expect
+'
+
 test_done