| Message ID | xmqqwmplvbsa.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t4126: fix "funny directory name" test on Windows (again) | expand |
On Thu, Mar 28, 2024 at 10:37:25PM -0700, Junio C Hamano wrote: > Even though "git update-index --cacheinfo" ought to be filesystem > agnostic, somehow > > $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" > > fails only there. That unfortunately makes the approach of the > previous step unworkable. > > Resurrect the earlier approach to protect the test with a > prerequisite to make sure we do not needlessly fail the CI. I think this is a reasonable path forward. Looking at the patch itself... > -test_expect_success 'parsing a patch with no-contents and a funny pathname' ' > +test_expect_success 'setup patches in dir ending in SP' ' > + test_when_finished "rm -fr \"funny \"" && > + mkdir "funny " && > + >"funny /empty" && > + git add "funny /empty" && > + git diff HEAD -- "funny /" >sample.patch && > + git diff -R HEAD -- "funny /" >elpmas.patch && > git reset --hard && > - empty_blob=$(test_oid empty_blob) && > - echo "$empty_blob" >expect && > > - git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" && > - git diff --cached HEAD -- "funny /" >sample.patch && > - git diff --cached -R HEAD -- "funny /" >elpmas.patch && > - git reset && > + if grep "a/funny /empty b/funny /empty" sample.patch && > + grep "b/funny /empty a/funny /empty" elpmas.patch > + then > + test_set_prereq DIR_ENDS_WITH_SP > + else > + # Win test??? > + ls -l > + fi > +' It is a little funny that we set our prereq based only on the emptiness of the patches. That is certainly what happens on Windows, but it is a weird thing to expect. It implies that "mkdir" and "git add" returned success, but the latter without actually adding the index entry. That's what happens now, but given that such paths are forbidden by the filesystem, I'm not sure it's a good thing to rely on. If we think that Windows is the only problematic platform, should we just use !MINGW as the prereq? If we think there may be other platforms and would rather test the actual behavior, then we should probably be more careful. If "mkdir" fails above, we'd fail the test, rather than just not set the prereq. To solve that you can put the whole &&-chain into an if, but probably using a lazy prereq block might be more readable. I guess it is a little inefficient, though, because we have to actually add/diff to see if things are working. IMHO just using !MINGW would be simple, efficient, and effective (especially with "--cached", where we know that the problem is not the filesystem but our own is_valid_win32_path()). One other possible variant: we could skip the add/diff altogether and just include the patch as a test vector. After all, people on Windows could be sent such a patch without regard to their filesystem. That doesn't solve the whole issue, though, as "git apply" would fail (even with --cached) because of the verify_path() call. But you could still check the error output to confirm that we parsed the patch correctly. That lets every platform check the main bug fix, and more capable platforms test the whole process. Like: -- >8 -- apply_funny_path () { expect=$1; shift path=$1; shift if git apply --cached --stat --check --apply "$@" 2>err then if test "$expect" = "missing" then test_must_fail git rev-parse --verify "$path" else git rev-parse --verify ":$path" >actual && echo "$expect" >expect && test_cmp expect actual fi else # some platforms (like Windows) do not allow path entries with # trailing spaces, even just in the index. But we should # at least be able to verify that we parsed the patch # correctly. if test "$expect" = "missing" then echo "error: $path: does not exist in index" else echo "error: invalid path '$path'" fi >expect && test_cmp expect err fi } test_expect_success 'apply with no-contents and a funny pathname' ' empty=$(git rev-parse --verify :empty) && cat >sample.patch <<-EOF && diff --git a/funny /empty b/funny /empty new file mode 100644 index 0000000..$empty EOF cat >elpmas.patch <<-EOF && diff --git b/funny /empty a/funny /empty deleted file mode 100644 index e69de29..$empty EOF apply_funny_path $empty "funny /empty" sample.patch && apply_funny_path missing "funny /empty" elpmas.patch && apply_funny_path $empty "funny /empty" -R elpmas.patch && apply_funny_path missing "funny /empty" -R sample.patch ' -- 8< -- I didn't test it on Windows, but I did check that it does the right thing with a manual: diff --git a/read-cache.c b/read-cache.c index d1aef437aa..b75aeb0be8 100644 --- a/read-cache.c +++ b/read-cache.c @@ -976,6 +976,8 @@ static enum verify_path_result verify_path_internal(const char *path, if (has_dos_drive_prefix(path)) return PATH_INVALID; + if (strchr(path, ' ')) + return PATH_INVALID; if (!is_valid_path(path)) return PATH_INVALID; -Peff
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index 2462cdf904..d2ac7a486f 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -66,29 +66,38 @@ test_expect_success 'apply --index create' ' git diff --exit-code ' -test_expect_success 'parsing a patch with no-contents and a funny pathname' ' +test_expect_success 'setup patches in dir ending in SP' ' + test_when_finished "rm -fr \"funny \"" && + mkdir "funny " && + >"funny /empty" && + git add "funny /empty" && + git diff HEAD -- "funny /" >sample.patch && + git diff -R HEAD -- "funny /" >elpmas.patch && git reset --hard && - empty_blob=$(test_oid empty_blob) && - echo "$empty_blob" >expect && - git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" && - git diff --cached HEAD -- "funny /" >sample.patch && - git diff --cached -R HEAD -- "funny /" >elpmas.patch && - git reset && + if grep "a/funny /empty b/funny /empty" sample.patch && + grep "b/funny /empty a/funny /empty" elpmas.patch + then + test_set_prereq DIR_ENDS_WITH_SP + else + # Win test??? + ls -l + fi +' + +test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' ' - git apply --cached --stat --check --apply sample.patch && - git rev-parse --verify ":funny /empty" >actual && - test_cmp expect actual && + git apply --stat --check --apply sample.patch && + test_must_be_empty "funny /empty" && - git apply --cached --stat --check --apply elpmas.patch && - test_must_fail git rev-parse --verify ":funny /empty" && + git apply --stat --check --apply elpmas.patch && + test_path_is_missing "funny /empty" && - git apply -R --cached --stat --check --apply elpmas.patch && - git rev-parse --verify ":funny /empty" >actual && - test_cmp expect actual && + git apply -R --stat --check --apply elpmas.patch && + test_must_be_empty "funny /empty" && - git apply -R --cached --stat --check --apply sample.patch && - test_must_fail git rev-parse --verify ":funny /empty" + git apply -R --stat --check --apply sample.patch && + test_path_is_missing "funny /empty" ' test_done
Even though "git update-index --cacheinfo" ought to be filesystem agnostic, somehow $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" fails only there. That unfortunately makes the approach of the previous step unworkable. Resurrect the earlier approach to protect the test with a prerequisite to make sure we do not needlessly fail the CI. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t4126-apply-empty.sh | 43 +++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)