Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
On Wednesday 03 January 2018 at 08:14 pm +0100, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jan 03 2018, Adam Dinwoodie jotted: > > > On Wednesday 03 January 2018 at 02:31 pm +0100, Ævar Arnfjörð Bjarmason > > wrote: > >> Does the fixup above in <878tdm8k2d@evledraar.gmail.com> work for > >> you, i.e. changing $10 in the script to ${10}? > > > > This fixes some but not all of the failures: I'm now down from 42 to 24 > > failures. > > > > Updated verbose test output is at > > https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02 > > Thanks lot, looking through our own commit logs I believe the rest > should be fixed by this (prior art in 6fd1106aa4), it would be great if > you could test it, I don't have access to a Windows machine: > > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh > index f985139b6f..5838fcb77d 100755 > --- a/t/t3070-wildmatch.sh > +++ b/t/t3070-wildmatch.sh > @@ -23,6 +23,15 @@ create_test_file() { > *//*) > return 1 > ;; > + # On Windows, \ in paths is silently converted to /, which > + # would result in the "touch" below working, but the test > + # itself failing. > + *\\*) > + if ! test_have_prereq BSLASHPSPEC > + then > + return 1 > + fi > + ;; > # When testing the difference between foo/bar and foo/bar/ we > # can't test the latter. > */) Confirmed this fixes all the outstanding test failures. Thank you!
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
On Wed, Jan 03 2018, Adam Dinwoodie jotted: > On Wednesday 03 January 2018 at 02:31 pm +0100, Ævar Arnfjörð Bjarmason wrote: >> >> On Wed, Jan 03 2018, Adam Dinwoodie jotted: >> >> > On Monday 25 December 2017 at 12:28 am +, Ævar Arnfjörð Bjarmason >> > wrote: >> >> There has never been any full roundtrip testing of what git-ls-files >> >> and other functions that use wildmatch() actually do, rather we've >> >> been satisfied with just testing the underlying C function. >> >> >> >> Due to git-ls-files and friends having their own codepaths before they >> >> call wildmatch() there's sometimes differences in the behavior between >> >> the two, and even when we test for those (as with >> >> 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06)) >> >> there was no one place where you can review how these two modes >> >> differ. >> >> >> >> Now there is. We now attempt to create a file called $haystack and >> >> match $needle against it for each pair of $needle and $haystack that >> >> we were passing to test-wildmatch. >> >> >> >> If we can't create the file we skip the test. This ensures that we can >> >> run this on all platforms and not maintain some infinitely growing >> >> whitelist of e.g. platforms that don't support certain characters in >> >> filenames. >> >> >> >> As a result of doing this we can now see the cases where these two >> >> ways of testing wildmatch differ: >> >> >> >> * Creating a file called 'a[]b' and running ls-files 'a[]b' will show >> >>that file, but wildmatch("a[]b", "a[]b") will not match >> >> >> >> * wildmatch() won't match a file called \ against \, but ls-files >> >>will. >> >> >> >> * `git --glob-pathspecs ls-files 'foo**'` will match a file >> >>'foo/bba/arr', but wildmatch won't, however pathmatch will. >> >> >> >>This seems like a bug to me, the two are otherwise equivalent as >> >>these tests show. >> >> >> >> This also reveals the case discussed in 9e4e8a64c2 above, where '' is >> >> now an error as far as ls-files is concerned, but wildmatch() itself >> >> happily accepts it. >> >> >> >> Signed-off-by: Ævar Arnfjörð Bjarmason>> > >> > I'm seeing this test script failing on the pu branch as a result of this >> > commit when building on Cygwin. Specifically, the test fails at >> > 9d45e1ca4 ("Merge branch 'bw/oidmap-autoinit' into pu", 2017-12-28), and >> > bisecting points the blame at 2ee0c785a ("wildmatch test: create & test >> > files on disk in addition to in-memory", 2017-12-25). >> > >> > I've copied the verbose error output for the first error below, and >> > uploaded the full output, including verbose and trace output for the >> > unexpectedly failing tests, at [0]. (With 42 failures among 1512 tests, >> > there's a lot of it, so I didn't want to include it in an email.) >> >> Does the fixup above in <878tdm8k2d@evledraar.gmail.com> work for >> you, i.e. changing $10 in the script to ${10}? > > This fixes some but not all of the failures: I'm now down from 42 to 24 > failures. > > Updated verbose test output is at > https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02 Thanks lot, looking through our own commit logs I believe the rest should be fixed by this (prior art in 6fd1106aa4), it would be great if you could test it, I don't have access to a Windows machine: diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh index f985139b6f..5838fcb77d 100755 --- a/t/t3070-wildmatch.sh +++ b/t/t3070-wildmatch.sh @@ -23,6 +23,15 @@ create_test_file() { *//*) return 1 ;; + # On Windows, \ in paths is silently converted to /, which + # would result in the "touch" below working, but the test + # itself failing. + *\\*) + if ! test_have_prereq BSLASHPSPEC + then + return 1 + fi + ;; # When testing the difference between foo/bar and foo/bar/ we # can't test the latter. */)
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
On Wednesday 03 January 2018 at 02:31 pm +0100, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Jan 03 2018, Adam Dinwoodie jotted: > > > On Monday 25 December 2017 at 12:28 am +, Ævar Arnfjörð Bjarmason wrote: > >> There has never been any full roundtrip testing of what git-ls-files > >> and other functions that use wildmatch() actually do, rather we've > >> been satisfied with just testing the underlying C function. > >> > >> Due to git-ls-files and friends having their own codepaths before they > >> call wildmatch() there's sometimes differences in the behavior between > >> the two, and even when we test for those (as with > >> 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06)) > >> there was no one place where you can review how these two modes > >> differ. > >> > >> Now there is. We now attempt to create a file called $haystack and > >> match $needle against it for each pair of $needle and $haystack that > >> we were passing to test-wildmatch. > >> > >> If we can't create the file we skip the test. This ensures that we can > >> run this on all platforms and not maintain some infinitely growing > >> whitelist of e.g. platforms that don't support certain characters in > >> filenames. > >> > >> As a result of doing this we can now see the cases where these two > >> ways of testing wildmatch differ: > >> > >> * Creating a file called 'a[]b' and running ls-files 'a[]b' will show > >>that file, but wildmatch("a[]b", "a[]b") will not match > >> > >> * wildmatch() won't match a file called \ against \, but ls-files > >>will. > >> > >> * `git --glob-pathspecs ls-files 'foo**'` will match a file > >>'foo/bba/arr', but wildmatch won't, however pathmatch will. > >> > >>This seems like a bug to me, the two are otherwise equivalent as > >>these tests show. > >> > >> This also reveals the case discussed in 9e4e8a64c2 above, where '' is > >> now an error as far as ls-files is concerned, but wildmatch() itself > >> happily accepts it. > >> > >> Signed-off-by: Ævar Arnfjörð Bjarmason> > > > I'm seeing this test script failing on the pu branch as a result of this > > commit when building on Cygwin. Specifically, the test fails at > > 9d45e1ca4 ("Merge branch 'bw/oidmap-autoinit' into pu", 2017-12-28), and > > bisecting points the blame at 2ee0c785a ("wildmatch test: create & test > > files on disk in addition to in-memory", 2017-12-25). > > > > I've copied the verbose error output for the first error below, and > > uploaded the full output, including verbose and trace output for the > > unexpectedly failing tests, at [0]. (With 42 failures among 1512 tests, > > there's a lot of it, so I didn't want to include it in an email.) > > Does the fixup above in <878tdm8k2d@evledraar.gmail.com> work for > you, i.e. changing $10 in the script to ${10}? This fixes some but not all of the failures: I'm now down from 42 to 24 failures. Updated verbose test output is at https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02 Thanks! Adam
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
On Wed, Jan 03 2018, Adam Dinwoodie jotted: > On Monday 25 December 2017 at 12:28 am +, Ævar Arnfjörð Bjarmason wrote: >> There has never been any full roundtrip testing of what git-ls-files >> and other functions that use wildmatch() actually do, rather we've >> been satisfied with just testing the underlying C function. >> >> Due to git-ls-files and friends having their own codepaths before they >> call wildmatch() there's sometimes differences in the behavior between >> the two, and even when we test for those (as with >> 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06)) >> there was no one place where you can review how these two modes >> differ. >> >> Now there is. We now attempt to create a file called $haystack and >> match $needle against it for each pair of $needle and $haystack that >> we were passing to test-wildmatch. >> >> If we can't create the file we skip the test. This ensures that we can >> run this on all platforms and not maintain some infinitely growing >> whitelist of e.g. platforms that don't support certain characters in >> filenames. >> >> As a result of doing this we can now see the cases where these two >> ways of testing wildmatch differ: >> >> * Creating a file called 'a[]b' and running ls-files 'a[]b' will show >>that file, but wildmatch("a[]b", "a[]b") will not match >> >> * wildmatch() won't match a file called \ against \, but ls-files >>will. >> >> * `git --glob-pathspecs ls-files 'foo**'` will match a file >>'foo/bba/arr', but wildmatch won't, however pathmatch will. >> >>This seems like a bug to me, the two are otherwise equivalent as >>these tests show. >> >> This also reveals the case discussed in 9e4e8a64c2 above, where '' is >> now an error as far as ls-files is concerned, but wildmatch() itself >> happily accepts it. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason> > I'm seeing this test script failing on the pu branch as a result of this > commit when building on Cygwin. Specifically, the test fails at > 9d45e1ca4 ("Merge branch 'bw/oidmap-autoinit' into pu", 2017-12-28), and > bisecting points the blame at 2ee0c785a ("wildmatch test: create & test > files on disk in addition to in-memory", 2017-12-25). > > I've copied the verbose error output for the first error below, and > uploaded the full output, including verbose and trace output for the > unexpectedly failing tests, at [0]. (With 42 failures among 1512 tests, > there's a lot of it, so I didn't want to include it in an email.) Does the fixup above in <878tdm8k2d@evledraar.gmail.com> work for you, i.e. changing $10 in the script to ${10}? I was waiting on Junio to see if he'd like to just squash that or if I should re-send. > expecting success: > wildtest_file_setup && > printf '%s' '\' >expect && > git --glob-pathspecs ls-files -z -- '00' > >actual.raw 2>actual.err && > wildtest_stdout_stderr_cmp > > ++ wildtest_file_setup > ++ test_when_finished ' > rm -rf -- * && > git reset > ' > ++ test 0 = 0 > ++ test_cleanup='{ > rm -rf -- * && > git reset > > } && (exit "$eval_ret"); eval_ret=$?; :' > ++ git add -A > ++ printf %s '\' > ++ git --glob-pathspecs ls-files -z -- 00 > ++ wildtest_stdout_stderr_cmp > ++ tr -d '\0' > ++ test_cmp expect.err actual.err > ++ diff -u expect.err actual.err > ++ test_cmp expect actual > ++ diff -u expect actual > --- expect 2018-01-03 12:43:11.116611200 + > +++ actual 2018-01-03 12:43:11.216180400 + > @@ -1 +0,0 @@ > -\ > \ No newline at end of file > + test_eval_ret_=1 > + want_trace > + test t = t > + test t = t > + set +x > error: last command exited with $?=1 > ++ rm -rf -- actual actual.err actual.raw expect expect.err > ++ git reset > ++ exit 1 > ++ eval_ret=1 > ++ : > + test_eval_ret_=0 > + want_trace > + test t = t > + test t = t > + set +x > not ok 490 - wildmatch(ls): match '00' '\' > # > # wildtest_file_setup && > # printf '%s' '\' >expect && > # git --glob-pathspecs ls-files -z > -- '00' >actual.raw 2>actual.err && > # wildtest_stdout_stderr_cmp > # > > I'm digging into the failures myself now, but wanted to report the > problem in the name of getting more eyes on it. > > Adam > > [0]: https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
On Monday 25 December 2017 at 12:28 am +, Ævar Arnfjörð Bjarmason wrote: > There has never been any full roundtrip testing of what git-ls-files > and other functions that use wildmatch() actually do, rather we've > been satisfied with just testing the underlying C function. > > Due to git-ls-files and friends having their own codepaths before they > call wildmatch() there's sometimes differences in the behavior between > the two, and even when we test for those (as with > 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06)) > there was no one place where you can review how these two modes > differ. > > Now there is. We now attempt to create a file called $haystack and > match $needle against it for each pair of $needle and $haystack that > we were passing to test-wildmatch. > > If we can't create the file we skip the test. This ensures that we can > run this on all platforms and not maintain some infinitely growing > whitelist of e.g. platforms that don't support certain characters in > filenames. > > As a result of doing this we can now see the cases where these two > ways of testing wildmatch differ: > > * Creating a file called 'a[]b' and running ls-files 'a[]b' will show >that file, but wildmatch("a[]b", "a[]b") will not match > > * wildmatch() won't match a file called \ against \, but ls-files >will. > > * `git --glob-pathspecs ls-files 'foo**'` will match a file >'foo/bba/arr', but wildmatch won't, however pathmatch will. > >This seems like a bug to me, the two are otherwise equivalent as >these tests show. > > This also reveals the case discussed in 9e4e8a64c2 above, where '' is > now an error as far as ls-files is concerned, but wildmatch() itself > happily accepts it. > > Signed-off-by: Ævar Arnfjörð BjarmasonI'm seeing this test script failing on the pu branch as a result of this commit when building on Cygwin. Specifically, the test fails at 9d45e1ca4 ("Merge branch 'bw/oidmap-autoinit' into pu", 2017-12-28), and bisecting points the blame at 2ee0c785a ("wildmatch test: create & test files on disk in addition to in-memory", 2017-12-25). I've copied the verbose error output for the first error below, and uploaded the full output, including verbose and trace output for the unexpectedly failing tests, at [0]. (With 42 failures among 1512 tests, there's a lot of it, so I didn't want to include it in an email.) expecting success: wildtest_file_setup && printf '%s' '\' >expect && git --glob-pathspecs ls-files -z -- '00' >actual.raw 2>actual.err && wildtest_stdout_stderr_cmp ++ wildtest_file_setup ++ test_when_finished ' rm -rf -- * && git reset ' ++ test 0 = 0 ++ test_cleanup='{ rm -rf -- * && git reset } && (exit "$eval_ret"); eval_ret=$?; :' ++ git add -A ++ printf %s '\' ++ git --glob-pathspecs ls-files -z -- 00 ++ wildtest_stdout_stderr_cmp ++ tr -d '\0' ++ test_cmp expect.err actual.err ++ diff -u expect.err actual.err ++ test_cmp expect actual ++ diff -u expect actual --- expect 2018-01-03 12:43:11.116611200 + +++ actual 2018-01-03 12:43:11.216180400 + @@ -1 +0,0 @@ -\ \ No newline at end of file + test_eval_ret_=1 + want_trace + test t = t + test t = t + set +x error: last command exited with $?=1 ++ rm -rf -- actual actual.err actual.raw expect expect.err ++ git reset ++ exit 1 ++ eval_ret=1 ++ : + test_eval_ret_=0 + want_trace + test t = t + test t = t + set +x not ok 490 - wildmatch(ls): match '00' '\' # # wildtest_file_setup && # printf '%s' '\' >expect && # git --glob-pathspecs ls-files -z -- '00' >actual.raw 2>actual.err && # wildtest_stdout_stderr_cmp # I'm digging into the failures myself now, but wanted to report the problem in the name of getting more eyes on it. Adam [0]: https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
Ævar Arnfjörð Bjarmasonwrites: > + # Turn foo/bar/baz into foo/bar to create foo/bar as a > + # directory structure. > + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!') Let's not limit this script to GNUism systems. You want to see if $file has a slash followed by one or more non-slash letters to the end and strip the whole thing. We further know that at this point $file does not end with a slash. So perhaps dirs=${file%/*} is sufficient? Thanks.
Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
Am 25.12.2017 um 01:28 schrieb Ævar Arnfjörð Bjarmason: +create_test_file() { + file=$1 + + case $file in + # `touch .` will succeed but obviously not do what we intend + # here. + ".") + return 1 + ;; + # We cannot create a file with an empty filename. + "") + return 1 + ;; + # The tests that are testing that e.g. foo//bar is matched by + # foo/*/bar can't be tested on filesystems since there's no + # way we're getting a double slash. + *//*) + return 1 + ;; + # When testing the difference between foo/bar and foo/bar/ we + # can't test the latter. + */) + return 1 + ;; + esac Nice! + + # Turn foo/bar/baz into foo/bar to create foo/bar as a + # directory structure. + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!') dirs=${file%/*} should do the same without forking processes, no? -- Hannes
[PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory
There has never been any full roundtrip testing of what git-ls-files and other functions that use wildmatch() actually do, rather we've been satisfied with just testing the underlying C function. Due to git-ls-files and friends having their own codepaths before they call wildmatch() there's sometimes differences in the behavior between the two, and even when we test for those (as with 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06)) there was no one place where you can review how these two modes differ. Now there is. We now attempt to create a file called $haystack and match $needle against it for each pair of $needle and $haystack that we were passing to test-wildmatch. If we can't create the file we skip the test. This ensures that we can run this on all platforms and not maintain some infinitely growing whitelist of e.g. platforms that don't support certain characters in filenames. As a result of doing this we can now see the cases where these two ways of testing wildmatch differ: * Creating a file called 'a[]b' and running ls-files 'a[]b' will show that file, but wildmatch("a[]b", "a[]b") will not match * wildmatch() won't match a file called \ against \, but ls-files will. * `git --glob-pathspecs ls-files 'foo**'` will match a file 'foo/bba/arr', but wildmatch won't, however pathmatch will. This seems like a bug to me, the two are otherwise equivalent as these tests show. This also reveals the case discussed in 9e4e8a64c2 above, where '' is now an error as far as ls-files is concerned, but wildmatch() itself happily accepts it. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t3070-wildmatch.sh | 285 --- 1 file changed, 273 insertions(+), 12 deletions(-) diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh index 593b25b278..80d13b5b60 100755 --- a/t/t3070-wildmatch.sh +++ b/t/t3070-wildmatch.sh @@ -4,14 +4,95 @@ test_description='wildmatch tests' . ./test-lib.sh +create_test_file() { + file=$1 + + case $file in + # `touch .` will succeed but obviously not do what we intend + # here. + ".") + return 1 + ;; + # We cannot create a file with an empty filename. + "") + return 1 + ;; + # The tests that are testing that e.g. foo//bar is matched by + # foo/*/bar can't be tested on filesystems since there's no + # way we're getting a double slash. + *//*) + return 1 + ;; + # When testing the difference between foo/bar and foo/bar/ we + # can't test the latter. + */) + return 1 + ;; + esac + + # Turn foo/bar/baz into foo/bar to create foo/bar as a + # directory structure. + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!') + + # We touch "./$file" instead of "$file" because even an + # escaped "touch -- -" means get arguments from stdin. + if test "$file" != "$dirs" + then + mkdir -p -- "$dirs" && + touch -- "./$file" && + return 0 + else + touch -- "./$file" && + return 0 + fi + return 1 +} + +wildtest_file_setup() { + test_when_finished " + rm -rf -- * && + git reset + " && + git add -A && + >expect.err +} + +wildtest_stdout_stderr_cmp() { + tr -d '\0' actual && + test_cmp expect.err actual.err && + test_cmp expect actual +} + wildtest() { - match_w_glob=$1 - match_w_globi=$2 - match_w_pathmatch=$3 - match_w_pathmatchi=$4 - text=$5 - pattern=$6 + if test "$#" = 6 + then + # When test-wildmatch and git ls-files produce the same + # result. + match_w_glob=$1 + match_f_w_glob=$match_w_glob + match_w_globi=$2 + match_f_w_globi=$match_w_globi + match_w_pathmatch=$3 + match_f_w_pathmatch=$match_w_pathmatch + match_w_pathmatchi=$4 + match_f_w_pathmatchi=$match_w_pathmatchi + text=$5 + pattern=$6 + elif test "$#" = 10 + then + match_w_glob=$1 + match_w_globi=$2 + match_w_pathmatch=$3 + match_w_pathmatchi=$4 + match_f_w_glob=$5 + match_f_w_globi=$6 + match_f_w_pathmatch=$7 + match_f_w_pathmatchi=$8 + text=$9 + pattern=$10 + fi + # $1: Case sensitive glob match: test-wildmatch if test "$match_w_glob" = 1 then test_expect_success "wildmatch: match '$text' '$pattern'" " @@ -26,6 +107,50 @@ wildtest() { test_expect_success "PANIC: Test framework error. Unknown matches value