[PATCH] git-p4: add failing tests for case-folding in p4d

[PATCH] git-p4: add failing tests for case-folding in p4d

[Luke Diamand]

Lex found out recently that when git-p4 is asked to clone a repo
and the case of the repo is incorrect (but otherwise correct) that
git-p4, instead of reporting an error, appears to work fine, but
actually produces an empty repo. This can be quite confusing.

This patch adds a couple of failing test cases that illustrate the
problem.

The next step is to figure out where it's going wrong, and how it
should actually behave.

Luke Diamand (1):
  git-p4: add failing tests for case-folding p4d

 t/lib-git-p4.sh                |  2 +-
 t/t9819-git-p4-case-folding.sh | 54 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100755 t/t9819-git-p4-case-folding.sh

-- 
2.4.0.rc3.380.g8e2ddc7

[PATCH] git-p4: add failing tests for case-folding p4d

[Luke Diamand]

When p4d runs on a case-folding OS, git-p4 can end up getting
very confused. This adds failing tests to demonstrate the problem.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/lib-git-p4.sh                |  2 +-
 t/t9819-git-p4-case-folding.sh | 54 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100755 t/t9819-git-p4-case-folding.sh

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 5aa8adc..7548225 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -69,7 +69,7 @@ start_p4d() {
 	(
 		cd "$db" &&
 		{
-			p4d -q -p $P4DPORT &
+			p4d -q -p $P4DPORT "$@" &
 			echo $! >"$pidfile"
 		}
 	) &&
diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
new file mode 100755
index 0000000..78f1d0f
--- /dev/null
+++ b/t/t9819-git-p4-case-folding.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='interaction with P4 case-folding'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d with case folding enabled' '
+	start_p4d -C1
+'
+
+test_expect_success 'Create a repo, name is lowercase' '
+	(
+		client_view "//depot/... //client/..." &&
+		cd "$cli" &&
+		mkdir -p lc UC &&
+		>lc/file.txt && >UC/file.txt &&
+		p4 add lc/file.txt UC/file.txt &&
+		p4 submit -d "Add initial lc and UC repos"
+	)
+'
+
+test_expect_success 'Check p4 is in case-folding mode' '
+	(
+		cd "$cli" &&
+		>lc/FILE.TXT &&
+		p4 add lc/FILE.TXT &&
+		test_must_fail p4 submit -d "Cannot add file differing only in case" lc/FILE.TXT
+	)
+'
+
+# Check we created the repo properly
+test_expect_success 'Clone lc repo using lc name' '
+	git p4 clone //depot/lc/... &&
+	test_path_is_file lc/file.txt &&
+	git p4 clone //depot/UC/... &&
+	test_path_is_file UC/file.txt
+'
+
+# The clone should fail, since there is no repo called LC, but because
+# we have case-insensitive p4d enabled, it appears to go ahead and work,
+# but leaves an empty git repo in place.
+test_expect_failure 'Clone lc repo using uc name' '
+	test_must_fail git p4 clone //depot/LC/...
+'
+
+test_expect_failure 'Clone UC repo with lc name' '
+	test_must_fail git p4 clone //depot/uc/...
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.4.0.rc3.380.g8e2ddc7

Re: [PATCH] git-p4: add failing tests for case-folding p4d

[Lex Spoon]

The last comment in the test took me a minute to decipher. I would
suggest "no repo path called LC" instead of "no repo called LC". Also,
it would have helped me to either have a little comment on the "UC"
version of the test, or to make the previous comment a little more
neutral so that it will apply to both test cases.

Otherwise, while I am not a regular maintainer of this code, the patch
does LGTM. Certainly it's good to have more test coverage.

For the underlying problem, I haven't thought about it very much, but
it looks like a plausible first step might be to simply probe the
given file name and see if it comes back the same way. If it comes
back differently, then maybe the command should abort?


What a tough problem all around...

Lex

Re: [PATCH] git-p4: add failing tests for case-folding p4d

[Luke Diamand]

(Adding Pete, Vitor, and Fusion in case they have any thoughts on 
working with P4 servers that do case-folding, or at least failing 
gracefully).

On 29/04/15 00:01, Lex Spoon wrote:
> The last comment in the test took me a minute to decipher. I would
> suggest "no repo path called LC" instead of "no repo called LC". Also,
> it would have helped me to either have a little comment on the "UC"
> version of the test, or to make the previous comment a little more
> neutral so that it will apply to both test cases.

OK, thanks!

>
> Otherwise, while I am not a regular maintainer of this code, the patch
> does LGTM. Certainly it's good to have more test coverage.
>
> For the underlying problem, I haven't thought about it very much, but
> it looks like a plausible first step might be to simply probe the
> given file name and see if it comes back the same way. If it comes
> back differently, then maybe the command should abort?

I think the problem may be a bit trickier than that.

I think what's happening when cloning is that when files come back from 
the server, git-p4 checks that they are contained within the directory 
it is cloning. This happens in p4StartsWith(), (called from 
extractFilesFromCommit()) which already tries to fix this problem by 
checking 'core.ignorecase'. However, that won't work if the local 
machine is case sensitive but the server isn't (e.g. Linux client, 
Windows server).

git-p4 does this because it's fetching *commits* from Perforce, and a 
commit might have files that are outside the directory being cloned.

I tried teaching p4StartsWith() to ask the server if it is case-folding 
('p4 info') and that then means that the git-p4 clone actually succeeds. 
However, git-p4 submit then fails because it gets terribly confused 
about pathnames - it probably needs to do some lowercasing somewhere. So 
that might be worth pursuing.

Open to other suggestions though!

>
>
> What a tough problem all around...

Indeed!

Luke

Re: [PATCH] git-p4: add failing tests for case-folding in p4d

[Luke Diamand]

On 28/04/15 10:08, Luke Diamand wrote:
> Lex found out recently that when git-p4 is asked to clone a repo
> and the case of the repo is incorrect (but otherwise correct) that
> git-p4, instead of reporting an error, appears to work fine, but
> actually produces an empty repo. This can be quite confusing.

Oops, not Lex, but Fusion, noticed the problem!

Thanks!
Luke