[PATCH v4 0/2] git-p4: handle "Translation of file content failed"

[PATCH v4 0/2] git-p4: handle "Translation of file content failed"

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v3:
* replace non portable "sed -i" call in test case (thanks Luke and Torsten!)
* use "test_expect_failure" in the first commit that adds the test case, flip it to "test_expect_success" in subsequent commit (thanks Eric and Luke!)
* rename test case from t9824... to t9825... to avoid clashes with "git-p4: add Git LFS backend for large file system" patch

Cheers,
Lars

Lars Schneider (2):
  git-p4: add test case for "Translation of file content failed" error
  git-p4: handle "Translation of file content failed"

 git-p4.py                                  | 27 +++++++++-------
 t/t9825-git-p4-handle-utf16-without-bom.sh | 50 ++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 11 deletions(-)
 create mode 100755 t/t9825-git-p4-handle-utf16-without-bom.sh

--
2.5.1

[PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

A P4 repository can get into a state where it contains a file with
type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
attempts to retrieve the file then the process crashes with a
"Translation of file content failed" error.

More info here: http://answers.perforce.com/articles/KB/3117

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t9825-git-p4-handle-utf16-without-bom.sh | 50 ++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100755 t/t9825-git-p4-handle-utf16-without-bom.sh

diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
new file mode 100755
index 0000000..65c3c4e
--- /dev/null
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='git p4 handling of UTF-16 files without BOM'
+
+. ./lib-git-p4.sh
+
+UTF16="\227\000\227\000"
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'init depot with UTF-16 encoded file and artificially remove BOM' '
+	(
+		cd "$cli" &&
+		printf "$UTF16" >file1 &&
+		p4 add -t utf16 file1 &&
+		p4 submit -d "file1"
+	) &&
+
+	(
+		cd "db" &&
+		p4d -jc &&
+		# P4D automatically adds a BOM. Remove it here to make the file invalid.
+		sed -e "$ d" depot/file1,v >depot/file1,v.new &&
+		mv -- depot/file1,v.new depot/file1,v &&
+		printf "@$UTF16@" >>depot/file1,v &&
+		p4d -jrF checkpoint.1
+	)
+'
+
+test_expect_failure 'clone depot with invalid UTF-16 file in verbose mode' '
+	git p4 clone --dest="$git" --verbose //depot &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		printf "$UTF16" >expect &&
+		test_cmp_bin expect file1
+	)
+'
+
+test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '
+	git p4 clone --dest="$git" //depot
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
2.5.1

[PATCH v4 2/2] git-p4: handle "Translation of file content failed"

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

A P4 repository can get into a state where it contains a file with
type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
attempts to retrieve the file then the process crashes with a
"Translation of file content failed" error.

More info here: http://answers.perforce.com/articles/KB/3117

Fix this by detecting this error and retrieving the file as binary
instead. The result in Git is the same.

Known issue: This works only if git-p4 is executed in verbose mode.
In normal mode no exceptions are thrown and git-p4 just exits.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 git-p4.py                                  | 27 ++++++++++++++++-----------
 t/t9825-git-p4-handle-utf16-without-bom.sh |  2 +-
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 073f87b..5ae25a6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -134,13 +134,11 @@ def read_pipe(c, ignore_error=False):
         sys.stderr.write('Reading pipe: %s\n' % str(c))
 
     expand = isinstance(c,basestring)
-    p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
-    pipe = p.stdout
-    val = pipe.read()
-    if p.wait() and not ignore_error:
-        die('Command failed: %s' % str(c))
-
-    return val
+    p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
+    (out, err) = p.communicate()
+    if p.returncode != 0 and not ignore_error:
+        die('Command failed: %s\nError: %s' % (str(c), err))
+    return out
 
 def p4_read_pipe(c, ignore_error=False):
     real_cmd = p4_build_cmd(c)
@@ -2186,10 +2184,17 @@ class P4Sync(Command, P4UserMap):
             # them back too.  This is not needed to the cygwin windows version,
             # just the native "NT" type.
             #
-            text = p4_read_pipe(['print', '-q', '-o', '-', "%s@%s" % (file['depotFile'], file['change']) ])
-            if p4_version_string().find("/NT") >= 0:
-                text = text.replace("\r\n", "\n")
-            contents = [ text ]
+            try:
+                text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % (file['depotFile'], file['change'])])
+            except Exception as e:
+                if 'Translation of file content failed' in str(e):
+                    type_base = 'binary'
+                else:
+                    raise e
+            else:
+                if p4_version_string().find('/NT') >= 0:
+                    text = text.replace('\r\n', '\n')
+                contents = [ text ]
 
         if type_base == "apple":
             # Apple filetype files will be streamed as a concatenation of
diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 65c3c4e..fd2edce 100755
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -29,7 +29,7 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove
 	)
 '
 
-test_expect_failure 'clone depot with invalid UTF-16 file in verbose mode' '
+test_expect_success 'clone depot with invalid UTF-16 file in verbose mode' '
 	git p4 clone --dest="$git" --verbose //depot &&
 	test_when_finished cleanup_git &&
 	(
-- 
2.5.1

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> A P4 repository can get into a state where it contains a file with
> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
> attempts to retrieve the file then the process crashes with a
> "Translation of file content failed" error.
>
> More info here: http://answers.perforce.com/articles/KB/3117
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  t/t9825-git-p4-handle-utf16-without-bom.sh | 50 ++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100755 t/t9825-git-p4-handle-utf16-without-bom.sh
>
> diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh
> b/t/t9825-git-p4-handle-utf16-without-bom.sh
> new file mode 100755
> index 0000000..65c3c4e
> --- /dev/null
> +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
> @@ -0,0 +1,50 @@
> +#!/bin/sh
> +
> +test_description='git p4 handling of UTF-16 files without BOM'
> +
> +. ./lib-git-p4.sh
> +
> +UTF16="\227\000\227\000"
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'init depot with UTF-16 encoded file and artificially remove BOM' '
> +	(
> +		cd "$cli" &&
> +		printf "$UTF16" >file1 &&
> +		p4 add -t utf16 file1 &&
> +		p4 submit -d "file1"
> +	) &&
> +
> +	(
> +		cd "db" &&
> +		p4d -jc &&
> +		# P4D automatically adds a BOM. Remove it here to make the file invalid.
> +		sed -e "$ d" depot/file1,v >depot/file1,v.new &&

Do you need the space between the address $ (i.e. the last line) and
operation 'd' (i.e. delete it)?  I am asking because that looks very
unusual at least in our codebase.

> +		mv -- depot/file1,v.new depot/file1,v &&
> +		printf "@$UTF16@" >>depot/file1,v &&
> +		p4d -jrF checkpoint.1
> +	)
> +'
> +
> +test_expect_failure 'clone depot with invalid UTF-16 file in verbose mode' '
> +	git p4 clone --dest="$git" --verbose //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		printf "$UTF16" >expect &&
> +		test_cmp_bin expect file1
> +	)
> +'
> +
> +test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '
> +	git p4 clone --dest="$git" //depot
> +'
> +
> +test_expect_success 'kill p4d' '
> +	kill_p4d
> +'
> +
> +test_done

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 21 Sep 2015, at 20:09, Junio C Hamano <gitster@pobox.com> wrote:

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> More info here: http://answers.perforce.com/articles/KB/3117
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> t/t9825-git-p4-handle-utf16-without-bom.sh | 50 ++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>> create mode 100755 t/t9825-git-p4-handle-utf16-without-bom.sh
>> 
>> diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh
>> b/t/t9825-git-p4-handle-utf16-without-bom.sh
>> new file mode 100755
>> index 0000000..65c3c4e
>> --- /dev/null
>> +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
>> @@ -0,0 +1,50 @@
>> +#!/bin/sh
>> +
>> +test_description='git p4 handling of UTF-16 files without BOM'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +UTF16="\227\000\227\000"
>> +
>> +test_expect_success 'start p4d' '
>> +	start_p4d
>> +'
>> +
>> +test_expect_success 'init depot with UTF-16 encoded file and artificially remove BOM' '
>> +	(
>> +		cd "$cli" &&
>> +		printf "$UTF16" >file1 &&
>> +		p4 add -t utf16 file1 &&
>> +		p4 submit -d "file1"
>> +	) &&
>> +
>> +	(
>> +		cd "db" &&
>> +		p4d -jc &&
>> +		# P4D automatically adds a BOM. Remove it here to make the file invalid.
>> +		sed -e "$ d" depot/file1,v >depot/file1,v.new &&
> 
> Do you need the space between the address $ (i.e. the last line) and
> operation 'd' (i.e. delete it)?  I am asking because that looks very
> unusual at least in our codebase.
Well, I am no “sed” pro. I have to admit that I found this snippet on the Internet and it just worked. If I remove the space then it does not work. I was not yet able to figure out why… anyone an idea?

Thanks,
Lars



> 
>> +		mv -- depot/file1,v.new depot/file1,v &&
>> +		printf "@$UTF16@" >>depot/file1,v &&
>> +		p4d -jrF checkpoint.1
>> +	)
>> +'
>> +
>> +test_expect_failure 'clone depot with invalid UTF-16 file in verbose mode' '
>> +	git p4 clone --dest="$git" --verbose //depot &&
>> +	test_when_finished cleanup_git &&
>> +	(
>> +		cd "$git" &&
>> +		printf "$UTF16" >expect &&
>> +		test_cmp_bin expect file1
>> +	)
>> +'
>> +
>> +test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose mode' '
>> +	git p4 clone --dest="$git" //depot
>> +'
>> +
>> +test_expect_success 'kill p4d' '
>> +	kill_p4d
>> +'
>> +
>> +test_done

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Eric Sunshine]

On Mon, Sep 21, 2015 at 7:03 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> On 21 Sep 2015, at 20:09, Junio C Hamano <gitster@pobox.com> wrote:
>> larsxschneider@gmail.com writes:
>>> +test_expect_success 'init depot with UTF-16 encoded file and artificially remove BOM' '
>>> +    (
>>> +            cd "db" &&
>>> +            p4d -jc &&
>>> +            # P4D automatically adds a BOM. Remove it here to make the file invalid.
>>> +            sed -e "$ d" depot/file1,v >depot/file1,v.new &&
>>
>> Do you need the space between the address $ (i.e. the last line) and
>> operation 'd' (i.e. delete it)?  I am asking because that looks very
>> unusual at least in our codebase.
>
> Well, I am no “sed” pro. I have to admit that I found this snippet
> on the Internet and it just worked. If I remove the space then it
> does not work. I was not yet able to figure out why… anyone an idea?

Yes, it's because $d is a variable reference, even within double
quotes. Typically, one uses single quotes around the sed argument to
suppress this sort of undesired behavior. Since the entire test body
is already within single quotes, however, changing the sed argument to
use single quotes, rather than double, will require escaping them:

    sed -e \'$d\' depot/file...

Aside: You could also drop the unnecessary quotes from the 'cd' argument.

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Yes, it's because $d is a variable reference, even within double
> quotes.

s/even/especially/ ;-)

Here is what I queued as SQUASH???

diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
index 65c3c4e..735c0bb 100644
--- a/t/t9825-git-p4-handle-utf16-without-bom.sh
+++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
@@ -22,8 +22,8 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove
 		cd "db" &&
 		p4d -jc &&
 		# P4D automatically adds a BOM. Remove it here to make the file invalid.
-		sed -e "$ d" depot/file1,v >depot/file1,v.new &&
-		mv -- depot/file1,v.new depot/file1,v &&
+		sed -e "\$d" depot/file1,v >depot/file1,v.new &&
+		mv depot/file1,v.new depot/file1,v &&
 		printf "@$UTF16@" >>depot/file1,v &&
 		p4d -jrF checkpoint.1
 	)

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 22 Sep 2015, at 03:10, Junio C Hamano <gitster@pobox.com> wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> Yes, it's because $d is a variable reference, even within double
>> quotes.
> 
> s/even/especially/ ;-)
> 
> Here is what I queued as SQUASH???
> 
> diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh
> index 65c3c4e..735c0bb 100644
> --- a/t/t9825-git-p4-handle-utf16-without-bom.sh
> +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
> @@ -22,8 +22,8 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove
> 		cd "db" &&
> 		p4d -jc &&
> 		# P4D automatically adds a BOM. Remove it here to make the file invalid.
> -		sed -e "$ d" depot/file1,v >depot/file1,v.new &&
> -		mv -- depot/file1,v.new depot/file1,v &&
> +		sed -e "\$d" depot/file1,v >depot/file1,v.new &&
> +		mv depot/file1,v.new depot/file1,v &&
> 		printf "@$UTF16@" >>depot/file1,v &&
> 		p4d -jrF checkpoint.1
> 	)

This works. I even tested successfully this one:

sed \$d depot/file1,v >depot/file1,v.new &&

Do we need the “-e” option?

Thanks,
Lars

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 22 Sep 2015, at 01:54, Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Mon, Sep 21, 2015 at 7:03 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> On 21 Sep 2015, at 20:09, Junio C Hamano <gitster@pobox.com> wrote:
>>> larsxschneider@gmail.com writes:
>>>> +test_expect_success 'init depot with UTF-16 encoded file and artificially remove BOM' '
>>>> +    (
>>>> +            cd "db" &&
>>>> +            p4d -jc &&
>>>> +            # P4D automatically adds a BOM. Remove it here to make the file invalid.
>>>> +            sed -e "$ d" depot/file1,v >depot/file1,v.new &&
>>> 
>>> Do you need the space between the address $ (i.e. the last line) and
>>> operation 'd' (i.e. delete it)?  I am asking because that looks very
>>> unusual at least in our codebase.
>> 
>> Well, I am no “sed” pro. I have to admit that I found this snippet
>> on the Internet and it just worked. If I remove the space then it
>> does not work. I was not yet able to figure out why… anyone an idea?
> 
> Yes, it's because $d is a variable reference, even within double
> quotes. Typically, one uses single quotes around the sed argument to
> suppress this sort of undesired behavior. Since the entire test body
> is already within single quotes, however, changing the sed argument to
> use single quotes, rather than double, will require escaping them:
> 
>    sed -e \'$d\' depot/file...
> 
> Aside: You could also drop the unnecessary quotes from the 'cd' argument.

Thanks for the explanation. Plus you are correct with the quotes around “db”… just a habit.

@Junio: 
If it is no extra work for you, you can remove the quotes around “db”. I can also create a new patch roll including the sed change and the quote change if it is easier for you.

Best,
Lars

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> This works.

OK, and thanks; as I don't do perforce, the squash was without any
testing.

> Do we need the “-e” option?

In syntactic sense, no, but our codebase tends to prefer to have
one, because it is easier to spot which ones are the instructions if
you consistently have "-e" even when you give only one.

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> If it is no extra work for you, you can remove the quotes around
> “db”. I can also create a new patch roll including the sed change
> and the quote change if it is easier for you.

Now you've tested the SQUASH??? for me, I can just squash that into
your original without resend.

Thanks.

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Michael Blume]

I'm seeing test failures

non-executable tests: t9825-git-p4-handle-utf16-without-bom.sh

ls -l shows that all the other tests are executable but t9825 isn't.

On Tue, Sep 22, 2015 at 9:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>> This works.
>
> OK, and thanks; as I don't do perforce, the squash was without any
> testing.
>
>> Do we need the “-e” option?
>
> In syntactic sense, no, but our codebase tends to prefer to have
> one, because it is easier to spot which ones are the instructions if
> you consistently have "-e" even when you give only one.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Junio C Hamano]

Yup, this was privately reported and I just squashed a fix in right now ;-)

Thanks. "cd t && make test-lint" would have caught it.

On Tue, Sep 22, 2015 at 12:11 PM, Michael Blume <blume.mike@gmail.com> wrote:
> I'm seeing test failures
>
> non-executable tests: t9825-git-p4-handle-utf16-without-bom.sh
>
> ls -l shows that all the other tests are executable but t9825 isn't.
>
> On Tue, Sep 22, 2015 at 9:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Lars Schneider <larsxschneider@gmail.com> writes:
>>
>>> This works.
>>
>> OK, and thanks; as I don't do perforce, the squash was without any
>> testing.
>>
>>> Do we need the “-e” option?
>>
>> In syntactic sense, no, but our codebase tends to prefer to have
>> one, because it is easier to spot which ones are the instructions if
>> you consistently have "-e" even when you give only one.
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

Thanks a lot for taking care of this!

- Lars

On 22 Sep 2015, at 21:17, Junio C Hamano <gitster@pobox.com> wrote:

> Yup, this was privately reported and I just squashed a fix in right now ;-)
> 
> Thanks. "cd t && make test-lint" would have caught it.
> 
> On Tue, Sep 22, 2015 at 12:11 PM, Michael Blume <blume.mike@gmail.com> wrote:
>> I'm seeing test failures
>> 
>> non-executable tests: t9825-git-p4-handle-utf16-without-bom.sh
>> 
>> ls -l shows that all the other tests are executable but t9825 isn't.
>> 
>> On Tue, Sep 22, 2015 at 9:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Lars Schneider <larsxschneider@gmail.com> writes:
>>> 
>>>> This works.
>>> 
>>> OK, and thanks; as I don't do perforce, the squash was without any
>>> testing.
>>> 
>>>> Do we need the “-e” option?
>>> 
>>> In syntactic sense, no, but our codebase tends to prefer to have
>>> one, because it is easier to spot which ones are the instructions if
>>> you consistently have "-e" even when you give only one.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html