From 5ea0176003f4a5f4a2bffbe87b6ba2365bf1c254 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 19 Mar 2024 15:56:44 -0700 Subject: [PATCH 1/3] apply: parse names out of "diff --git" more carefully "git apply" uses the pathname parsed out of the "diff --git" header to decide which path is being patched, but this is used only when there is no other names available in the patch. When there is any content change (like we can see in this patch, that modifies the contents of "apply.c") or rename (which comes with "rename from" and "rename to" extended diff headers), the names are available without having to parse this header. When we do need to parse this header, a special care needs to be taken, as the name of a directory or a file can have a SP in it so it is not like "find a space, and take everything before the space and that is the preimage filename, everything after the space is the postimage filename". We have a loop that stops at every SP on the "diff --git a/dir/file b/dir/foo" line and see if that SP is the right place that separates such a pair of names. Unfortunately, this loop can terminate prematurely when a crafted directory name ended with a SP. The next pathname component after that SP (i.e. the beginning of the possible postimage filename) will be a slash, and instead of rejecting that position as the valid separation point between pre- and post-image filenames and keep looping, we stopped processing right there. The fix is simple. Instead of stopping and giving up, keep going on when we see such a condition. Reported-by: Han Young Signed-off-by: Junio C Hamano --- apply.c | 9 ++++++++- t/t4126-apply-empty.sh | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/apply.c b/apply.c index 976215785d..785e055949 100644 --- a/apply.c +++ b/apply.c @@ -1277,8 +1277,15 @@ static char *git_header_name(int p_value, return NULL; /* no postimage name */ second = skip_tree_prefix(p_value, name + len + 1, line_len - (len + 1)); + /* + * If we are at the SP at the end of a directory, + * skip_tree_prefix() may return NULL as that makes + * it appears as if we have an absolute path. + * Keep going to find another SP. + */ if (!second) - return NULL; + continue; + /* * Does len bytes starting at "name" and "second" * (that are separated by one HT or SP we just diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index ece9fae207..eaf0c5304a 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -66,4 +66,26 @@ test_expect_success 'apply --index create' ' git diff --exit-code ' +test_expect_success 'apply with no-contents and a funny pathname' ' + mkdir "funny " && + >"funny /empty" && + git add "funny /empty" && + git diff HEAD "funny /" >sample.patch && + git diff -R HEAD "funny /" >elpmas.patch && + git reset --hard && + rm -fr "funny " && + + git apply --stat --check --apply sample.patch && + test_must_be_empty "funny /empty" && + + git apply --stat --check --apply elpmas.patch && + test_path_is_missing "funny /empty" && + + git apply -R --stat --check --apply elpmas.patch && + test_must_be_empty "funny /empty" && + + git apply -R --stat --check --apply sample.patch && + test_path_is_missing "funny /empty" +' + test_done From 012c8b307d339873bcdbbe95018ccfff904fc501 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 28 Mar 2024 14:08:47 -0700 Subject: [PATCH 2/3] t4126: make sure a directory with SP at the end is usable As afb31ad9 (t1010: fix unnoticed failure on Windows, 2021-12-11) said: On Microsoft Windows, a directory name should never end with a period. Quoting from Microsoft documentation[1]: Do not end a file or directory name with a space or a period. Although the underlying file system may support such names, the Windows shell and user interface does not. [1]: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file and the condition addressed by this change is exactly that. If the platform is unable to properly create these sample patches about a file that lives in a directory whose name ends with a SP, there is no point testing how "git apply" behaves there on the filesystem. Even though the ultimate purpose of "git apply" is to apply a patch and to update the filesystem entities, this particular test is mainly about parsing a patch on a funny pathname correctly, and even on a system that is incapable of checking out the resulting state correctly on its filesystem, at least the parsing can and should work fine. Rewrite the test to work inside the index without touching the filesystem. Helped-by: Jeff King Helped-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/t4126-apply-empty.sh | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index eaf0c5304a..2462cdf904 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -66,26 +66,29 @@ test_expect_success 'apply --index create' ' git diff --exit-code ' -test_expect_success 'apply with no-contents and a funny pathname' ' - mkdir "funny " && - >"funny /empty" && - git add "funny /empty" && - git diff HEAD "funny /" >sample.patch && - git diff -R HEAD "funny /" >elpmas.patch && +test_expect_success 'parsing a patch with no-contents and a funny pathname' ' git reset --hard && - rm -fr "funny " && + empty_blob=$(test_oid empty_blob) && + echo "$empty_blob" >expect && - git apply --stat --check --apply sample.patch && - test_must_be_empty "funny /empty" && + 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 && - git apply --stat --check --apply elpmas.patch && - test_path_is_missing "funny /empty" && + git apply --cached --stat --check --apply sample.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 --cached --stat --check --apply elpmas.patch && + test_must_fail git rev-parse --verify ":funny /empty" && - git apply -R --stat --check --apply sample.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 --cached --stat --check --apply sample.patch && + test_must_fail git rev-parse --verify ":funny /empty" ' test_done From 776ffd1a303afa8cf581d62e9d0478d112fecdd2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 29 Mar 2024 10:21:58 -0700 Subject: [PATCH 3/3] t4126: fix "funny directory name" test on Windows (again) Even though "git update-index --cacheinfo" ought to be filesystem agnostic, $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" fails only on Windows, and this unfortunately makes the approach of the previous step unworkable. Resurrect the earlier approach to give up on running the test on known-bad platforms. Instead of computing a custom prerequisite, just use !MINGW we have used elsewhere. Signed-off-by: Junio C Hamano --- t/t4126-apply-empty.sh | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh index 2462cdf904..56210b5609 100755 --- a/t/t4126-apply-empty.sh +++ b/t/t4126-apply-empty.sh @@ -66,29 +66,28 @@ 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 !MINGW 'apply with no-contents and a funny pathname' ' + test_when_finished "rm -fr \"funny \"; git reset --hard" && + + 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 && + git apply --stat --check --apply sample.patch && + test_must_be_empty "funny /empty" && - git apply --cached --stat --check --apply sample.patch && - git rev-parse --verify ":funny /empty" >actual && - test_cmp expect actual && + git apply --stat --check --apply elpmas.patch && + test_path_is_missing "funny /empty" && - git apply --cached --stat --check --apply elpmas.patch && - test_must_fail git rev-parse --verify ":funny /empty" && + git apply -R --stat --check --apply elpmas.patch && + test_must_be_empty "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 --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