{"thread":{"id":"63287","subject":"[PATCH 0/1] add: fix pathspec handling when literal filenames match wildcard","startedAt":"2025-04-12T09:46:16Z","lastAt":"2025-04-22T11:58:06Z","messageCount":19,"participants":["K Jayatheerth","Lucas Seiki Oshiro","JAYATHEERTH K","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"516033","messageId":"20250412094607.236382-1-jayatheerthkulkarni2005@gmail.com","threadId":"63287","inReplyTo":null,"subject":"[PATCH 0/1] add: fix pathspec handling when literal filenames match wildcard","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-12T09:46:06Z","receivedAt":"2025-04-12T09:46:16Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"This patch fixes a bug in `git add` where a wildcard pathspec (e.g., 'f*')\nfails to expand correctly if a file with the *exact* name ('f*') exists.\nPreviously, Git would incorrectly add only the literal match on the first\nrun and skip expanding the wildcard.\n\nWith this fix, wildcard expansion behaves consistently even in the presence\nof an exact filename match.\n\nTo explicitly add the literal file, users should quote the wildcard:\n git add 'f\\*'\n\nThanks you\n\nK Jayatheerth (1):\n add: fix handling literal filenames and wildcards\n\n dir.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\n-- \n2.49.0\n\n"},{"id":"516034","messageId":"20250412094607.236382-2-jayatheerthkulkarni2005@gmail.com","threadId":"63287","inReplyTo":"20250412094607.236382-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH 1/1] add: fix handling literal filenames and wildcards","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-12T09:46:07Z","receivedAt":"2025-04-12T09:46:23Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When `git add` is given a wildcard pathspec (e.g., 'f*'), and a file\nwith that *exact* name ('f*') also exists, Git incorrectly adds only\nthe literal file on the first invocation, ignoring other wildcard\nmatches like 'foo'. On subsequent invocations, the wildcard expands\nas expected.\n\nThis occurs because the pathspec matching logic short-circuits when\nan exact match is found, skipping wildcard evaluation.\n\nWith this fix, wildcard expansion is always performed, ensuring\nconsistent behavior even when a literal filename matches the\nwildcard.\n\nTo explicitly add the literal file named 'f*', users should use:\n git add 'f\\*'\n\nreported-by: piotrsiupa \nMentored-by: Jeff King \nSigned-off-by: K Jayatheerth \n---\n dir.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex cbd82be6c9..0546c00952 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -519,7 +519,8 @@ static int do_match_pathspec(struct index_state *istate,\n \t\t ( exclude && !(ps->items[i].magic & PATHSPEC_EXCLUDE)))\n \t\t\tcontinue;\n \n-\t\tif (seen && seen[i] == MATCHED_EXACTLY)\n+\t\tif (seen && seen[i] == MATCHED_EXACTLY &&\n+\t\t\tps->items[i].nowildcard_len == ps->items[i].len)\n \t\t\tcontinue;\n \t\t/*\n \t\t * Make exclude patterns optional and never report\n-- \n2.49.0\n\n"},{"id":"516043","messageId":"9EB38EEC-97EA-4F8D-BCC6-883771203755@gmail.com","threadId":"63287","inReplyTo":"20250412094607.236382-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH 0/1] add: fix pathspec handling when literal filenames match wildcard","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2025-04-12T15:45:40Z","receivedAt":"2025-04-12T15:45:55Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Hi!\n\nThe original bug repot included some simple steps to reproduce\nthe bug. Since it is a corner case that wasn't covered by our\ncurrent test suite, it would be nice if you created a new\ntest case covering that.\n\nPS: Since this is a single patch, you could sent this without\na cover letter, putting your comments after the scissors mark\n(---).\n"},{"id":"516045","messageId":"CA+rGoLdDVYKUz02dhgPy50nhG-K-cjyLUySFtunGL1+f=AD7wg@mail.gmail.com","threadId":"63287","inReplyTo":"9EB38EEC-97EA-4F8D-BCC6-883771203755@gmail.com","subject":"Re: [PATCH 0/1] add: fix pathspec handling when literal filenames match wildcard","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-12T17:34:40Z","receivedAt":"2025-04-12T17:34:53Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Sat, Apr 12, 2025 at 9:15 PM Lucas Seiki Oshiro\n wrote:\n>\n> Hi!\n\nHey Lucas!\n\n>\n> The original bug repot included some simple steps to reproduce\n> the bug. Since it is a corner case that wasn't covered by our\n> current test suite, it would be nice if you created a new\n> test case covering that.\n>\nYeah!! I was testing them with options like f** and f\\*\\* for specific\nfile additions for commits\nI have framed a test frame in t-3706 which is the add and pathspec\ncoverage as far as I can see.\n\n> PS: Since this is a single patch, you could sent this without\n> a cover letter, putting your comments after the scissors mark\n> (---).\n\nAhh I do that because if I receive feedback it forms like a good\nsummary in the beginning.\nBut sure, I think the *scissors mark* method sounds good too!!\n\nWill send the patch inline to this email\nI think you've worked with Tests in your microproject\nDrop some feedback. Those will be very helpful!!\n\nThank you,\n-Jayatheerth\n"},{"id":"516046","messageId":"20250412174051.780148-1-jayatheerthkulkarni2005@gmail.com","threadId":"63287","inReplyTo":"20250412094607.236382-2-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-12T17:40:51Z","receivedAt":"2025-04-12T17:41:01Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When 'git add ' is run, and a file exists whose literal\nname matches (e.g., a file named \"f*\" when running\n'git add f*'), Git should treat the pattern as a wildcard\nmatching multiple files, rather than just adding the literal file.\n\nAdd a test case that:\n 1. Creates files 'f*', 'f**', and 'foo'.\n 2. Verifies that 'git add \"f\\*\\*\"'\n correctly adds only the literal file 'f**'.\n 3. Verifies that 'git add 'f*'' (quoted to prevent shell expansion)\n correctly adds 'f*', 'f**', and 'foo' by treating 'f*' as a\n wildcard.\n\nCovering these the test adds 5 cases.\n\nSigned-off-by: K Jayatheerth \n---\n t/meson.build | 1 +\n t/t3706-add-wildcard-literal.sh | 44 +++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n create mode 100755 t/t3706-add-wildcard-literal.sh\n\ndiff --git a/t/meson.build b/t/meson.build\nindex 8b3aed14ea..e9cd9da8a2 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -417,6 +417,7 @@ integration_tests = [\n 't3703-add-magic-pathspec.sh',\n 't3704-add-pathspec-file.sh',\n 't3705-add-sparse-checkout.sh',\n+ 't3706-add-wildcard-literal.sh',\n 't3800-mktag.sh',\n 't3900-i18n-commit.sh',\n 't3901-i18n-patch.sh',\ndiff --git a/t/t3706-add-wildcard-literal.sh b/t/t3706-add-wildcard-literal.sh\nnew file mode 100755\nindex 0000000000..0fc27b28ac\n--- /dev/null\n+++ b/t/t3706-add-wildcard-literal.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+\n+test_description='git add: wildcard must not be shadowed by literal filename'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: create files and initial commit' '\n+ mkdir testdir &&\n+ >testdir/f\\* &&\n+ >testdir/f\\*\\* &&\n+ >testdir/foo &&\n+ git add testdir &&\n+ git commit -m \"Initial setup with literal wildcard files\"\n+'\n+\n+test_expect_success 'clean slate before testing wildcard behavior' '\n+ git rm -rf testdir &&\n+ git commit -m \"Clean state\"\n+'\n+\n+test_expect_success 'recreate files to test add behavior' '\n+ mkdir testdir &&\n+ >testdir/f\\* &&\n+ >testdir/f\\*\\* &&\n+ >testdir/foo\n+'\n+\n+test_expect_success 'quoted literal: git add \"f\\\\*\\\\*\" adds only f**' '\n+ git reset &&\n+ git add \"testdir/f\\\\*\\\\*\" &&\n+ git ls-files >actual &&\n+ echo \"testdir/f**\" >expected &&\n+ test_cmp expected actual\n+'\n+\n+test_expect_success 'wildcard: git add f* adds f*, f** and foo' '\n+ git reset &&\n+ git add '\\''testdir/f*'\\'' &&\n+ git ls-files | sort >actual &&\n+ printf \"%s\\n\" \"testdir/f*\" \"testdir/f**\" \"testdir/foo\" | sort >expected &&\n+ test_cmp expected actual\n+'\n+\n+test_done\n\\ No newline at end of file\n-- \n2.49.0\n\n"},{"id":"516116","messageId":"FFC5C6BE-9A4B-4EFD-AEC4-E69CE6610054@gmail.com","threadId":"63287","inReplyTo":"20250412174051.780148-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2025-04-14T16:51:22Z","receivedAt":"2025-04-14T16:51:37Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Hi!\n\n> +test_expect_success 'setup: create files and initial commit' '\n> + mkdir testdir &&\n> + >testdir/f\\* &&\n> + >testdir/f\\*\\* &&\n> + >testdir/foo &&\n> + git add testdir &&\n> + git commit -m \"Initial setup with literal wildcard files\"\n> +'\n> +\n> +test_expect_success 'clean slate before testing wildcard behavior' '\n> + git rm -rf testdir &&\n> + git commit -m \"Clean state\"\n> +'\n>\n> +test_expect_success 'recreate files to test add behavior' '\n> + mkdir testdir &&\n> + >testdir/f\\* &&\n> + >testdir/f\\*\\* &&\n> + >testdir/foo\n> +'\n\n\nTwo questions:\n\n1. Does this need to be inside a test_expect_success? It seems to me\n that those two tests cases are actually setup code for the next\n two.\n\n2. If so, does it need to have all that setup? I could reproduce the\n bug by only running:\n\n ```\n git reset\n touch foo 'f*' 'f**'\n git add 'f*'\n git ls-files \n ```\n\n btw, this works with your code, congrats!\n\nOther idea: `?` is another wildcard for matching only one character.\nHave you tested if the same bug happens with it?\n\nPS: while I was writing this review I pushed this to my GitHub just\nto make the CI run the entire test suite since pathspecs are a\nsensible part of Git.\n\nTake look at this, it seems that your tests aren't passing on Windows:\nhttps://github.com/lucasoshiro/git/actions/runs/14450183624/job/40521015897.\nPerhaps you'll need to change something there. It seems to be\nrelated to how Windows handle paths (specially the \\ character, which\nmeans the same as / in Unix). Personally, I'm not a Windows guy and\ncan't help you further with this. A quick reference on how paths on\nWindows work is this (and yeah, they are far more complex than in\nUnix):\nhttps://www.fileside.app/blog/2023-03-17_windows-file-paths/\n\n\n"},{"id":"516117","messageId":"CA+rGoLc=CYehMms0bxaLAu3xP-p62cMJ++RnBxSMOF2Ppt38QQ@mail.gmail.com","threadId":"63287","inReplyTo":"FFC5C6BE-9A4B-4EFD-AEC4-E69CE6610054@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-14T17:08:39Z","receivedAt":"2025-04-14T17:08:51Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Mon, Apr 14, 2025 at 10:21 PM Lucas Seiki Oshiro\n wrote:\n>\n> Hi!\n>\nHi Lucas,\n\n> > +test_expect_success 'setup: create files and initial commit' '\n> > + mkdir testdir &&\n> > + >testdir/f\\* &&\n> > + >testdir/f\\*\\* &&\n> > + >testdir/foo &&\n> > + git add testdir &&\n> > + git commit -m \"Initial setup with literal wildcard files\"\n> > +'\n> > +\n> > +test_expect_success 'clean slate before testing wildcard behavior' '\n> > + git rm -rf testdir &&\n> > + git commit -m \"Clean state\"\n> > +'\n> >\n> > +test_expect_success 'recreate files to test add behavior' '\n> > + mkdir testdir &&\n> > + >testdir/f\\* &&\n> > + >testdir/f\\*\\* &&\n> > + >testdir/foo\n> > +'\n>\n>\n> Two questions:\n>\n> 1. Does this need to be inside a test_expect_success? It seems to me\n> that those two tests cases are actually setup code for the next\n> two.\n>\n> 2. If so, does it need to have all that setup? I could reproduce the\n> bug by only running:\n>\n> ```\n> git reset\n> touch foo 'f*' 'f**'\n> git add 'f*'\n> git ls-files\n> ```\n>\n\nUmm, I think the setup should just be a single block. I will send a\npatch on this\nI'm parallely working on a second patch.\n\n> btw, this works with your code, congrats!\n>\n\nThank you, credit goes to Peff(Jeff King) I almost lost track.\n\n> Other idea: `?` is another wildcard for matching only one character.\n> Have you tested if the same bug happens with it?\n>\n\nYup I think that's also a great suggestion, I think consolidating setup\nand adding at least a few different wildcards will be good.\n\n> PS: while I was writing this review I pushed this to my GitHub just\n> to make the CI run the entire test suite since pathspecs are a\n> sensible part of Git.\n>\n> Take look at this, it seems that your tests aren't passing on Windows:\n> https://github.com/lucasoshiro/git/actions/runs/14450183624/job/40521015897.\n\nOh damn!! That's a silly mistake. I almost forgot windows exist!!\nThanks for letting me know\n\n> Perhaps you'll need to change something there. It seems to be\n> related to how Windows handle paths (specially the \\ character, which\n> means the same as / in Unix). Personally, I'm not a Windows guy and\n\nSame not a windows guy, but I will have to read some things out.\nWill figure it out.\n\n> can't help you further with this. A quick reference on how paths on\n> Windows work is this (and yeah, they are far more complex than in\n> Unix):\n> https://www.fileside.app/blog/2023-03-17_windows-file-paths/\n>\n>\n\nThanks again Lucas, these help.\n\n-Jayatheerth\n"},{"id":"516148","messageId":"xmqqfriapghv.fsf@gitster.g","threadId":"63287","inReplyTo":"20250412174051.780148-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-14T21:42:20Z","receivedAt":"2025-04-14T21:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth writes:\n\n> When 'git add ' is run, and a file exists whose literal\n> name matches (e.g., a file named \"f*\" when running\n> 'git add f*'), Git should treat the pattern as a wildcard\n> matching multiple files, rather than just adding the literal file.\n\nThis pretends to be a separate and independent patch, but it in\nreality depends on the other change to dir.c, doesn't it?\n\nEither make them into two-patch series, or better yet make them into\na single patch, perhaps?\n\n> Add a test case that:\n> 1. Creates files 'f*', 'f**', and 'foo'.\n> 2. Verifies that 'git add \"f\\*\\*\"'\n> correctly adds only the literal file 'f**'.\n> 3. Verifies that 'git add 'f*'' (quoted to prevent shell expansion)\n> correctly adds 'f*', 'f**', and 'foo' by treating 'f*' as a\n> wildcard.\n\nThat is something easily can be read from the script itself. Don't\nrepeat what the code does, UNLESS the explanation helps readers to\nunderstand what the code does NOT tell them (like, \"why does the\ncode does what it does?\").\n\n> Covering these the test adds 5 cases.\n>\n> Signed-off-by: K Jayatheerth \n> ---\n> t/meson.build | 1 +\n> t/t3706-add-wildcard-literal.sh | 44 +++++++++++++++++++++++++++++++++\n> 2 files changed, 45 insertions(+)\n> create mode 100755 t/t3706-add-wildcard-literal.sh\n\nI am not sure if this should be done as a new test script and covers\nonly \"git add\". Is \"git add\" the only code paths that triggers\ndo_match_pathspec()? If there are many, it may make sense to have a\nnew script (like this patch does) and cover all of these commands\nthat are corrected by the previous patch. If not, and if it truly\naffects only \"git add\", then it would make more sense to add to an\nexisting test script that covers \"git add\".\n\n> +test_expect_success 'setup: create files and initial commit' '\n> + mkdir testdir &&\n> + >testdir/f\\* &&\n> + >testdir/f\\*\\* &&\n> + >testdir/foo &&\n> + git add testdir &&\n> + git commit -m \"Initial setup with literal wildcard files\"\n> +'\n> +\n> +test_expect_success 'clean slate before testing wildcard behavior' '\n> + git rm -rf testdir &&\n> + git commit -m \"Clean state\"\n> +'\n\nWhat's the point of the above two test pieces? The initial commit\nis not used in any way after it gets created, the wildcard pathspec,\nwhich is the topic of this test, is not involved in its creation in\nany way, and everything gets removed before the real test after this\nstep starts. Not complaining, but showing puzzlement, as I truly do\nnot see the point of these two steps.\n\n> +test_expect_success 'recreate files to test add behavior' '\n> + mkdir testdir &&\n> + >testdir/f\\* &&\n> + >testdir/f\\*\\* &&\n> + >testdir/foo\n> +'\n> +\n> +test_expect_success 'quoted literal: git add \"f\\\\*\\\\*\" adds only f**' '\n> + git reset &&\n\nWhat is the point of this \"reset\"? Since the last commit, the tree\nof HEAD matches the index, and nobody touched the index after that.\n\n> + git add \"testdir/f\\\\*\\\\*\" &&\n\nOK. We are giving the pathspec with asterisks quoted in it, to make\nsure the asterisks are not interpreted as glob. So there is only\none path that gets added in the end, and that is what is tested.\n\nThat's a good test (except that I do not understand what the \"reset\"\nat the beginning is trying to do).\n\n> + git ls-files >actual &&\n> + echo \"testdir/f**\" >expected &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'wildcard: git add f* adds f*, f** and foo' '\n> + git reset &&\n> + git add '\\''testdir/f*'\\'' &&\n\nWhy not just\n\n\tgit add \"testdir/f*\" &&\n\nto match the previous one?\n\n> + git ls-files | sort >actual &&\n\n\"git\" command on the left hand side of a pipe means even if it\nfails, its exit status is hidden. Also, the order ls-files emits\npaths is well defined (sorted in the byte-value order regardless of\nlocale), so piping its output through sort loses information\nunnecessarily (you wouldn't notice if ls-files started emitting its\npaths in an incorrect order).\n\nSince you use these three pathnames in this order quote often in\nthis script, why not do this once upfront:\n\n\tcat >list-of-files <<-\\EOF &&\n\ttestdir/f*\n\ttestdir/f**\n testdir/foo\n\tEOF\n\nand then use the list of files in later steps like so:\n\n\twhile read path\n\tdo\n\t\t>\"$path\"\n\tdone actual &&\n\ttest_cmp list-of-files actual\n\n> + printf \"%s\\n\" \"testdir/f*\" \"testdir/f**\" \"testdir/foo\" | sort >expected &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_done\n> \\ No newline at end of file\n\nPlease make sure your files are properly formatted. Perhaps we\nshould add whitespace error class 'incomplete-lines' that catches\nthis, and enable it globally, or something?\n\n"},{"id":"516228","messageId":"Z_7ekhsBzXK6LKuV@tapette.crustytoothpaste.net","threadId":"63287","inReplyTo":"20250412174051.780148-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-04-15T22:32:50Z","receivedAt":"2025-04-15T22:32:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-04-12 at 17:40:51, K Jayatheerth wrote:\n> +test_expect_success 'recreate files to test add behavior' '\n> + mkdir testdir &&\n> + >testdir/f\\* &&\n> + >testdir/f\\*\\* &&\n\nI just want to point out that creating files with asterisks may not be\npossible on Windows due to limitations in the file system. I'm not a\nWindows expert, so unfortunately I can't provide more details than that,\nbut you may end up needing to add a prerequisite here to skip this on\nour Windows platforms if necessary. Hopefully CI and a suitable search\ncan help you figure it out.\n\n> +test_done\n> \\ No newline at end of file\n\nWe do want to keep newlines at the end of a file. POSIX mandates one on\ntext files and some systems are less tolerant of missing newlines than\nothers. Usually Linux and the BSDs handle this just fine, but some\nproprietary Unix systems, which unfortunately we don't have CI for, tend\nto be the ones that are less happy about this.\n\nI haven't given this a full review, since others have done that instead,\nbut just pointed out one or two things that got my attention.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"516246","messageId":"CA+rGoLfAidyuomeNym5WX8Bo7-jPcfHx35wDeZ7W2aorAN-B7g@mail.gmail.com","threadId":"63287","inReplyTo":"Z_7ekhsBzXK6LKuV@tapette.crustytoothpaste.net","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-16T01:56:26Z","receivedAt":"2025-04-16T01:56:39Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Wed, Apr 16, 2025 at 4:02 AM brian m. carlson\n wrote:\n>\n> On 2025-04-12 at 17:40:51, K Jayatheerth wrote:\n> > +test_expect_success 'recreate files to test add behavior' '\n> > + mkdir testdir &&\n> > + >testdir/f\\* &&\n> > + >testdir/f\\*\\* &&\n>\n> I just want to point out that creating files with asterisks may not be\n> possible on Windows due to limitations in the file system. I'm not a\n> Windows expert, so unfortunately I can't provide more details than that,\n> but you may end up needing to add a prerequisite here to skip this on\n> our Windows platforms if necessary. Hopefully CI and a suitable search\n> can help you figure it out.\n>\n\nOk I will look into it, thank you for letting me know\n\n> > +test_done\n> > \\ No newline at end of file\n>\n> We do want to keep newlines at the end of a file. POSIX mandates one on\n> text files and some systems are less tolerant of missing newlines than\n> others. Usually Linux and the BSDs handle this just fine, but some\n> proprietary Unix systems, which unfortunately we don't have CI for, tend\n> to be the ones that are less happy about this.\n>\n\nOk I will make sure of that\n\n> I haven't given this a full review, since others have done that instead,\n> but just pointed out one or two things that got my attention.\n> --\n> brian m. carlson (they/them)\n> Toronto, Ontario, CA\n\n\nWhile I was looking into the reviews I was creating various test cases\nwith these files\n'*' '**' '?' '\\*' '[abc]' commit_files 'f*' 'f**' 'file[1-3]'\n 'foo*bar' 'f?z' 'hello?world'\n\nEverything went correct\nBut when I checked \\* and which is used for getting * as specific\nbut there is also a literal \\* in the above files\n\nSo it still adds both, I'm unsure if that is the intended behaviour.\n\nbut when I say git add \"\\*\" it adds both the files * and \\*\nBut rest of the other wildcards and literals work as intended which is why\nI incorporated the \\* literal\n\nI also think I will still divide the test file because\ngit add isn't the only one that looks into wildcards and pathspec\n\nI think something like git commit \"*\" -m \"Test\" also would be a great test\n\nor even git rm command.\n\nAbout the windows question, I think I will see if there is any common\nground I could find\nBut until then I think prereq is a great option.\n\nFor reference my test file looks something like this,\n\n--- /dev/null\n+++ b/t/t6137-pathspec-wildcard-literal.sh\n@@ -0,0 +1,139 @@\n+#!/bin/sh\n+\n+test_description='test wildcards and literals with various git commands'\n+\n+. ./test-lib.sh\n+\n+reset_git_repo () {\n+ rm -rf .git &&\n+ git init\n+}\n+\n+test_expect_success 'setup' '\n+ mkdir testdir &&\n+ cd testdir &&\n+ touch \"*\" \"?\" \"[abc]\" \"f*\" \"f?z\" \"a\" &&\n+ touch \"**\" \"foo*bar\" \"hello?world\" \"f**\" \"hello_world\" &&\n+ git init\n+'\n+\n+test_expect_success 'check * wildcard in git add' '\n+ git init &&\n+ git add \"*\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check \\* literal in git add' '\n+ reset_git_repo &&\n+ git add \"\\*\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check f* wildcard in git add' '\n+ reset_git_repo &&\n+ git add \"f*\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check f\\* literal in git add' '\n+ reset_git_repo &&\n+ git add \"f\\*\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check f** wildcard in git add' '\n+ reset_git_repo &&\n+ git add \"f**\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check f\\*\\* literal in git add' '\n+ reset_git_repo &&\n+ git add \"f\\*\\*\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check ? wildcard in git add' '\n+ reset_git_repo &&\n+ git add \"?\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check \\? literal in git add' '\n+ reset_git_repo &&\n+ git add \"\\?\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check hello?world wildcard in git add' '\n+ reset_git_repo &&\n+ git add \"hello?world\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_expect_success 'check hello\\?world literal in git add' '\n+ reset_git_repo &&\n+ git add \"hello\\?world\" &&\n+ cat >expected_files <actual_files &&\n+ test_cmp expected_files actual_files\n+'\n+\n+test_done\n-- \n2.49.GIT\n"},{"id":"516285","messageId":"xmqqa58gjlnk.fsf@gitster.g","threadId":"63287","inReplyTo":"CA+rGoLfAidyuomeNym5WX8Bo7-jPcfHx35wDeZ7W2aorAN-B7g@mail.gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-16T13:11:59Z","receivedAt":"2025-04-16T13:12:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"JAYATHEERTH K writes:\n\n> I think something like git commit \"*\" -m \"Test\" also would be a great test\n> or even git rm command.\n\nYes, there are things other than \"git add\" that take pathspec (even\n\"git ls-files\" does so), and demonstrating the blast radious of the\nexisting \"bug\" you fixed, with how they behave differently and\nbetter with your fix, would be a good thing to do.\n\nBut make sure you follow \"dashed options first, then other args\"\nconvention. I do not offhand know (and *MORE* *IMPORTANTLY*, I do\nnot want anybody to depend on) what the current command line parser\nhappens to do to\n\n git commit \"*\" -m \"Test\"\n\nIf you meant to say that a pathspec with glob, always write it the\nright way:\n\n\tgit commit -m \"Test\" -- \"*\"\n\n> About the windows question, I think I will see if there is any common\n> ground I could find\n> But until then I think prereq is a great option.\n\nThe FUNNYNAMES prereq was invented to mark tests that rely on\nfilesystem being able to handle certain letters, so that may be a\ngood thing to use.\n"},{"id":"516291","messageId":"CA+rGoLesZ3nSjruJ8_XRWVsFpMu8mo_4cCOdB-GFHU_qXkXDCQ@mail.gmail.com","threadId":"63287","inReplyTo":"xmqqa58gjlnk.fsf@gitster.g","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-16T14:49:46Z","receivedAt":"2025-04-16T14:49:58Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Wed, Apr 16, 2025 at 6:42 PM Junio C Hamano wrote:\n>\n> JAYATHEERTH K writes:\n>\n> > I think something like git commit \"*\" -m \"Test\" also would be a great test\n> > or even git rm command.\n>\n> Yes, there are things other than \"git add\" that take pathspec (even\n> \"git ls-files\" does so), and demonstrating the blast radious of the\n> existing \"bug\" you fixed, with how they behave differently and\n> better with your fix, would be a good thing to do.\n>\n\nYes ls-files is also a great example, I will add them in the test.\nI think for the pathspec and glob specific commands almost all the commands\nshare the same code, so it should work the same for all.\n\n> But make sure you follow \"dashed options first, then other args\"\n> convention. I do not offhand know (and *MORE* *IMPORTANTLY*, I do\n> not want anybody to depend on) what the current command line parser\n> happens to do to\n>\n> git commit \"*\" -m \"Test\"\n>\n> If you meant to say that a pathspec with glob, always write it the\n> right way:\n>\n> git commit -m \"Test\" -- \"*\"\n>\n\nOk, it makes sense, I will follow this format.\n\n> > About the windows question, I think I will see if there is any common\n> > ground I could find\n> > But until then I think prereq is a great option.\n>\n> The FUNNYNAMES prereq was invented to mark tests that rely on\n> filesystem being able to handle certain letters, so that may be a\n> good thing to use.\n\nNoted will add this too, I'm still finding all the possible files and\nhow they could matter in this test.\nOnce I do that and check if the CI works or not and if I cannot make\nit work with both platforms\n(Which I think most probably will happen) I will add the prereq\nFUNNYNAMES as intended.\n\n-Jayatheerth\n"},{"id":"516298","messageId":"717161C8-497D-42C7-8C10-AC112238EEFD@gmail.com","threadId":"63287","inReplyTo":"CA+rGoLesZ3nSjruJ8_XRWVsFpMu8mo_4cCOdB-GFHU_qXkXDCQ@mail.gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2025-04-16T15:49:51Z","receivedAt":"2025-04-16T15:50:07Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"\n> Yes ls-files is also a great example, I will add them in the test.\n\n\nI was going to suggest you to use `git ls-files -o 'f**'` in your test,\n which would eliminate the need of `git add` and `git reset`. However, I\njust found that the bug doesn't happen here:\n\n```\ngit init\ntouch foo 'f*' 'f**' f bar\ngit ls-files -o 'f*'\n```\n\nHere (I'm using the current `next`, currently at `fd585f7`),\n`git ls-files -o 'f*'` list the files correctly:\n\n```\nf\nf*\nf**\nfoo\n```\n\nI also tried with `git grep`:\n\n```\ngit init\ntouch foo 'f*' 'f**' f bar\nfor f in *; do echo 123 > \"$f\"; done\ngit add -A\ngit grep 123 -- 'f*'\n\nand somehow it worked:\n\n```\nf:1:123\nf*:1:123\nf**:1:123\nfoo:1:123\n```\n\nSo, if I'm not doing anything wrong, it looks that it is not solely\nrelated to pathspecs, but related to pathspecs when used with some other\ncommands. hmmm...\n\n> I think for the pathspec and glob specific commands almost all the commands\n> share the same code, so it should work the same for all.\n\n\nI also though the same, but somehow it behaves differently at least with\n`ls-files` and `grep`. Perhaps it will need further investigation on how\nsome commands behave correctly and some don't. I would start by\ninspecting other commands that uses pathspecs (some that I remember:\ncheckout, log, show, stash, status, ls-files, grep) and see if they work\ncorrectly or not, then compare the two groups and see what differs\nbetween them under the hook.\n"},{"id":"516299","messageId":"xmqqcydchz9o.fsf@gitster.g","threadId":"63287","inReplyTo":"717161C8-497D-42C7-8C10-AC112238EEFD@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-16T16:00:51Z","receivedAt":"2025-04-16T16:00:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Seiki Oshiro writes:\n\n> So, if I'm not doing anything wrong, it looks that it is not solely\n> related to pathspecs, but related to pathspecs when used with some other\n> commands. hmmm...\n\nI haven't looked into the code, but if my recollection is correct,\n\"add\" is a bit curious in that it has to deal with paths that are\nnot yet in the index, unlike \"ls-files\" and \"grep\". It could be one\nhalf of the code paths use the \"grab everything that matches the\nglob\" while the other half uses \"stop when there is an exact match\",\nperhaps? In the very early days of Git, I do recall making a very\nconscious decision to stop when there is an exact match to help\nthose who add funny pathsnames with glob characters in it, but that\nis a long time ago, so I wouldn't be surprised if we gained multiple\ncode paths to do the same thing, some of which have been corrected\nwhile the original ones haven't.\n\n"},{"id":"516302","messageId":"CA+rGoLeUojJ4BAUDy2kBsMjRCEFTP=so7sDgy1BX6d7RoBusEA@mail.gmail.com","threadId":"63287","inReplyTo":"717161C8-497D-42C7-8C10-AC112238EEFD@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-16T16:40:27Z","receivedAt":"2025-04-16T16:40:39Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Wed, Apr 16, 2025 at 9:20 PM Lucas Seiki Oshiro\n wrote:\n>\n>\n> > Yes ls-files is also a great example, I will add them in the test.\n>\n>\n> I was going to suggest you to use `git ls-files -o 'f**'` in your test,\n> which would eliminate the need of `git add` and `git reset`. However, I\n> just found that the bug doesn't happen here:\n>\n> ```\n> git init\n> touch foo 'f*' 'f**' f bar\n> git ls-files -o 'f*'\n> ```\n>\n> Here (I'm using the current `next`, currently at `fd585f7`),\n> `git ls-files -o 'f*'` list the files correctly:\n>\n> ```\n> f\n> f*\n> f**\n> foo\n> ```\n>\n> I also tried with `git grep`:\n>\n> ```\n> git init\n> touch foo 'f*' 'f**' f bar\n> for f in *; do echo 123 > \"$f\"; done\n> git add -A\n> git grep 123 -- 'f*'\n>\n> and somehow it worked:\n>\n> ```\n> f:1:123\n> f*:1:123\n> f**:1:123\n> foo:1:123\n> ```\n>\n> So, if I'm not doing anything wrong, it looks that it is not solely\n> related to pathspecs, but related to pathspecs when used with some other\n> commands. hmmm...\n>\n> > I think for the pathspec and glob specific commands almost all the commands\n> > share the same code, so it should work the same for all.\n>\n>\n> I also though the same, but somehow it behaves differently at least with\n> `ls-files` and `grep`. Perhaps it will need further investigation on how\n> some commands behave correctly and some don't. I would start by\n> inspecting other commands that uses pathspecs (some that I remember:\n> checkout, log, show, stash, status, ls-files, grep) and see if they work\n> correctly or not, then compare the two groups and see what differs\n> between them under the hook.\n\n\nThat is interesting\ngiven that this part of code\n\nwhile (--i >= 0) {\nstruct dir_entry *entry = *src++;\nif (dir_path_match(repo->index, entry, pathspec, prefix, seen))\n*dst++ = entry;\n}\n\ntakes part in dir_path_match\nWhich traces to *Exact Match* problem that was found\n\nand from ls files\n\nif (!index_name_is_other(istate, ent->name, ent->len))\ncontinue;\nshow_dir_entry(istate, tag_other, ent);\n\nWhere show_dir_entry calls\nstatic void show_dir_entry(struct index_state *istate,\nconst char *tag, struct dir_entry *ent)\n{\n int len = max_prefix_len;\n\n if (len > ent->len)\n die(\"git ls-files: internal error - directory entry not\nsuperset of prefix\");\n\n /* If ps_matches is non-NULL, figure out which pathspec(s) match. */\n if (ps_matched)\n dir_path_match(istate, ent, &pathspec, len, ps_matched);\n\n\nSomething like this\nSo I think the argument values are making the difference but I'm still unsure.\n"},{"id":"516303","messageId":"CA+rGoLfxtVL7ang320YZ3nN++GCCrPW8E4pTHbWyMu2EEaWz3g@mail.gmail.com","threadId":"63287","inReplyTo":"CA+rGoLeUojJ4BAUDy2kBsMjRCEFTP=so7sDgy1BX6d7RoBusEA@mail.gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-16T16:43:49Z","receivedAt":"2025-04-16T16:44:01Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"> Something like this\n> So I think the argument values are making the difference but I'm still unsure.\n\nI think from ls-files it's how &pathspec values are received,\nit contains has_wildcard kinds of variables in the structure\n\n-Jayatheerth\n"},{"id":"516390","messageId":"CA+rGoLes1u3LZiStmDPmh4iiUNzNY43KyYNdLadQU+cy1TCc4A@mail.gmail.com","threadId":"63287","inReplyTo":"xmqqcydchz9o.fsf@gitster.g","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-19T04:59:46Z","receivedAt":"2025-04-19T04:59:58Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"test_expect_success 'commit wildcard pathspec limits commit' '\nreset_git_repo &&\ngit add . &&\ngit commit --allow-empty -m \"Test commit with * wildcard\" -- \"*\" &&\ncat >expected_files <<-\\EOF &&\n*\n**\n?\n[abc]\na\nf*\nf**\nf?z\nfoo*bar\nhello?world\nhello_world\nEOF\ngit ls-tree -r --name-only HEAD > actual_files &&\ntest_cmp expected_files actual_files\n'\n\n\nI get issues with these test cases,\nSpecially at git ls-tree -r --name-only HEAD > actual_files\n\nFor some reason the test_cmp doesn't get resolved\nIs it a bad way to do? or is there a better way?\n"},{"id":"516391","messageId":"9200B571-071D-4BBF-B5E3-886B5CCFB89A@gmail.com","threadId":"63287","inReplyTo":"CA+rGoLes1u3LZiStmDPmh4iiUNzNY43KyYNdLadQU+cy1TCc4A@mail.gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"Lucas Seiki Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2025-04-19T17:43:40Z","receivedAt":"2025-04-19T17:43:56Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Hi!\n\n> reset_git_repo &&\n\nI couldn't reproduce this as I don't have that function here. Is it a\ncommand that you defined locally?\n\n> I get issues with these test cases,\n> Specially at git ls-tree -r --name-only HEAD > actual_files\n> \n> For some reason the test_cmp doesn't get resolved\n\nYou can check the output of those commands by calling your test script\nwith the `-v` flag.\n\nIf `test_cmp a b` is false, you can check why by placing a temporary\n`diff a b` for displaying what changed, of course running with a `-v`.\n\n> Is it a bad way to do? or is there a better way?\n\nI don't see any problem based on that code snippet (except for the\nreset_git_repo that is not defined), but it looks like that there are\nother changes compared to the code you sent in the patch, so I don't\nhave enough context.\n\n"},{"id":"516491","messageId":"CA+rGoLe+K-yE5e6TgVOPbtpqinXrg7+k-thDBVTCtimzrWaG8A@mail.gmail.com","threadId":"63287","inReplyTo":"9200B571-071D-4BBF-B5E3-886B5CCFB89A@gmail.com","subject":"Re: [PATCH] t3706: Add test for wildcard vs literal pathspec","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-04-22T11:57:54Z","receivedAt":"2025-04-22T11:58:06Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"Hey Lucas\n\nThank you for the -v tag advice,\nfound the problem\n\nFixing and sending patch as soon as possible\n\nSorry for the late emails\n\nexams on head : \\\n"}]}