All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] git-p4: add "--path-encoding" option
@ 2015-08-31 22:10 larsxschneider
  2015-08-31 22:10 ` larsxschneider
  0 siblings, 1 reply; 13+ messages in thread
From: larsxschneider @ 2015-08-31 22:10 UTC (permalink / raw)
  To: git; +Cc: luke, gitster, tboegi, Lars Schneider

From: Lars Schneider <larsxschneider@gmail.com>

Diff to v2:
* remove comment
* use "core.quotepath false" to simplify test case

Unit test passed on OS X and Linux. The unit test did also pass on v2 as "printf $(git ls-files) >actual" removes the LF.

Cheers,
Lars

Lars Schneider (1):
  git-p4: add "--path-encoding" option

 Documentation/git-p4.txt        |  5 +++++
 git-p4.py                       |  6 ++++++
 t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
 create mode 100755 t/t9821-git-p4-path-encoding.sh

--
1.9.5 (Apple Git-50.3)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3] git-p4: add "--path-encoding" option
  2015-08-31 22:10 [PATCH v3] git-p4: add "--path-encoding" option larsxschneider
@ 2015-08-31 22:10 ` larsxschneider
  2015-08-31 23:13   ` Junio C Hamano
  2015-09-01  4:37   ` Torsten Bögershausen
  0 siblings, 2 replies; 13+ messages in thread
From: larsxschneider @ 2015-08-31 22:10 UTC (permalink / raw)
  To: git; +Cc: luke, gitster, tboegi, Lars Schneider

From: Lars Schneider <larsxschneider@gmail.com>

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/git-p4.txt        |  5 +++++
 git-p4.py                       |  6 ++++++
 t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
 create mode 100755 t/t9821-git-p4-path-encoding.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 82aa5d6..14bb79c 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -252,6 +252,11 @@ Git repository:
 	Use a client spec to find the list of interesting files in p4.
 	See the "CLIENT SPEC" section below.
 
+--path-encoding <encoding>::
+	The encoding to use when reading p4 client paths. With this option
+	non ASCII paths are properly stored in Git. For example, the encoding
+	'cp1252' is often used on Windows systems.
+
 -/ <path>::
 	Exclude selected depot paths when cloning or syncing.
 
diff --git a/git-p4.py b/git-p4.py
index 073f87b..2b3bfc4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1981,6 +1981,8 @@ class P4Sync(Command, P4UserMap):
                 optparse.make_option("--silent", dest="silent", action="store_true"),
                 optparse.make_option("--detect-labels", dest="detectLabels", action="store_true"),
                 optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
+                optparse.make_option("--path-encoding", dest="pathEncoding", type="string",
+                                     help="Encoding to use for paths"),
                 optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
                                      help="Import into refs/heads/ , not refs/remotes"),
                 optparse.make_option("--max-changes", dest="maxChanges",
@@ -2025,6 +2027,7 @@ class P4Sync(Command, P4UserMap):
         self.clientSpecDirs = None
         self.tempBranches = []
         self.tempBranchLocation = "git-p4-tmp"
+        self.pathEncoding = None
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -2213,6 +2216,9 @@ class P4Sync(Command, P4UserMap):
             text = regexp.sub(r'$\1$', text)
             contents = [ text ]
 
+        if self.pathEncoding:
+            relPath = relPath.decode(self.pathEncoding).encode('utf8', 'replace')
+
         self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
 
         # total length...
diff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh
new file mode 100755
index 0000000..1626fc5
--- /dev/null
+++ b/t/t9821-git-p4-path-encoding.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Clone repositories with non ASCII paths'
+
+. ./lib-git-p4.sh
+
+UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
+	cd "$cli" &&
+
+	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
+	>"$ISO8859" &&
+	p4 add "$ISO8859" &&
+	p4 submit -d "test commit"
+'
+
+test_expect_success 'Clone repo containing iso8859-1 encoded paths' '
+	git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		printf "$UTF8_ESCAPED\n" >expect &&
+		test_config core.quotepath false &&
+		git ls-files >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.9.5 (Apple Git-50.3)

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-08-31 22:10 ` larsxschneider
@ 2015-08-31 23:13   ` Junio C Hamano
  2015-09-01 13:42     ` Lars Schneider
  2015-09-01  4:37   ` Torsten Bögershausen
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-08-31 23:13 UTC (permalink / raw)
  To: larsxschneider; +Cc: git, luke, tboegi

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>

Here is a space for you to describe what it does and why it is a
good idea to have it.

> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  Documentation/git-p4.txt        |  5 +++++
>  git-p4.py                       |  6 ++++++
>  t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++

I'll move this to 9822, as 9821 is taken by another git-p4 test,
while queuing.

> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 82aa5d6..14bb79c 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -252,6 +252,11 @@ Git repository:
>  	Use a client spec to find the list of interesting files in p4.
>  	See the "CLIENT SPEC" section below.
>  
> +--path-encoding <encoding>::
> +	The encoding to use when reading p4 client paths. With this option
> +	non ASCII paths are properly stored in Git. For example, the encoding
> +	'cp1252' is often used on Windows systems.
> +

Is this something that needs to be consistently given while
interacting with the same P4 depot (in which case you may want to
allow this to be given only once, record the value in the config and
never allow it to be updated once it is set, or something)?

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-08-31 22:10 ` larsxschneider
  2015-08-31 23:13   ` Junio C Hamano
@ 2015-09-01  4:37   ` Torsten Bögershausen
  2015-09-01  7:14     ` Fwd: " Torsten Bögershausen
  2015-09-01 12:47     ` Lars Schneider
  1 sibling, 2 replies; 13+ messages in thread
From: Torsten Bögershausen @ 2015-09-01  4:37 UTC (permalink / raw)
  To: larsxschneider, git; +Cc: luke, gitster



On 01/09/15 00:10, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   Documentation/git-p4.txt        |  5 +++++
>   git-p4.py                       |  6 ++++++
>   t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 49 insertions(+)
>   create mode 100755 t/t9821-git-p4-path-encoding.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 82aa5d6..14bb79c 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -252,6 +252,11 @@ Git repository:
>   	Use a client spec to find the list of interesting files in p4.
>   	See the "CLIENT SPEC" section below.
>   
> +--path-encoding <encoding>::
> +	The encoding to use when reading p4 client paths. With this option
> +	non ASCII paths are properly stored in Git. For example, the encoding
> +	'cp1252' is often used on Windows systems.
> +
>   -/ <path>::
>   	Exclude selected depot paths when cloning or syncing.
>   
> diff --git a/git-p4.py b/git-p4.py
> index 073f87b..2b3bfc4 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1981,6 +1981,8 @@ class P4Sync(Command, P4UserMap):
>                   optparse.make_option("--silent", dest="silent", action="store_true"),
>                   optparse.make_option("--detect-labels", dest="detectLabels", action="store_true"),
>                   optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
> +                optparse.make_option("--path-encoding", dest="pathEncoding", type="string",
> +                                     help="Encoding to use for paths"),
>                   optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
>                                        help="Import into refs/heads/ , not refs/remotes"),
>                   optparse.make_option("--max-changes", dest="maxChanges",
> @@ -2025,6 +2027,7 @@ class P4Sync(Command, P4UserMap):
>           self.clientSpecDirs = None
>           self.tempBranches = []
>           self.tempBranchLocation = "git-p4-tmp"
> +        self.pathEncoding = None
>   
>           if gitConfig("git-p4.syncFromOrigin") == "false":
>               self.syncWithOrigin = False
> @@ -2213,6 +2216,9 @@ class P4Sync(Command, P4UserMap):
>               text = regexp.sub(r'$\1$', text)
>               contents = [ text ]
>   
> +        if self.pathEncoding:
> +            relPath = relPath.decode(self.pathEncoding).encode('utf8', 'replace')
> +
>           self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
>   
>           # total length...
> diff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh
> new file mode 100755
> index 0000000..1626fc5
> --- /dev/null
> +++ b/t/t9821-git-p4-path-encoding.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories with non ASCII paths'
> +
> +. ./lib-git-p4.sh
> +
> +UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
> +	cd "$cli" &&
> +
> +	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
> +	>"$ISO8859" &&
> +	p4 add "$ISO8859" &&
> +	p4 submit -d "test commit"
> +'
Sorry for being persistant,
but you can't create files with names that  are ISO-8859-1 encoded under Mac OS,
we end up like this:

  a-%E4_o-%F6_u-%FC.txt


(And I'm still not convinced, that we need to call iconv each time we execute 
the TC,
for a string that is always the same.
The string can be converted once, and embedded in the TC:
The following should work under Mac OS (but I don't have p4 to test it)

ISO8859_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"

UTF8_ESCAPED="\141\055\303\203\302\244\137\157\055\303\203\302\266\137\165\055\303\203\302\274\056\164\170\164"

ISO8859=$(printf "$ISO8859_ESCAPED")

> +
> +test_expect_success 'Clone repo containing iso8859-1 encoded paths' '
> +	git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		printf "$UTF8_ESCAPED\n" >expect &&
> +		test_config core.quotepath false &&
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
>
The ls-files can be written shorter (if we like short code)

+		git -c core.quotepath=false ls-files >actual &&

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Fwd: Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01  4:37   ` Torsten Bögershausen
@ 2015-09-01  7:14     ` Torsten Bögershausen
  2015-09-01 12:47     ` Lars Schneider
  1 sibling, 0 replies; 13+ messages in thread
From: Torsten Bögershausen @ 2015-09-01  7:14 UTC (permalink / raw)
  To: larsxschneider, Git Mailing List, Luke Diamand

Sorry if this is possible re-sending


-------- Forwarded Message --------
Subject: 	Re: [PATCH v3] git-p4: add "--path-encoding" option
Date: 	Tue, 1 Sep 2015 06:37:59 +0200
From: 	Torsten Bögershausen <tboegi@web.de>
To: 	larsxschneider@gmail.com, git@vger.kernel.org
CC: 	luke@diamand.org, gitster@pobox.com



On 01/09/15 00:10, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>   Documentation/git-p4.txt        |  5 +++++
>   git-p4.py                       |  6 ++++++
>   t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 49 insertions(+)
>   create mode 100755 t/t9821-git-p4-path-encoding.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 82aa5d6..14bb79c 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -252,6 +252,11 @@ Git repository:
>   	Use a client spec to find the list of interesting files in p4.
>   	See the "CLIENT SPEC" section below.
>
> +--path-encoding <encoding>::
> +	The encoding to use when reading p4 client paths. With this option
> +	non ASCII paths are properly stored in Git. For example, the encoding
> +	'cp1252' is often used on Windows systems.
> +
>   -/ <path>::
>   	Exclude selected depot paths when cloning or syncing.
>
> diff --git a/git-p4.py b/git-p4.py
> index 073f87b..2b3bfc4 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1981,6 +1981,8 @@ class P4Sync(Command, P4UserMap):
>                   optparse.make_option("--silent", dest="silent", action="store_true"),
>                   optparse.make_option("--detect-labels", dest="detectLabels", action="store_true"),
>                   optparse.make_option("--import-labels", dest="importLabels", action="store_true"),
> +                optparse.make_option("--path-encoding", dest="pathEncoding", type="string",
> +                                     help="Encoding to use for paths"),
>                   optparse.make_option("--import-local", dest="importIntoRemotes", action="store_false",
>                                        help="Import into refs/heads/ , not refs/remotes"),
>                   optparse.make_option("--max-changes", dest="maxChanges",
> @@ -2025,6 +2027,7 @@ class P4Sync(Command, P4UserMap):
>           self.clientSpecDirs = None
>           self.tempBranches = []
>           self.tempBranchLocation = "git-p4-tmp"
> +        self.pathEncoding = None
>
>           if gitConfig("git-p4.syncFromOrigin") == "false":
>               self.syncWithOrigin = False
> @@ -2213,6 +2216,9 @@ class P4Sync(Command, P4UserMap):
>               text = regexp.sub(r'$\1$', text)
>               contents = [ text ]
>
> +        if self.pathEncoding:
> +            relPath = relPath.decode(self.pathEncoding).encode('utf8', 'replace')
> +
>           self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
>
>           # total length...
> diff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh
> new file mode 100755
> index 0000000..1626fc5
> --- /dev/null
> +++ b/t/t9821-git-p4-path-encoding.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories with non ASCII paths'
> +
> +. ./lib-git-p4.sh
> +
> +UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
> +	cd "$cli" &&
> +
> +	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
> +	>"$ISO8859" &&
> +	p4 add "$ISO8859" &&
> +	p4 submit -d "test commit"
> +'
Sorry for being persistant,
but you can't create files with names that  are ISO-8859-1 encoded under Mac OS,
we end up like this:

  a-%E4_o-%F6_u-%FC.txt


(And I'm still not convinced, that we need to call iconv each time we execute
the TC,
for a string that is always the same.
The string can be converted once, and embedded in the TC:
The following should work under Mac OS (but I don't have p4 to test it)

ISO8859_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"

UTF8_ESCAPED="\141\055\303\203\302\244\137\157\055\303\203\302\266\137\165\055\303\203\302\274\056\164\170\164"

ISO8859=$(printf "$ISO8859_ESCAPED")

> +
> +test_expect_success 'Clone repo containing iso8859-1 encoded paths' '
> +	git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		printf "$UTF8_ESCAPED\n" >expect &&
> +		test_config core.quotepath false &&
> +		git ls-files >actual &&
> +		test_cmp expect actual
> +	)
> +'
>
The ls-files can be written shorter (if we like short code)

+		git -c core.quotepath=false ls-files >actual &&

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01  4:37   ` Torsten Bögershausen
  2015-09-01  7:14     ` Fwd: " Torsten Bögershausen
@ 2015-09-01 12:47     ` Lars Schneider
  2015-09-01 14:35       ` Torsten Bögershausen
  1 sibling, 1 reply; 13+ messages in thread
From: Lars Schneider @ 2015-09-01 12:47 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: git, luke, gitster


On 01 Sep 2015, at 06:37, Torsten Bögershausen <tboegi@web.de> wrote:

> On 01/09/15 00:10, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>>  Documentation/git-p4.txt        |  5 +++++
>>  git-p4.py                       |  6 ++++++
>>  t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 49 insertions(+)
>>  create mode 100755 t/t9821-git-p4-path-encoding.sh
>> 
>> 
<snip>
> 
>> +
>> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
>> +	cd "$cli" &&
>> +
>> +	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
>> +	>"$ISO8859" &&
>> +	p4 add "$ISO8859" &&
>> +	p4 submit -d "test commit"
>> +'
> Sorry for being persistant,
> but you can't create files with names that  are ISO-8859-1 encoded under Mac OS,
> we end up like this:
> 
> a-%E4_o-%F6_u-%FC.txt
You are right. However, my goal is not to create a file with ISO-8859-1 characters in Mac OS. My goal is to create this file in Perforce and this approach seems to work.

> 
> (And I'm still not convinced, that we need to call iconv each time we execute the TC,
> for a string that is always the same.
> The string can be converted once, and embedded in the TC:
> The following should work under Mac OS (but I don't have p4 to test it)
> 
> ISO8859_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
> 
> UTF8_ESCAPED="\141\055\303\203\302\244\137\157\055\303\203\302\266\137\165\055\303\203\302\274\056\164\170\164"
> 
> ISO8859=$(printf "$ISO8859_ESCAPED”)
OK. However, how about this?

UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
ISO8859_ESCAPED="\141\55\344\137\157\55\366\137\165\55\374\56\164\170\164"

# You can generate the ISO8859_ESCAPED with the following command:
# printf "$UTF8_ESCAPED" | \
# iconv -f utf-8 -t iso8859-1 | \
# xxd -ps -u -c 1 | xargs bash -c 'for v; do echo "ibase=16; obase=8; $v" | bc; done' bash | \
# tr "\n" "\\"



> 
>> +
>> +test_expect_success 'Clone repo containing iso8859-1 encoded paths' '
>> +	git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot &&
>> +	test_when_finished cleanup_git &&
>> +	(
>> +		cd "$git" &&
>> +		printf "$UTF8_ESCAPED\n" >expect &&
>> +		test_config core.quotepath false &&
>> +		git ls-files >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> 
> The ls-files can be written shorter (if we like short code)
> 
> +		git -c core.quotepath=false ls-files >actual &&
Fixed. Thank you!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-08-31 23:13   ` Junio C Hamano
@ 2015-09-01 13:42     ` Lars Schneider
  2015-09-01 17:19       ` Junio C Hamano
  2015-09-01 17:35       ` Junio C Hamano
  0 siblings, 2 replies; 13+ messages in thread
From: Lars Schneider @ 2015-09-01 13:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, luke, tboegi


On 01 Sep 2015, at 01:13, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
> 
> Here is a space for you to describe what it does and why it is a
> good idea to have it.
How about this:

Perforce keeps the encoding of a path as given by the originating OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4 what encoding Perforce had used for the paths. This encoding is used to transcode the paths to UTF-8. As an example, Perforce on Windows uses “cp1252” to encode path names. 


> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/git-p4.txt        |  5 +++++
>> git-p4.py                       |  6 ++++++
>> t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
> 
> I'll move this to 9822, as 9821 is taken by another git-p4 test,
> while queuing.
OK. I wasn’t sure how this is handled. Just for my understanding: As soon as a TC number is occupied in one of the official branches (master/next/pu/maint) then the next number should be used, right?

> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 82aa5d6..14bb79c 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -252,6 +252,11 @@ Git repository:
>> 	Use a client spec to find the list of interesting files in p4.
>> 	See the "CLIENT SPEC" section below.
>> 
>> +--path-encoding <encoding>::
>> +	The encoding to use when reading p4 client paths. With this option
>> +	non ASCII paths are properly stored in Git. For example, the encoding
>> +	'cp1252' is often used on Windows systems.
>> +
> 
> Is this something that needs to be consistently given while
> interacting with the same P4 depot (in which case you may want to
> allow this to be given only once, record the value in the config and
> never allow it to be updated once it is set, or something)?
Good idea! I will add this. However, I really wonder why nobody tripped over this rather obvious bug already.

@Luke: Can you double check my fix?


Two general questions:

(1) I saw that the path encoding fix is already on “pu”. What is more convenient for you? Do you want to get a [PATCH v4] with one commit including the changes mentioned above or do you want me to keep the v3 commit and add a v4 commit on top?

(2) Is there a CI setup for Git somewhere accessible on the Internet? How about building and testing Git on Travis (https://travis-ci.org/git/git)?

Thanks,
Lars

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 12:47     ` Lars Schneider
@ 2015-09-01 14:35       ` Torsten Bögershausen
  2015-09-01 15:02         ` Lars Schneider
  0 siblings, 1 reply; 13+ messages in thread
From: Torsten Bögershausen @ 2015-09-01 14:35 UTC (permalink / raw)
  To: Lars Schneider, Torsten Bögershausen; +Cc: git, luke, gitster

On 2015-09-01 14.47, Lars Schneider wrote:
>>> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
>>> >> +	cd "$cli" &&
>>> >> +
>>> >> +	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
>>> >> +	>"$ISO8859" &&
>>> >> +	p4 add "$ISO8859" &&
>>> >> +	p4 submit -d "test commit"
>>> >> +'
>> > Sorry for being persistant,
>> > but you can't create files with names that  are ISO-8859-1 encoded under Mac OS,
>> > we end up like this:
>> > 
>> > a-%E4_o-%F6_u-%FC.txt
> You are right. However, my goal is not to create a file with ISO-8859-1 characters in Mac OS. My goal is to create this file in Perforce and this approach seems to work.
> 
>> > 
But this line creates a file, doesn't it ?
>"$ISO8859" &&

(I just wonder how this works on you machine )

And, may be, we could fill the file with some content, to be double-sure that
the file name conversion works with Perforce ?

like
echo content >"$ISO8859" &&

and test the content later ?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 14:35       ` Torsten Bögershausen
@ 2015-09-01 15:02         ` Lars Schneider
  0 siblings, 0 replies; 13+ messages in thread
From: Lars Schneider @ 2015-09-01 15:02 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: git, luke, gitster


On 01 Sep 2015, at 16:35, Torsten Bögershausen <tboegi@web.de> wrote:

> On 2015-09-01 14.47, Lars Schneider wrote:
>>>> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
>>>>>> +	cd "$cli" &&
>>>>>> +
>>>>>> +	ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
>>>>>> +	>"$ISO8859" &&
>>>>>> +	p4 add "$ISO8859" &&
>>>>>> +	p4 submit -d "test commit"
>>>>>> +'
>>>> Sorry for being persistant,
>>>> but you can't create files with names that  are ISO-8859-1 encoded under Mac OS,
>>>> we end up like this:
>>>> 
>>>> a-%E4_o-%F6_u-%FC.txt
>> You are right. However, my goal is not to create a file with ISO-8859-1 characters in Mac OS. My goal is to create this file in Perforce and this approach seems to work.
>> 
>>>> 
> But this line creates a file, doesn't it ?
>> "$ISO8859" &&
> 
> (I just wonder how this works on you machine )
I tested it on OS X (HPFS) and Linux (ext4). Can you reproduce problems on your machine? If yes, what is your OS/filesystem?

> And, may be, we could fill the file with some content, to be double-sure that
> the file name conversion works with Perforce ?
> 
> like
> echo content >"$ISO8859" &&
> 
> and test the content later ?
OK, I will add this.

Thanks,
Lars

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 13:42     ` Lars Schneider
@ 2015-09-01 17:19       ` Junio C Hamano
  2015-09-01 17:35       ` Junio C Hamano
  1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2015-09-01 17:19 UTC (permalink / raw)
  To: Lars Schneider; +Cc: git, luke, tboegi

Lars Schneider <larsxschneider@gmail.com> writes:

>> I'll move this to 9822, as 9821 is taken by another git-p4 test,
>> while queuing.
> OK. I wasn’t sure how this is handled. Just for my understanding: As
> soon as a TC number is occupied in one of the official branches
> (master/next/pu/maint) then the next number should be used, right?

That would help me great deal (as I'd be doing that if you didn't).

> Two general questions:
>
> (1) I saw that the path encoding fix is already on “pu”. What is
> more convenient for you? Do you want to get a [PATCH v4] with one
> commit including the changes mentioned above or do you want me to keep
> the v3 commit and add a v4 commit on top?

The convention is to do [PATCH v$n+1] (i.e. reroll) before the topic
is merged to 'next' (i.e. while in 'pu').

When further improvements are needed after the topic is merged to
'next', we switch to incremental improvements.

> (2) Is there a CI setup for Git somewhere accessible on the Internet?
> How about building and testing Git on Travis
> (https://travis-ci.org/git/git)?

Sorry but you are asking the right question to a wrong guy ;-)

There may be people who have already arranged something like that,
who can speak up.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 13:42     ` Lars Schneider
  2015-09-01 17:19       ` Junio C Hamano
@ 2015-09-01 17:35       ` Junio C Hamano
  2015-09-01 18:55         ` Lars Schneider
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2015-09-01 17:35 UTC (permalink / raw)
  To: Lars Schneider; +Cc: git, luke, tboegi

Lars Schneider <larsxschneider@gmail.com> writes:

> On 01 Sep 2015, at 01:13, Junio C Hamano <gitster@pobox.com> wrote:
>
>> larsxschneider@gmail.com writes:
>> 
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>> 
>> 
>> Here is a space for you to describe what it does and why it is a
>> good idea to have it.
> How about this:
>
> Perforce keeps the encoding of a path as given by the originating
> OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4
> what encoding Perforce had used for the paths. This encoding is used
> to transcode the paths to UTF-8. As an example, Perforce on Windows
> uses “cp1252” to encode path names.

Very readable.  Does "Perforce on Windows" always use cp1252, or
is it more correct to say "often uses" here?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 17:35       ` Junio C Hamano
@ 2015-09-01 18:55         ` Lars Schneider
  2015-09-01 19:06           ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Lars Schneider @ 2015-09-01 18:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, luke, tboegi


On 01 Sep 2015, at 19:35, Junio C Hamano <gitster@pobox.com> wrote:

> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> On 01 Sep 2015, at 01:13, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>>> larsxschneider@gmail.com writes:
>>> 
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> 
>>> 
>>> Here is a space for you to describe what it does and why it is a
>>> good idea to have it.
>> How about this:
>> 
>> Perforce keeps the encoding of a path as given by the originating
>> OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4
>> what encoding Perforce had used for the paths. This encoding is used
>> to transcode the paths to UTF-8. As an example, Perforce on Windows
>> uses “cp1252” to encode path names.
> 
> Very readable.  Does "Perforce on Windows" always use cp1252, or
> is it more correct to say "often uses" here?
Thank you! I don’t know if “always” or “often” is better. On my Windows test system it is “always”… but that’s not a valid sample size :-)
I searched the Internet for clues around cp1252 and found that a similar patch was submitted to Mercurial just a month ago. The author seconds my cp1252 observation:
http://mercurial.808500.n3.nabble.com/PATCH-stable-convert-use-original-local-encoding-when-converting-from-Perfoce-tp4025088p4025094.html

- Lars

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] git-p4: add "--path-encoding" option
  2015-09-01 18:55         ` Lars Schneider
@ 2015-09-01 19:06           ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2015-09-01 19:06 UTC (permalink / raw)
  To: Lars Schneider; +Cc: git, luke, tboegi

Lars Schneider <larsxschneider@gmail.com> writes:

> I searched the Internet for clues around cp1252 and found that a
> similar patch was submitted to Mercurial just a month ago. The author
> seconds my cp1252 observation:
> http://mercurial.808500.n3.nabble.com/PATCH-stable-convert-use-original-local-encoding-when-converting-from-Perfoce-tp4025088p4025094.html

Thanks for a pointer.  I see this bit in that thread:

     If P4CHARSET is not set explicitly when connecting to a Unicode
     mode server, a default charset will be chosen based on the
     client's platform and/or code page.

So I would guess that it is reproducible for you as you are always
on a client with the same platform and/or code page, and it would be
more honest to phrase it as "As an example, Perforce on my Windows
box uses..."  or something along that line.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-09-01 19:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-31 22:10 [PATCH v3] git-p4: add "--path-encoding" option larsxschneider
2015-08-31 22:10 ` larsxschneider
2015-08-31 23:13   ` Junio C Hamano
2015-09-01 13:42     ` Lars Schneider
2015-09-01 17:19       ` Junio C Hamano
2015-09-01 17:35       ` Junio C Hamano
2015-09-01 18:55         ` Lars Schneider
2015-09-01 19:06           ` Junio C Hamano
2015-09-01  4:37   ` Torsten Bögershausen
2015-09-01  7:14     ` Fwd: " Torsten Bögershausen
2015-09-01 12:47     ` Lars Schneider
2015-09-01 14:35       ` Torsten Bögershausen
2015-09-01 15:02         ` Lars Schneider

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.