{"thread":{"id":"54029","subject":"Possible bug with git restore","startedAt":"2020-08-12T18:51:37Z","lastAt":"2020-08-24T20:25:58Z","messageCount":11,"participants":["Sergii Shkarnikov","Eric Sunshine","Jeff King","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"403485","messageId":"CAFvH=vsWyX79j-9pyC5gpxGu8rRxYyrXwywHjh-_T2opHjT8Xg@mail.gmail.com","threadId":"54029","inReplyTo":null,"subject":"Possible bug with git restore","fromName":"Sergii Shkarnikov","fromEmail":"sergii.shkarnikov@globallogic.com","sentAt":"2020-08-12T18:51:23Z","receivedAt":"2020-08-12T18:51:37Z","isPatch":false,"sender":{"key":"sergii.shkarnikov@globallogic.com","avatar":null},"body":"Hi.\n\nI see some strange behavior of the \"git restore\" command. Here is a\nbug report template filled out:\n\n*********************************************************************\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n    I tried to restore a couple of files from an earlier commit\nrunning the restore command with a wildcard:\n\n    git restore -s HEAD~ -- */filename.*\n\n    In my work tree those are .cpp and .hpp files stored in different folders.\n\nWhat did you expect to happen? (Expected behavior)\n\n    Both files are restored to what they were in HEAD~.\n\nWhat happened instead? (Actual behavior)\n\n    Both files were deleted (and got (delete) status).\n\nWhat's different between what you expected and what actually happened?\n\n    Files became deleted for no apparent reason instead of restoring\nto expected revision.\n\nAnything else you want to add:\n\n    Running this command without wildcards for each file separately\nworks as expected.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.27.0.windows.1\ncpu: x86_64\nbuilt from commit: 907ab1011dce9112700498e034b974ba60f8b407\nsizeof-long: 4\nsizeof-size_t: 8\nuname: Windows 10.0 18363\ncompiler info: gnuc: 10.1\nlibc info: no libc information available\n\n\n[Enabled Hooks]\npost-commit\npost-checkout\npost-merge\npre-push\n***************************************************************\nRegards,\nSergii Shkarnikov\n"},{"id":"403730","messageId":"CAPig+cSCd_8YB90sypTe1bHMQhPgo+Tr2PHNucdqfCpEe+Dosg@mail.gmail.com","threadId":"54029","inReplyTo":"CAFvH=vsWyX79j-9pyC5gpxGu8rRxYyrXwywHjh-_T2opHjT8Xg@mail.gmail.com","subject":"Re: Possible bug with git restore","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-14T22:41:51Z","receivedAt":"2020-08-14T22:42:05Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2020 at 2:51 PM Sergii Shkarnikov\n<sergii.shkarnikov@globallogic.com> wrote:\n>     I tried to restore a couple of files from an earlier commit\n> running the restore command with a wildcard:\n>\n>     git restore -s HEAD~ -- */filename.*\n>\n>     In my work tree those are .cpp and .hpp files stored in different folders.\n>     Both files were deleted (and got (delete) status).\n>     Running this command without wildcards for each file separately\n> works as expected.\n\nThanks for the report. Can you provide a complete recipe in the form\nof shell command to make this happen so others can reproduce the\nbehavior? Doing so will help track down the issue. Also, since this is\nWindows, do the cases of the filenames in the referenced commit match\nthe cases actually on the filesystem (and have the cases changed\nbetween commits)?\n"},{"id":"404091","messageId":"CAFvH=vuFg+kM2GkBaE7jRqHWWcTcZMrs36KLS+-VTy8tgNZXJw@mail.gmail.com","threadId":"54029","inReplyTo":"CAPig+cSCd_8YB90sypTe1bHMQhPgo+Tr2PHNucdqfCpEe+Dosg@mail.gmail.com","subject":"Re: Possible bug with git restore","fromName":"Sergii Shkarnikov","fromEmail":"sergii.shkarnikov@globallogic.com","sentAt":"2020-08-20T12:59:00Z","receivedAt":"2020-08-20T12:59:46Z","isPatch":false,"sender":{"key":"sergii.shkarnikov@globallogic.com","avatar":null},"body":"Here is a script to reproduce the issue that works for me in Git Bash:\n\n=============================================\n#!/bin/bash\n\n#create repo with corresponding structure\nmkdir restore_bug_test\ncd restore_bug_test\nmkdir incl\nmkdir src\ntouch incl/test_file.hpp\ntouch src/test_file.cpp\ngit init\ngit add .\ngit commit -m\"initial\"\n\n#add a couple of commits\necho \"1\" >> incl/test_file.hpp\necho \"1\" >> src/test_file.cpp\ngit commit -am\"1\"\necho \"2\" >> incl/test_file.hpp\necho \"2\" >> src/test_file.cpp\ngit commit -am\"2\"\n\n#reproduce bug\ngit restore -s HEAD~ -- *test_file.*\ngit status\n===============================================\n\nHaven't checked filenames explicitly, but they haven't been changed manually.\nAnd there are no explicit case changes in the attached script.\n\nOn Sat, Aug 15, 2020 at 1:42 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Wed, Aug 12, 2020 at 2:51 PM Sergii Shkarnikov\n> <sergii.shkarnikov@globallogic.com> wrote:\n> >     I tried to restore a couple of files from an earlier commit\n> > running the restore command with a wildcard:\n> >\n> >     git restore -s HEAD~ -- */filename.*\n> >\n> >     In my work tree those are .cpp and .hpp files stored in different folders.\n> >     Both files were deleted (and got (delete) status).\n> >     Running this command without wildcards for each file separately\n> > works as expected.\n>\n> Thanks for the report. Can you provide a complete recipe in the form\n> of shell command to make this happen so others can reproduce the\n> behavior? Doing so will help track down the issue. Also, since this is\n> Windows, do the cases of the filenames in the referenced commit match\n> the cases actually on the filesystem (and have the cases changed\n> between commits)?\n"},{"id":"404094","messageId":"20200820134013.GA2526241@coredump.intra.peff.net","threadId":"54029","inReplyTo":"CAFvH=vuFg+kM2GkBaE7jRqHWWcTcZMrs36KLS+-VTy8tgNZXJw@mail.gmail.com","subject":"Re: Possible bug with git restore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-20T13:40:13Z","receivedAt":"2020-08-20T13:48:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 20, 2020 at 03:59:00PM +0300, Sergii Shkarnikov wrote:\n\n> Here is a script to reproduce the issue that works for me in Git Bash:\n> \n> =============================================\n> #!/bin/bash\n> \n> #create repo with corresponding structure\n> mkdir restore_bug_test\n> cd restore_bug_test\n> mkdir incl\n> mkdir src\n> touch incl/test_file.hpp\n> touch src/test_file.cpp\n> git init\n> git add .\n> git commit -m\"initial\"\n> \n> #add a couple of commits\n> echo \"1\" >> incl/test_file.hpp\n> echo \"1\" >> src/test_file.cpp\n> git commit -am\"1\"\n> echo \"2\" >> incl/test_file.hpp\n> echo \"2\" >> src/test_file.cpp\n> git commit -am\"2\"\n> \n> #reproduce bug\n> git restore -s HEAD~ -- *test_file.*\n> git status\n> ===============================================\n\nThat reproduces for me here on Linux, as well (for those just joining,\nthe interesting thing is that the final \"git status\" reports the files\nas deleted, rather than modified back to \"1\").\n\nInterestingly, if I do:\n\n  git restore -s HEAD~ --overlay -- *test_file.*\n\nthen I get:\n\n  error: pathspec '*test_file.*' did not match any file(s) known to git\n\nSo there are two oddities here:\n\n  - shouldn't that wildcard pathspec match those files? I've confirmed\n    that the glob characters make it into Git's pathspec machinery, and\n    since it doesn't have slashes, I think we'd match a basename (and\n    certainly \"git ls-files *test_file.*\" does what I expect).\n\n  - even if it doesn't match, it seems weird that overlay mode would\n    remove files that don't match. I'd expect it to remove files in\n    trees that _did_ match the pathspec, but leave anything outside of\n    the pathspec untouched.\n\n    It's almost like we matched the pathspec in the pass over the\n    working tree files, but failed to do so when reading in the tree.\n\n-Peff\n"},{"id":"404105","messageId":"c3f0d51a-d0e3-ed0a-c9ed-da092704da5c@web.de","threadId":"54029","inReplyTo":"20200820134013.GA2526241@coredump.intra.peff.net","subject":"Re: Possible bug with git restore","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-20T17:48:48Z","receivedAt":"2020-08-20T17:49:20Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.08.20 um 15:40 schrieb Jeff King:\n> On Thu, Aug 20, 2020 at 03:59:00PM +0300, Sergii Shkarnikov wrote:\n>\n>> Here is a script to reproduce the issue that works for me in Git Bash:\n\nThat's very helpful!\n\n>   - shouldn't that wildcard pathspec match those files? I've confirmed\n>     that the glob characters make it into Git's pathspec machinery, and\n>     since it doesn't have slashes, I think we'd match a basename (and\n>     certainly \"git ls-files *test_file.*\" does what I expect).\n\nNo, because restore doesn't interpret pathspecs recursively.  I don't\nknow why that causes files to disappear, though.  But here's a fix.\n\nNo sign-off because I don't understand why pathspec recursiveness is a\nthing that can be turned off -- I'd expect pathspec syntax to be\nconsistent for all commands.  So there might be a good reason why it was\nnot enabled for restore (and switch and checkout).\n\nThe flag was introduced in bc96cc87dbb (tree_entry_interesting():\nsupport depth limit, 2010-12-15), but I still don't fully get it.\nAnyway, here's what I got -- feel free to incorporate it in a real\npatch:\n\nRené\n\n\n---\n builtin/checkout.c               |  4 ++++\n t/t2072-restore-pathspec-file.sh | 19 ++++++++++++++++---\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28371954912..8d2dc0cfa48 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -66,6 +66,7 @@ struct checkout_opts {\n \tint can_switch_when_in_progress;\n \tint orphan_from_empty_tree;\n \tint empty_pathspec_ok;\n+\tint recursive_pathspec;\n \tint checkout_index;\n \tint checkout_worktree;\n \tconst char *ignore_unmerged_opt;\n@@ -1707,6 +1708,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n \t}\n\n+\topts->pathspec.recursive = opts->recursive_pathspec;\n+\n \tif (opts->pathspec.nr) {\n \t\tif (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)\n \t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n@@ -1852,6 +1855,7 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \topts.checkout_index = -1;    /* default off */\n \topts.checkout_worktree = -2; /* default on */\n \topts.ignore_unmerged_opt = \"--ignore-unmerged\";\n+\topts.recursive_pathspec = 1;\n\n \toptions = parse_options_dup(restore_options);\n \toptions = add_common_options(&opts, options);\ndiff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh\nindex 0d47946e8a9..da976665095 100755\n--- a/t/t2072-restore-pathspec-file.sh\n+++ b/t/t2072-restore-pathspec-file.sh\n@@ -9,18 +9,21 @@ test_tick\n test_expect_success setup '\n \ttest_commit file0 &&\n\n+\tmkdir dir1 &&\n+\techo 1 >dir1/file &&\n \techo 1 >fileA.t &&\n \techo 1 >fileB.t &&\n \techo 1 >fileC.t &&\n \techo 1 >fileD.t &&\n-\tgit add fileA.t fileB.t fileC.t fileD.t &&\n+\tgit add dir1 fileA.t fileB.t fileC.t fileD.t &&\n \tgit commit -m \"files 1\" &&\n\n+\techo 2 >dir1/file &&\n \techo 2 >fileA.t &&\n \techo 2 >fileB.t &&\n \techo 2 >fileC.t &&\n \techo 2 >fileD.t &&\n-\tgit add fileA.t fileB.t fileC.t fileD.t &&\n+\tgit add dir1 fileA.t fileB.t fileC.t fileD.t &&\n \tgit commit -m \"files 2\" &&\n\n \tgit tag checkpoint\n@@ -31,7 +34,7 @@ restore_checkpoint () {\n }\n\n verify_expect () {\n-\tgit status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual &&\n+\tgit status --porcelain --untracked-files=no -- dir1 fileA.t fileB.t fileC.t fileD.t >actual &&\n \ttest_cmp expect actual\n }\n\n@@ -161,4 +164,14 @@ test_expect_success 'error conditions' '\n \ttest_i18ngrep -e \"you must specify path(s) to restore\" err\n '\n\n+test_expect_success 'pathspec matches file in subdirectory' '\n+\trestore_checkpoint &&\n+\n+\techo \"*file\" | git restore --pathspec-from-file=- --source=HEAD^1 &&\n+\tcat >expect <<-\\EOF &&\n+\t M dir1/file\n+\tEOF\n+\tverify_expect\n+'\n+\n test_done\n--\n2.28.0\n"},{"id":"404107","messageId":"20200820182720.GA2537643@coredump.intra.peff.net","threadId":"54029","inReplyTo":"c3f0d51a-d0e3-ed0a-c9ed-da092704da5c@web.de","subject":"Re: Possible bug with git restore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-20T18:27:20Z","receivedAt":"2020-08-20T18:27:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 20, 2020 at 07:48:48PM +0200, René Scharfe wrote:\n\n> >   - shouldn't that wildcard pathspec match those files? I've confirmed\n> >     that the glob characters make it into Git's pathspec machinery, and\n> >     since it doesn't have slashes, I think we'd match a basename (and\n> >     certainly \"git ls-files *test_file.*\" does what I expect).\n> \n> No, because restore doesn't interpret pathspecs recursively.  I don't\n> know why that causes files to disappear, though.  But here's a fix.\n\nI think it's because of this comment from bc96cc87dbb:\n\n  When pathspec.recursive == 0, the behavior depends on match functions:\n  non-recursive for tree_entry_interesting() and recursive for\n  match_pathspec{,_depth}\n\nSo when we read the tree, we don't match recursively, and those entries\ndon't appear. But then we correlate that with the index:\n\n          /*\n           * Make sure all pathspecs participated in locating the paths\n           * to be checked out.\n           */\n          for (pos = 0; pos < active_nr; pos++)\n                  if (opts->overlay_mode)\n                          mark_ce_for_checkout_overlay(active_cache[pos],\n                                                       ps_matched,\n                                                       opts);\n                  else\n                          mark_ce_for_checkout_no_overlay(active_cache[pos],\n                                                          ps_matched,\n                                                          opts);\n\nAnd in no-overlay mode (the default for restore), we do:\n\n  \n  static void mark_ce_for_checkout_no_overlay(struct cache_entry *ce,\n                                              char *ps_matched,\n                                              const struct checkout_opts *opts)\n  {\n          ce->ce_flags &= ~CE_MATCHED;\n          if (!opts->ignore_skipworktree && ce_skip_worktree(ce))\n                  return;\n          if (ce_path_match(&the_index, ce, &opts->pathspec, ps_matched)) {\n                  ce->ce_flags |= CE_MATCHED;\n                  if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))\n                          /*\n                           * In overlay mode, but the path is not in\n                           * tree-ish, which means we should remove it\n                           * from the index and the working tree.\n                           */\n                          ce->ce_flags |= CE_REMOVE | CE_WT_REMOVE;\n          }\n  }\n\nAnd that ce_path_match() _does_ treat the pathspec recursively. So we\nsay \"yes, it matches in the index but wasn't in the tree, and therefore\nwe must delete it\".\n\nSo the fundamental issue is treating the pathspec in two different ways,\nand then correlating the results. We need to either do a recursive match\nfor the tree match (as your patch does), or do non-recursive for this\nindex match (which I don't think is trivial, because of the way the\nrecursive flag works).\n\n> No sign-off because I don't understand why pathspec recursiveness is a\n> thing that can be turned off -- I'd expect pathspec syntax to be\n> consistent for all commands.  So there might be a good reason why it was\n> not enabled for restore (and switch and checkout).\n\nI think it was originally done this way for compatibility of some\ncommands as we unified the pathspec code. But I'm having trouble digging\nup the exact details.\n\nHowever, it seems particularly egregious in checkout/restore, because we\nmay also be using the index as a source, in which case the pathspecs\n_would_ be recursive by default. E.g., in the test repo we've been\ndiscussing:\n\n  [make the index and working tree differ]\n  $ git reset HEAD^\n  Unstaged changes after reset:\n  M\tincl/test_file.hpp\n  M\tsrc/test_file.cpp\n\n  [restore using a wildcard, but out of the index rather than a tree]\n  $ git restore -- '*.hpp'\n\n  [and check that we did indeed match]\n  $ git status\n  On branch master\n  Changes not staged for commit:\n\tmodified:   src/test_file.cpp\n\nSo I think this inconsistency in pathspec matching between trees and the\nindex has probably existed in git-checkout for ages (and I guess people\ndon't do wildcards with trees often enough for anybody to have noticed).\nBut it didn't cause the index-deletion problem, because that only\nappeared more recently with the --no-overlay mode. That's the default\nfor restore, but you can trigger the problem with checkout, too:\n\n  $ git reset --hard\n  $ git checkout --no-overlay HEAD^ '*.hpp'\n  Updated 0 paths from 2668463\n  $ git status\n  On branch master\n  Changes to be committed:\n\tdeleted:    incl/test_file.hpp\n\n-Peff\n"},{"id":"404248","messageId":"7c5144d8-24cd-0d50-a7c6-ff77160d503d@web.de","threadId":"54029","inReplyTo":"20200820182720.GA2537643@coredump.intra.peff.net","subject":"[PATCH] checkout, restore: make pathspec recursive","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-22T08:57:59Z","receivedAt":"2020-08-22T08:58:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The pathspec given to git checkout and git restore is used with both\ntree_entry_interesting (via read_tree_recursive) and match_pathspec\n(via ce_path_match).  The latter effectively only supports recursive\nmatching regardless of the value of the pathspec flag \"recursive\",\nwhich is unset here.\n\nThat causes different match results for pathspecs with wildcards, and\ncan lead checkout and restore in no-overlay mode to remove entries\ninstead of modifying them.  Enable recursive matching for both checkout\nand restore to make matching consistent.\n\nSetting the flag in checkout_main() technically also affects git switch,\nbut since that command doesn't accept pathspecs at all this has no\nactual consequence.\n\nReported-by: Sergii Shkarnikov <sergii.shkarnikov@globallogic.com>\nInitial-test-by: Sergii Shkarnikov <sergii.shkarnikov@globallogic.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/checkout.c               |  2 ++\n t/t2025-checkout-no-overlay.sh   | 12 ++++++++++++\n t/t2072-restore-pathspec-file.sh | 19 ++++++++++++++++---\n 3 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28371954912..a65b4bf0b71 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1707,6 +1707,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\tdie(_(\"--pathspec-file-nul requires --pathspec-from-file\"));\n \t}\n\n+\topts->pathspec.recursive = 1;\n+\n \tif (opts->pathspec.nr) {\n \t\tif (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)\n \t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\ndiff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh\nindex 76330cb5ab7..fa9e0987063 100755\n--- a/t/t2025-checkout-no-overlay.sh\n+++ b/t/t2025-checkout-no-overlay.sh\n@@ -44,4 +44,16 @@ test_expect_success '--no-overlay --theirs with D/F conflict deletes file' '\n \ttest_path_is_missing file1\n '\n\n+test_expect_success 'wildcard pathspec matches file in subdirectory' '\n+\tgit reset --hard &&\n+\tmkdir subdir &&\n+\ttest_commit file3-1 subdir/file3 &&\n+\ttest_commit file3-2 subdir/file3 &&\n+\n+\tgit checkout --no-overlay file3-1 \"*file3\" &&\n+\techo file3-1 >expect &&\n+\ttest_path_is_file subdir/file3 &&\n+\ttest_cmp expect subdir/file3\n+'\n+\n test_done\ndiff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh\nindex 0d47946e8a9..b48345bf95f 100755\n--- a/t/t2072-restore-pathspec-file.sh\n+++ b/t/t2072-restore-pathspec-file.sh\n@@ -9,18 +9,21 @@ test_tick\n test_expect_success setup '\n \ttest_commit file0 &&\n\n+\tmkdir dir1 &&\n+\techo 1 >dir1/file &&\n \techo 1 >fileA.t &&\n \techo 1 >fileB.t &&\n \techo 1 >fileC.t &&\n \techo 1 >fileD.t &&\n-\tgit add fileA.t fileB.t fileC.t fileD.t &&\n+\tgit add dir1 fileA.t fileB.t fileC.t fileD.t &&\n \tgit commit -m \"files 1\" &&\n\n+\techo 2 >dir1/file &&\n \techo 2 >fileA.t &&\n \techo 2 >fileB.t &&\n \techo 2 >fileC.t &&\n \techo 2 >fileD.t &&\n-\tgit add fileA.t fileB.t fileC.t fileD.t &&\n+\tgit add dir1 fileA.t fileB.t fileC.t fileD.t &&\n \tgit commit -m \"files 2\" &&\n\n \tgit tag checkpoint\n@@ -31,7 +34,7 @@ restore_checkpoint () {\n }\n\n verify_expect () {\n-\tgit status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual &&\n+\tgit status --porcelain --untracked-files=no -- dir1 fileA.t fileB.t fileC.t fileD.t >actual &&\n \ttest_cmp expect actual\n }\n\n@@ -161,4 +164,14 @@ test_expect_success 'error conditions' '\n \ttest_i18ngrep -e \"you must specify path(s) to restore\" err\n '\n\n+test_expect_success 'wildcard pathspec matches file in subdirectory' '\n+\trestore_checkpoint &&\n+\n+\techo \"*file\" | git restore --pathspec-from-file=- --source=HEAD^1 &&\n+\tcat >expect <<-\\EOF &&\n+\t M dir1/file\n+\tEOF\n+\tverify_expect\n+'\n+\n test_done\n--\n2.28.0\n"},{"id":"404251","messageId":"73bddcfc-f8c9-6ec1-328d-a2f427c4aef2@web.de","threadId":"54029","inReplyTo":"20200820182720.GA2537643@coredump.intra.peff.net","subject":"Re: Possible bug with git restore","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-08-22T10:29:23Z","receivedAt":"2020-08-22T10:29:37Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.08.20 um 20:27 schrieb Jeff King:\n> On Thu, Aug 20, 2020 at 07:48:48PM +0200, René Scharfe wrote:\n>\n>>>   - shouldn't that wildcard pathspec match those files? I've confirmed\n>>>     that the glob characters make it into Git's pathspec machinery, and\n>>>     since it doesn't have slashes, I think we'd match a basename (and\n>>>     certainly \"git ls-files *test_file.*\" does what I expect).\n>>\n>> No, because restore doesn't interpret pathspecs recursively.  I don't\n>> know why that causes files to disappear, though.  But here's a fix.\n>\n> I think it's because of this comment from bc96cc87dbb:\n>\n>   When pathspec.recursive == 0, the behavior depends on match functions:\n>   non-recursive for tree_entry_interesting() and recursive for\n>   match_pathspec{,_depth}\n\n> So the fundamental issue is treating the pathspec in two different ways,\n> and then correlating the results. We need to either do a recursive match\n> for the tree match (as your patch does), or do non-recursive for this\n> index match (which I don't think is trivial, because of the way the\n> recursive flag works).\n\nIf using the same pathspec with both tree_entry_interesting and\nmatch_pathspec gives inconsistent results and can even lead to data loss\nas we've seen here, then we better prevent it.\n\nThe easiest way to do that would be to BUG out in match_pathspec if\nrecursive is unset, to indicate that it doesn't support non-recursive\nmatching.  Finding all the places that didn't bothered to set this flag\nsince it doesn't affect match_pathspec anyway would be quite tedious,\nthough.\n\nAt least the test suite still completes with the following evil patch\nand the fix I sent earlier (evil because it ignores const), so we\ncurrently don't have any other mismatches in covered code.\n\nRené\n\n---\n dir.c       | 4 ++++\n pathspec.h  | 2 ++\n tree-walk.c | 5 +++++\n 3 files changed, 11 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex fe64be30ed6..87d5ffa62d0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -432,6 +432,10 @@ static int do_match_pathspec(const struct index_state *istate,\n {\n \tint i, retval = 0, exclude = flags & DO_MATCH_EXCLUDE;\n\n+\t((struct pathspec *)ps)->match_pathspec = 1;\n+\tif (ps->tree_entry_interesting && !ps->recursive)\n+\t\tBUG(\"match_pathspec tree_entry_interesting !recursive\");\n+\n \tGUARD_PATHSPEC(ps,\n \t\t       PATHSPEC_FROMTOP |\n \t\t       PATHSPEC_MAXDEPTH |\ndiff --git a/pathspec.h b/pathspec.h\nindex 454ce364fac..bbae0abb249 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -32,6 +32,8 @@ struct pathspec {\n \tunsigned int has_wildcard:1;\n \tunsigned int recursive:1;\n \tunsigned int recurse_submodules:1;\n+\tunsigned int match_pathspec:1;\n+\tunsigned int tree_entry_interesting:1;\n \tunsigned magic;\n \tint max_depth;\n \tstruct pathspec_item {\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 0160294712b..f6465cd9cf4 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -1185,6 +1185,11 @@ enum interesting tree_entry_interesting(struct index_state *istate,\n \t\t\t\t\tconst struct pathspec *ps)\n {\n \tenum interesting positive, negative;\n+\n+\t((struct pathspec *)ps)->tree_entry_interesting = 1;\n+\tif (ps->match_pathspec && !ps->recursive)\n+\t\tBUG(\"match_pathspec tree_entry_interesting !recursive\");\n+\n \tpositive = do_match(istate, entry, base, base_offset, ps, 0);\n\n \t/*\n--\n2.28.0\n"},{"id":"404261","messageId":"xmqq364e79sm.fsf@gitster.c.googlers.com","threadId":"54029","inReplyTo":"20200820182720.GA2537643@coredump.intra.peff.net","subject":"Re: Possible bug with git restore","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-22T19:36:09Z","receivedAt":"2020-08-22T19:36:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I think this inconsistency in pathspec matching between trees and the\n> index has probably existed in git-checkout for ages (and I guess people\n> don't do wildcards with trees often enough for anybody to have noticed).\n> But it didn't cause the index-deletion problem, because that only\n> appeared more recently with the --no-overlay mode. That's the default\n> for restore, but you can trigger the problem with checkout, too:\n>\n>   $ git reset --hard\n>   $ git checkout --no-overlay HEAD^ '*.hpp'\n>   Updated 0 paths from 2668463\n>   $ git status\n>   On branch master\n>   Changes to be committed:\n> \tdeleted:    incl/test_file.hpp\n\nThe --no-overlay mode is an enhancement added on top of reasonably\naged codebase relatively recently.  Most of the core code in\ncheckout dates back to early 2008, while --no-overlay was done as a\nlong-overdue-afterthought in early last year.\n\nAnd it is not all that surprising that this issue took a long time\nto be discovered.\n\nThank you all for finding, analysing and fixing it promptly.\n\n\n"},{"id":"404369","messageId":"20200824202111.GA709709@coredump.intra.peff.net","threadId":"54029","inReplyTo":"7c5144d8-24cd-0d50-a7c6-ff77160d503d@web.de","subject":"Re: [PATCH] checkout, restore: make pathspec recursive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-24T20:21:11Z","receivedAt":"2020-08-24T20:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 22, 2020 at 10:57:59AM +0200, René Scharfe wrote:\n\n> The pathspec given to git checkout and git restore is used with both\n> tree_entry_interesting (via read_tree_recursive) and match_pathspec\n> (via ce_path_match).  The latter effectively only supports recursive\n> matching regardless of the value of the pathspec flag \"recursive\",\n> which is unset here.\n> \n> That causes different match results for pathspecs with wildcards, and\n> can lead checkout and restore in no-overlay mode to remove entries\n> instead of modifying them.  Enable recursive matching for both checkout\n> and restore to make matching consistent.\n> \n> Setting the flag in checkout_main() technically also affects git switch,\n> but since that command doesn't accept pathspecs at all this has no\n> actual consequence.\n\nThanks, I think this is the thing to do (and the code change looks\nobviously correct ;) ).\n\nThis may cause a user-visible behavior change even in overlay mode\n(because we'll now match some tree entries that we wouldn't before). But\nI think it's an improvement, because it makes:\n\n  git checkout <tree> -- <paths>\n\nconsistent with:\n\n  git checkout -- <paths>\n\nI don't know if you want to call that out more directly in the commit\nmessage.\n\n-Peff\n"},{"id":"404370","messageId":"20200824202556.GB709709@coredump.intra.peff.net","threadId":"54029","inReplyTo":"73bddcfc-f8c9-6ec1-328d-a2f427c4aef2@web.de","subject":"Re: Possible bug with git restore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-24T20:25:56Z","receivedAt":"2020-08-24T20:25:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 22, 2020 at 12:29:23PM +0200, René Scharfe wrote:\n\n> > So the fundamental issue is treating the pathspec in two different ways,\n> > and then correlating the results. We need to either do a recursive match\n> > for the tree match (as your patch does), or do non-recursive for this\n> > index match (which I don't think is trivial, because of the way the\n> > recursive flag works).\n> \n> If using the same pathspec with both tree_entry_interesting and\n> match_pathspec gives inconsistent results and can even lead to data loss\n> as we've seen here, then we better prevent it.\n\nYeah, it would be nice to do away with this inconsistency entirely. I\ndon't know if that even causes user-visible behavior changes at this\npoint, though.\n\n> The easiest way to do that would be to BUG out in match_pathspec if\n> recursive is unset, to indicate that it doesn't support non-recursive\n> matching.  Finding all the places that didn't bothered to set this flag\n> since it doesn't affect match_pathspec anyway would be quite tedious,\n> though.\n\nYeah. I think it's easier to approach it from the tree-entry side, and\nsay: which spots are not recursive, and could/should they be. Which I\nthink is where your patch below is going.\n\n> At least the test suite still completes with the following evil patch\n> and the fix I sent earlier (evil because it ignores const), so we\n> currently don't have any other mismatches in covered code.\n\nThat's kind-of good news, in that it lends support to the notion that\nbasically everything wants to be recursive. But I'd be worried there's a\nlingering code path that is not covered in the test suite. Certainly\nshipping your BUG() patch would flush it out, but it's not very friendly\nto users who do run into it.\n\nI wonder if we can drop it to a warning() and see if anybody reports it.\n\nOr the more-work but more-responsible version: manually trace all of the\npathspec calls to see which code paths might rely on leaving \"recursive\"\nunset.\n\n-Peff\n"}]}