[PATCH v6] git-p4: Obey core.ignorecase when using P4 client specs.

[PATCH v6] git-p4: Obey core.ignorecase when using P4 client specs.

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

We run P4 servers on Linux and P4 clients on Windows. For an unknown
reason the file path for a number of files in P4 does not match the
directory path with respect to case sensitivity.

E.g. "p4 files" might return
//depot/path/to/file1
//depot/pATH/to/file2

If you use P4/P4V then these files end up in the same directory, e.g.
//depot/path/to/file1
//depot/path/to/file2

If you use git-p4 and clone the code via client spec "//depot/path/..."
then all files not matching the case in the client spec will be ignored
(in the example above "file2"). This is correct if core.ignorecase=false
but not otherwise.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py                         |   7 ++
 t/t9821-git-p4-path-variations.sh | 200 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)
 create mode 100755 t/t9821-git-p4-path-variations.sh

diff --git a/git-p4.py b/git-p4.py
index 073f87b..0093fa3 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1950,10 +1950,14 @@ class View(object):
             if "unmap" in res:
                 # it will list all of them, but only one not unmap-ped
                 continue
+            if gitConfigBool("core.ignorecase"):
+                res['depotFile'] = res['depotFile'].lower()
             self.client_spec_path_cache[res['depotFile']] = self.convert_client_path(res["clientFile"])
 
         # not found files or unmap files set to ""
         for depotFile in fileArgs:
+            if gitConfigBool("core.ignorecase"):
+                depotFile = depotFile.lower()
             if depotFile not in self.client_spec_path_cache:
                 self.client_spec_path_cache[depotFile] = ""
 
@@ -1962,6 +1966,9 @@ class View(object):
            depot file should live.  Returns "" if the file should
            not be mapped in the client."""
 
+        if gitConfigBool("core.ignorecase"):
+            depot_path = depot_path.lower()
+
         if depot_path in self.client_spec_path_cache:
             return self.client_spec_path_cache[depot_path]
 
diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
new file mode 100755
index 0000000..599d16c
--- /dev/null
+++ b/t/t9821-git-p4-path-variations.sh
@@ -0,0 +1,200 @@
+#!/bin/sh
+
+test_description='Clone repositories with path case variations'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d with case folding enabled' '
+	start_p4d -C1
+'
+
+test_expect_success 'Create a repo with path case variations' '
+	client_view "//depot/... //client/..." &&
+	(
+		cd "$cli" &&
+
+		mkdir -p One/two &&
+		>One/two/File2.txt &&
+		p4 add One/two/File2.txt &&
+		p4 submit -d "Add file2" &&
+		rm -rf One &&
+
+		mkdir -p one/TWO &&
+		>one/TWO/file1.txt &&
+		p4 add one/TWO/file1.txt &&
+		p4 submit -d "Add file1" &&
+		rm -rf one &&
+
+		mkdir -p one/two &&
+		>one/two/file3.txt &&
+		p4 add one/two/file3.txt &&
+		p4 submit -d "Add file3" &&
+		rm -rf one &&
+
+		mkdir -p outside-spec &&
+		>outside-spec/file4.txt &&
+		p4 add outside-spec/file4.txt &&
+		p4 submit -d "Add file4" &&
+		rm -rf outside-spec
+	)
+'
+
+test_expect_success 'Clone root' '
+	client_view "//depot/... //client/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase false &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		# This method is used instead of "test -f" to ensure the case is
+		# checked even if the test is executed on case-insensitive file systems.
+		# All files are there as expected although the path cases look random.
+		cat >expect <<-\EOF &&
+		One/two/File2.txt
+		one/TWO/file1.txt
+		one/two/file3.txt
+		outside-spec/file4.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone root (ignorecase)' '
+	client_view "//depot/... //client/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase true &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		# This method is used instead of "test -f" to ensure the case is
+		# checked even if the test is executed on case-insensitive file systems.
+		# All files are there as expected although the path cases look random.
+		cat >expect <<-\EOF &&
+		one/TWO/File2.txt
+		one/TWO/file1.txt
+		one/TWO/file3.txt
+		outside-spec/file4.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone root and ignore one file' '
+	client_view \
+		"//depot/... //client/..." \
+		"-//depot/one/TWO/file1.txt //client/one/TWO/file1.txt" &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase false &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		# We ignore one file in the client spec and all path cases change from
+		# "TWO" to "two"!
+		cat >expect <<-\EOF &&
+		One/two/File2.txt
+		one/two/file3.txt
+		outside-spec/file4.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone root and ignore one file (ignorecase)' '
+	client_view \
+		"//depot/... //client/..." \
+		"-//depot/one/TWO/file1.txt //client/one/TWO/file1.txt" &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase true &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		# We ignore one file in the client spec and all path cases change from
+		# "TWO" to "two"!
+		cat >expect <<-\EOF &&
+		One/two/File2.txt
+		One/two/file3.txt
+		outside-spec/file4.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone path' '
+	client_view "//depot/One/... //client/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase false &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		cat >expect <<-\EOF &&
+		two/File2.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'Clone path (ignorecase)' '
+	client_view "//depot/One/... //client/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase true &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		cat >expect <<-\EOF &&
+		TWO/File2.txt
+		TWO/file1.txt
+		TWO/file3.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+# It looks like P4 determines the path case based on the first file in
+# lexicographical order. Please note the lower case "two" directory for all
+# files triggered through the addition of "File0.txt".
+test_expect_success 'Add a new file and clone path with new file (ignorecase)' '
+	client_view "//depot/... //client/..." &&
+	(
+		cd "$cli" &&
+		mkdir -p One/two &&
+		>One/two/File0.txt &&
+		p4 add One/two/File0.txt &&
+		p4 submit -d "Add file" &&
+		rm -rf One
+	) &&
+
+	client_view "//depot/One/... //client/..." &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git init . &&
+		git config core.ignorecase true &&
+		git p4 clone --use-client-spec --destination="$git" //depot &&
+		cat >expect <<-\EOF &&
+		two/File0.txt
+		two/File2.txt
+		two/file1.txt
+		two/file3.txt
+		EOF
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.9.5 (Apple Git-50.3)

Re: [PATCH v6] git-p4: Obey core.ignorecase when using P4 client specs.

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> We run P4 servers on Linux and P4 clients on Windows. For an unknown
> reason the file path for a number of files in P4 does not match the
> directory path with respect to case sensitivity.

Thanks, but is this still "For an unknown reason", or during the
course of debugging you found the root cause, which is what led to
this fix?

>
> E.g. "p4 files" might return
> //depot/path/to/file1
> //depot/pATH/to/file2
>
> If you use P4/P4V then these files end up in the same directory, e.g.
> //depot/path/to/file1
> //depot/path/to/file2
>
> If you use git-p4 and clone the code via client spec "//depot/path/..."
> then all files not matching the case in the client spec will be ignored
> (in the example above "file2"). This is correct if core.ignorecase=false
> but not otherwise.

This sentence is hard to grok.  What are you describing?  Solution?
Current problematic behaviour?  Desired behaviour that the patch
attempts to obtain?

If I paraphrase it like this, did I understood you correctly?

    The current code always ignores paths in wrong case that do not
    match client spec.  It is correct to ignore them when
    core.ignorecase is not set; //depot/path/ and //depot/pATH/ are
    different things in that case.

    But it is a wrong thing to do if core.ignorecase is set to true.
    Let's make sure we avoid it by downcasing the depot_path when
    core.ignorecase is set to true.



>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> Acked-by: Luke Diamand <luke@diamand.org>
> ---
>  git-p4.py                         |   7 ++
>  t/t9821-git-p4-path-variations.sh | 200 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 207 insertions(+)
>  create mode 100755 t/t9821-git-p4-path-variations.sh
>
> diff --git a/git-p4.py b/git-p4.py
> index 073f87b..0093fa3 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1950,10 +1950,14 @@ class View(object):
>              if "unmap" in res:
>                  # it will list all of them, but only one not unmap-ped
>                  continue
> +            if gitConfigBool("core.ignorecase"):
> +                res['depotFile'] = res['depotFile'].lower()
>              self.client_spec_path_cache[res['depotFile']] = self.convert_client_path(res["clientFile"])
>  
>          # not found files or unmap files set to ""
>          for depotFile in fileArgs:
> +            if gitConfigBool("core.ignorecase"):
> +                depotFile = depotFile.lower()
>              if depotFile not in self.client_spec_path_cache:
>                  self.client_spec_path_cache[depotFile] = ""
>  
> @@ -1962,6 +1966,9 @@ class View(object):
>             depot file should live.  Returns "" if the file should
>             not be mapped in the client."""
>  
> +        if gitConfigBool("core.ignorecase"):
> +            depot_path = depot_path.lower()
> +
>          if depot_path in self.client_spec_path_cache:
>              return self.client_spec_path_cache[depot_path]
>  
> diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
> new file mode 100755
> index 0000000..599d16c
> --- /dev/null
> +++ b/t/t9821-git-p4-path-variations.sh
> @@ -0,0 +1,200 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories with path case variations'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d with case folding enabled' '
> +	start_p4d -C1
> +'
> +
> +test_expect_success 'Create a repo with path case variations' '
> +	client_view "//depot/... //client/..." &&
> +	(
> +		cd "$cli" &&
> +
> +		mkdir -p One/two &&
> +		>One/two/File2.txt &&
> +		p4 add One/two/File2.txt &&
> +		p4 submit -d "Add file2" &&
> +		rm -rf One &&
> +
> +		mkdir -p one/TWO &&
> +		>one/TWO/file1.txt &&
> +		p4 add one/TWO/file1.txt &&
> +		p4 submit -d "Add file1" &&
> +		rm -rf one &&
> +
> +		mkdir -p one/two &&
> +		>one/two/file3.txt &&
> +		p4 add one/two/file3.txt &&
> +		p4 submit -d "Add file3" &&
> +		rm -rf one &&
> +
> +		mkdir -p outside-spec &&
> +		>outside-spec/file4.txt &&
> +		p4 add outside-spec/file4.txt &&
> +		p4 submit -d "Add file4" &&
> +		rm -rf outside-spec
> +	)
> +'
> +
> +test_expect_success 'Clone root' '
> +	client_view "//depot/... //client/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase false &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		# This method is used instead of "test -f" to ensure the case is
> +		# checked even if the test is executed on case-insensitive file systems.
> +		# All files are there as expected although the path cases look random.
> +		cat >expect <<-\EOF &&
> +		One/two/File2.txt
> +		one/TWO/file1.txt
> +		one/two/file3.txt
> +		outside-spec/file4.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone root (ignorecase)' '
> +	client_view "//depot/... //client/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase true &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		# This method is used instead of "test -f" to ensure the case is
> +		# checked even if the test is executed on case-insensitive file systems.
> +		# All files are there as expected although the path cases look random.
> +		cat >expect <<-\EOF &&
> +		one/TWO/File2.txt
> +		one/TWO/file1.txt
> +		one/TWO/file3.txt
> +		outside-spec/file4.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone root and ignore one file' '
> +	client_view \
> +		"//depot/... //client/..." \
> +		"-//depot/one/TWO/file1.txt //client/one/TWO/file1.txt" &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase false &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		# We ignore one file in the client spec and all path cases change from
> +		# "TWO" to "two"!
> +		cat >expect <<-\EOF &&
> +		One/two/File2.txt
> +		one/two/file3.txt
> +		outside-spec/file4.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone root and ignore one file (ignorecase)' '
> +	client_view \
> +		"//depot/... //client/..." \
> +		"-//depot/one/TWO/file1.txt //client/one/TWO/file1.txt" &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase true &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		# We ignore one file in the client spec and all path cases change from
> +		# "TWO" to "two"!
> +		cat >expect <<-\EOF &&
> +		One/two/File2.txt
> +		One/two/file3.txt
> +		outside-spec/file4.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone path' '
> +	client_view "//depot/One/... //client/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase false &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		cat >expect <<-\EOF &&
> +		two/File2.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'Clone path (ignorecase)' '
> +	client_view "//depot/One/... //client/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase true &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		cat >expect <<-\EOF &&
> +		TWO/File2.txt
> +		TWO/file1.txt
> +		TWO/file3.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +# It looks like P4 determines the path case based on the first file in
> +# lexicographical order. Please note the lower case "two" directory for all
> +# files triggered through the addition of "File0.txt".
> +test_expect_success 'Add a new file and clone path with new file (ignorecase)' '
> +	client_view "//depot/... //client/..." &&
> +	(
> +		cd "$cli" &&
> +		mkdir -p One/two &&
> +		>One/two/File0.txt &&
> +		p4 add One/two/File0.txt &&
> +		p4 submit -d "Add file" &&
> +		rm -rf One
> +	) &&
> +
> +	client_view "//depot/One/... //client/..." &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git init . &&
> +		git config core.ignorecase true &&
> +		git p4 clone --use-client-spec --destination="$git" //depot &&
> +		cat >expect <<-\EOF &&
> +		two/File0.txt
> +		two/File2.txt
> +		two/file1.txt
> +		two/file3.txt
> +		EOF
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done

Re: [PATCH v6] git-p4: Obey core.ignorecase when using P4 client specs.

[Lars Schneider]

On 26 Aug 2015, at 22:05, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> We run P4 servers on Linux and P4 clients on Windows. For an unknown
>> reason the file path for a number of files in P4 does not match the
>> directory path with respect to case sensitivity.
> 
> Thanks, but is this still "For an unknown reason", or during the
> course of debugging you found the root cause, which is what led to
> this fix?
We are migrating away from P4 and therefore I haven’t debugged the root cause. The source of this problem is 100% P4 related. No Git involvement at all. Maybe I just remove this paragraph?


> 
>> 
>> E.g. "p4 files" might return
>> //depot/path/to/file1
>> //depot/pATH/to/file2
>> 
>> If you use P4/P4V then these files end up in the same directory, e.g.
>> //depot/path/to/file1
>> //depot/path/to/file2
>> 
>> If you use git-p4 and clone the code via client spec "//depot/path/..."
>> then all files not matching the case in the client spec will be ignored
>> (in the example above "file2"). This is correct if core.ignorecase=false
>> but not otherwise.
> 
> This sentence is hard to grok.  What are you describing?  Solution?
> Current problematic behaviour?  Desired behaviour that the patch
> attempts to obtain?
I tried to describe the situation that causes the strange behavior.

> 
> If I paraphrase it like this, did I understood you correctly?
> 
>    The current code always ignores paths in wrong case that do not
>    match client spec.  It is correct to ignore them when
>    core.ignorecase is not set; //depot/path/ and //depot/pATH/ are
>    different things in that case.
> 
>    But it is a wrong thing to do if core.ignorecase is set to true.
>    Let's make sure we avoid it by downcasing the depot_path when
>    core.ignorecase is set to true.

How about this:

-----------------
The current code always ignores files with paths that do not match the case of the paths defined in the client spec. This commit changes this behavior and obeys these files if “core.ignorecase” is set to "true”.

Example:
A P4 repository contains the following files:
//depot/path/to/file1
//depot/pATH/to/file2

The P4 repository is cloned with git-p4 and the following client spec view:
//depot/path/… //client/...

The cloned Git repository will contain only the following file:
to/file1

“file2” is not present in the cloned Git repository. If “core.ignorecase" is set to “true” then path case sensitivity is ignored and “file2” will be present.
————————

Thanks,
Lars

Re: [PATCH v6] git-p4: Obey core.ignorecase when using P4 client specs.

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> On 26 Aug 2015, at 22:05, Junio C Hamano <gitster@pobox.com> wrote:
>
>> larsxschneider@gmail.com writes:
>> 
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> 
>>> We run P4 servers on Linux and P4 clients on Windows. For an unknown
>>> reason the file path for a number of files in P4 does not match the
>>> directory path with respect to case sensitivity.
>> 
>> Thanks, but is this still "For an unknown reason", or during the
>> course of debugging you found the root cause, which is what led to
>> this fix?
> We are migrating away from P4 and therefore I haven’t debugged the
> root cause. The source of this problem is 100% P4 related. No Git
> involvement at all. Maybe I just remove this paragraph?

Perhaps.  If it is irrelevant how the P4 depot ended up recording
the paths in mixed cases, then we can just say that as a prerequiste
condition to trigger the problem.

Perhaps rephrase the entire thing like this?

    Perforce depot may record paths in mixed cases, e.g. "p4 files" may
    show that there are these two paths:

        //depot/path/to/file1
        //depot/pATH/to/file2

    and with "p4" or "p4v", these end up in the same directory, e.g.

        //depot/path/to/file1
        //depot/path/to/file2

    which is the desired outcome on case insensitive systems.

    If git-p4 is used with client spec "//depot/path/...", however, then
    all files not matching the case in the client spec are ignored (in
    the example above "//depot/pATH/to/file2").

    Fix this by downcasing the depot_path when core.ignorcase is set to
    true.

Thanks.