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

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

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Thanks to Luke Diamand I realized the core problem and propose here a
substiantially simpler fix to my PATCH v4.

The test cases remain and prove the problem. In particular
"8 - Clone path (ignorecase)" and
"Add a new file and clone path with new file (ignorecase)" fail with the
current implementation on OS X and Linux.

Lars Schneider (1):
  git-p4: Obey core.ignorecase when using P4 client specs.

 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

--
1.9.5 (Apple Git-50.3)

[PATCH v5] 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>
---
 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 v5] git-p4: Obey core.ignorecase when using P4 client specs.

[Luke Diamand]

On 24/08/15 22:30, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Thanks to Luke Diamand I realized the core problem and propose here a
> substiantially simpler fix to my PATCH v4.
>
> The test cases remain and prove the problem. In particular
> "8 - Clone path (ignorecase)" and
> "Add a new file and clone path with new file (ignorecase)" fail with the
> current implementation on OS X and Linux.

That's a lot simpler, thanks!

Could we give this its own command line option and git config variable?

Core.ignorecase gets set if the client is on a filing system that 
ignores case. This is slightly different - it squashes case in depot 
files for people with depots that have incorrectly jumbled-up case.

Conflating the two seems like it would cause confusion at some point - 
for example, I have no idea how the rest of git behaves if 
core.ignorecase is set to True on a case-preserving file system.

It would probably be necessary to change p4PathStartsWith() to also 
check the same flag.


>
> Lars Schneider (1):
>    git-p4: Obey core.ignorecase when using P4 client specs.
>
>   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
>
> --
> 1.9.5 (Apple Git-50.3)
>

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

[Lars Schneider]

On 25 Aug 2015, at 08:54, Luke Diamand <luke@diamand.org> wrote:

> On 24/08/15 22:30, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Thanks to Luke Diamand I realized the core problem and propose here a
>> substiantially simpler fix to my PATCH v4.
>> 
>> The test cases remain and prove the problem. In particular
>> "8 - Clone path (ignorecase)" and
>> "Add a new file and clone path with new file (ignorecase)" fail with the
>> current implementation on OS X and Linux.
> 
> That's a lot simpler, thanks!
> 
> Could we give this its own command line option and git config variable?
Can you outline the command line option you have in mind?

> 
> Core.ignorecase gets set if the client is on a filing system that ignores case. This is slightly different - it squashes case in depot files for people with depots that have incorrectly jumbled-up case.
Well, you can’t tell if the depots have incorrectly jumbled-up case. It could be intentional after all. Therefore I believe the “ignorecase” flag is a good fit because we explicitly state that we are not interested in case sensitivity on the client.


> Conflating the two seems like it would cause confusion at some point - for example, I have no idea how the rest of git behaves if core.ignorecase is set to True on a case-preserving file system.
> 
> It would probably be necessary to change p4PathStartsWith() to also check the same flag.
> 
> 
>> 
>> Lars Schneider (1):
>>   git-p4: Obey core.ignorecase when using P4 client specs.
>> 
>>  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
>> 
>> --
>> 1.9.5 (Apple Git-50.3)
>> 
> 

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

[Torsten Bögershausen]

On 08/25/2015 08:54 AM, Luke Diamand wrote:
> On 24/08/15 22:30, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Thanks to Luke Diamand I realized the core problem and propose here a
>> substiantially simpler fix to my PATCH v4.
>>
>> The test cases remain and prove the problem. In particular
>> "8 - Clone path (ignorecase)" and
>> "Add a new file and clone path with new file (ignorecase)" fail with the
>> current implementation on OS X and Linux.
>
> That's a lot simpler, thanks!
>
> Could we give this its own command line option and git config variable?
>
> Core.ignorecase gets set if the client is on a filing system that 
> ignores case. This is slightly different - it squashes case in depot 
> files for people with depots that have incorrectly jumbled-up case.
>
> Conflating the two seems like it would cause confusion at some point - 
> for example, I have no idea how the rest of git behaves if 
> core.ignorecase is set to True on a case-preserving file system.
>
That doesn't work as expected and is not allowed (or say strictly 
forbidden, or strongly recommended not to do)

Remembering older discussions about importers from foreign VCS:
This should work best for most people:
Look at the command line option, if no one is given,
look at core.ignorecase.

So the command line option overrides core.ignorecase,
and the  user can either run
--ignore-path-case
or
--no-ignore-path-case
  PS:
Need to drop Eric from the list: the MX/A from from web.de problem still 
exists

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

[Lars Schneider]

On 25 Aug 2015, at 10:33, Torsten Bögershausen <tboegi@web.de> wrote:

> On 08/25/2015 08:54 AM, Luke Diamand wrote:
>> On 24/08/15 22:30, larsxschneider@gmail.com wrote:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> 
>>> Thanks to Luke Diamand I realized the core problem and propose here a
>>> substiantially simpler fix to my PATCH v4.
>>> 
>>> The test cases remain and prove the problem. In particular
>>> "8 - Clone path (ignorecase)" and
>>> "Add a new file and clone path with new file (ignorecase)" fail with the
>>> current implementation on OS X and Linux.
>> 
>> That's a lot simpler, thanks!
>> 
>> Could we give this its own command line option and git config variable?
>> 
>> Core.ignorecase gets set if the client is on a filing system that ignores case. This is slightly different - it squashes case in depot files for people with depots that have incorrectly jumbled-up case.
>> 
>> Conflating the two seems like it would cause confusion at some point - for example, I have no idea how the rest of git behaves if core.ignorecase is set to True on a case-preserving file system.
>> 
> That doesn't work as expected and is not allowed (or say strictly forbidden, or strongly recommended not to do)
> 
> Remembering older discussions about importers from foreign VCS:
> This should work best for most people:
> Look at the command line option, if no one is given,
> look at core.ignorecase.
> 
> So the command line option overrides core.ignorecase,
> and the  user can either run
> --ignore-path-case
> or
> --no-ignore-path-case
Unfortunately the command line option is not sufficient as the resulting paths are still messed up. I added the switch but it looks like as core.ignorecase does some additional magic on fast-import. You can see my changes here:
https://github.com/larsxschneider/git/commit/b4399179ff542161c2c5b83c34c5b4901287ceb0

You can also run the unit tests to see the results here:
https://github.com/larsxschneider/git/tree/lars/fix-path-v5-with-command-switch

The only way I could image to fix that is to request every path from P4 as shown in my PATCH v4. This would be slow and the change would be rather huge.

I am curious: 
I run all my P4 -> git migrations on a Linux box with EXT4 and core.ignorecase=True. I did not realize that this might cause trouble. What could happen and what should I look out for? 
One thing to keep in mind: This is a one time migration and we don’t develop on these Linux boxes. We usually develop on Windows and Mac. I just use the Linux boxes as migration workers as these scripts usually run a while.

- Lars

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

[Luke Diamand]

 On 25/08/15 11:30, larsxschneider@gmail.com wrote:

> Unfortunately the command line option is not sufficient as the resulting paths are still messed up. I added the switch but it looks like as core.ignorecase does some additional magic on fast-import. You can see my changes here:
> https://github.com/larsxschneider/git/commit/b4399179ff542161c2c5b83c34c5b4901287ceb0
>
> You can also run the unit tests to see the results here:
> https://github.com/larsxschneider/git/tree/lars/fix-path-v5-with-command-switch
>
> The only way I could image to fix that is to request every path from P4 as shown in my PATCH v4. This would be slow and the change would be rather huge.

Yes, you're right - fast-import has special handling based on core.ignorecase.

There was a thread a while back saying that it shouldn't do this, and
instead should have a new --casefold option, which would make more
sense, but isn't the case at present.

http://www.spinics.net/lists/git/msg243264.html

> I am curious:
> I run all my P4 -> git migrations on a Linux box with EXT4 and core.ignorecase=True. I did not realize that this might cause trouble. What could happen and what should I look out for?

An obvious way it could go wrong would be if you had a a repo that
_did_ care about case (e.g. had Makefile and makefile in the same
directory) and you then tried to git-p4 clone a separate repo into a
different branch. In an ideal world, you would only use the
case-folding on the git-p4 based repo. I think while fast-import just
checks core.ignorecase, that's not possible.

So the choices are:

1. A new command-line option which would silently set core.ignorecase
2. Users just have to know to set core.ignorecase manually before
using git-p4 (i.e. Lars' patch v5)
3. Fix fast-import to take a --casefold option (but that's a much bigger change)

Luke

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

[Lars Schneider]

On 25 Aug 2015, at 13:57, Luke Diamand <luke@diamand.org> wrote:

> On 25/08/15 11:30, larsxschneider@gmail.com wrote:
> 
>> Unfortunately the command line option is not sufficient as the resulting paths are still messed up. I added the switch but it looks like as core.ignorecase does some additional magic on fast-import. You can see my changes here:
>> https://github.com/larsxschneider/git/commit/b4399179ff542161c2c5b83c34c5b4901287ceb0
>> 
>> You can also run the unit tests to see the results here:
>> https://github.com/larsxschneider/git/tree/lars/fix-path-v5-with-command-switch
>> 
>> The only way I could image to fix that is to request every path from P4 as shown in my PATCH v4. This would be slow and the change would be rather huge.
> 
> Yes, you're right - fast-import has special handling based on core.ignorecase.
> 
> There was a thread a while back saying that it shouldn't do this, and
> instead should have a new --casefold option, which would make more
> sense, but isn't the case at present.
> 
> http://www.spinics.net/lists/git/msg243264.html
> 
>> I am curious:
>> I run all my P4 -> git migrations on a Linux box with EXT4 and core.ignorecase=True. I did not realize that this might cause trouble. What could happen and what should I look out for?
> 
> An obvious way it could go wrong would be if you had a a repo that
> _did_ care about case (e.g. had Makefile and makefile in the same
> directory) and you then tried to git-p4 clone a separate repo into a
> different branch. In an ideal world, you would only use the
> case-folding on the git-p4 based repo. I think while fast-import just
> checks core.ignorecase, that's not possible.
OK. Thanks for the explanation.

> 
> So the choices are:
> 
> 1. A new command-line option which would silently set core.ignorecase
> 2. Users just have to know to set core.ignorecase manually before
> using git-p4 (i.e. Lars' patch v5)
> 3. Fix fast-import to take a --casefold option (but that's a much bigger change)
> 
I vote for 2 because that solves the problem consistently with the existing implementation for now. That means we don’t surprise git-p4 users. In addition I would try to fix (3), the —casefold option, in a separate patch. Although this (3) patch could take a bit as I have two more git-p4 patches in the queue that I want to propose to the mailing list first.

- Lars

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

[Luke Diamand]

On 25/08/15 14:14, Lars Schneider wrote:
>>
>> So the choices are:
>>
>> 1. A new command-line option which would silently set core.ignorecase
>> 2. Users just have to know to set core.ignorecase manually before
>> using git-p4 (i.e. Lars' patch v5)
>> 3. Fix fast-import to take a --casefold option (but that's a much bigger change)
>>
> I vote for 2 because that solves the problem consistently with the existing implementation for now. That means we don’t surprise git-p4 users. In addition I would try to fix (3), the —casefold option, in a separate patch. Although this (3) patch could take a bit as I have two more git-p4 patches in the queue that I want to propose to the mailing list first.

That works for me. Ack.

Thanks!
Luke

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

[Luke Diamand]

On 25 August 2015 at 19:24, Luke Diamand <luke@diamand.org> wrote:
> On 25/08/15 14:14, Lars Schneider wrote:
>>>
>>>
>>> So the choices are:
>>>
>>> 1. A new command-line option which would silently set core.ignorecase
>>> 2. Users just have to know to set core.ignorecase manually before
>>> using git-p4 (i.e. Lars' patch v5)
>>> 3. Fix fast-import to take a --casefold option (but that's a much bigger
>>> change)
>>>
>> I vote for 2 because that solves the problem consistently with the
>> existing implementation for now. That means we don’t surprise git-p4 users.
>> In addition I would try to fix (3), the —casefold option, in a separate
>> patch. Although this (3) patch could take a bit as I have two more git-p4
>> patches in the queue that I want to propose to the mailing list first.
>
>
> That works for me. Ack.
>
> Thanks!
> Luke

Lars - could you resubmit PATCHv5 as v6, with Acked-by from me added
after the Signed-off-by: line.
It then this from SubmittingPatches:

> After the list reached a consensus that it is a good idea to apply the
> patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
> list [*2*] for inclusion.

Thanks
Luke