From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>,
Han Young <hanyang.tony@bytedance.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] t4126: fix "funny directory name" test on Windows (again)
Date: Fri, 29 Mar 2024 08:00:38 -0400 [thread overview]
Message-ID: <20240329120038.GB15842@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqwmplvbsa.fsf_-_@gitster.g>
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
next prev parent reply other threads:[~2024-03-29 12:00 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-19 9:52 [PATCH 0/1] quote: quote space Han Young
2024-03-19 9:52 ` [PATCH 1/1] " Han Young
2024-03-19 9:59 ` Kristoffer Haugsbakk
2024-03-19 15:15 ` [PATCH 0/1] " Junio C Hamano
2024-03-19 22:56 ` Junio C Hamano
2024-03-26 21:41 ` Junio C Hamano
2024-03-27 9:17 ` Jeff King
2024-03-27 14:59 ` Junio C Hamano
2024-03-27 22:11 ` Junio C Hamano
2024-03-28 10:32 ` Jeff King
2024-03-28 11:40 ` Jeff King
2024-03-28 17:05 ` Eric Sunshine
2024-03-28 17:31 ` Junio C Hamano
2024-03-28 21:08 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Junio C Hamano
2024-03-29 2:18 ` Junio C Hamano
2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano
2024-03-29 12:00 ` Jeff King [this message]
2024-03-29 17:21 ` [PATCH v2] " Junio C Hamano
2024-03-29 18:34 ` Jeff King
2024-03-29 11:27 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Jeff King
2024-03-29 17:01 ` Junio C Hamano
2024-04-27 14:47 ` Johannes Schindelin
2024-04-27 17:20 ` Junio C Hamano
2024-03-28 16:19 ` [PATCH 0/1] quote: quote space Junio C Hamano
2024-03-28 16:30 ` Jeff King
2024-03-28 16:53 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240329120038.GB15842@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hanyang.tony@bytedance.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).