threads / patch / 40387

v3, 2 partsgit-p4: handle "Translation of file content failed"

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

## tl;dr

12 messages between Sep 20, 2015 and Sep 21, 2015. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

larsxschneider@gmail.com· Sep 20, 2015, 16:22 UTC · lore

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

From: Lars Schneider <larsxschneider@gmail.com>
diff to v2:
* remove Perl calls for printing characters (Thanks Torsten!)
* remove whitespaces after ">" (Thanks Torsten!)
* add test case to show that the fix is not working for non "--verbose" mode (Thanks Luke!)
* add P4 knowledge base link with detailed error description to commit message (Thanks Luke!)
* remove unnecessary .gitattributes file in test data

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/t9824-git-p4-handle-utf16-without-bom.sh | 49 ++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 11 deletions(-)
 create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh

-- 2.5.1

larsxschneider@gmail.com· Sep 20, 2015, 16:22 UTC · re: larsxschneider@gmail.com · lore

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

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/t9824-git-p4-handle-utf16-without-bom.sh | 49 ++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh
Show changes to t/t9824-git-p4-handle-utf16-without-bom.sh +49 −0
diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh b/t/t9824-git-p4-handle-utf16-without-bom.sh
new file mode 100755
index 0000000..517f6da
--- /dev/null
+++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
@@ -0,0 +1,49 @@
+#!/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 -i.bak "$ d" depot/file1,v &&
+		printf "@$UTF16@" >>depot/file1,v &&
+		p4d -jrF checkpoint.1
+	)
+'
+
+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 &&
+	(
+		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
Eric Sunshine· Sep 20, 2015, 21:16 UTC · re: larsxschneider@gmail.com · lore

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

On Sun, Sep 20, 2015 at 12:22 PM,  <larsxschneider@gmail.com> wrote:
> 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.

Hmm, are these tests going to succeed only after patch 2/2 is applied? If so, the order of these patches is backward since you want each patch to be able to stand on its own and not introduce any sort of breakage.

Show 61 quoted lines
> More info here: http://answers.perforce.com/articles/KB/3117
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh b/t/t9824-git-p4-handle-utf16-without-bom.sh
> new file mode 100755
> index 0000000..517f6da
> --- /dev/null
> +++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
> @@ -0,0 +1,49 @@
> +#!/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 -i.bak "$ d" depot/file1,v &&
> +               printf "@$UTF16@" >>depot/file1,v &&
> +               p4d -jrF checkpoint.1
> +       )
> +'
> +
> +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 &&
> +       (
> +               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
Lars Schneider· Sep 20, 2015, 21:34 UTC · re: Eric Sunshine · lore

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

On 20 Sep 2015, at 23:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
> On Sun, Sep 20, 2015 at 12:22 PM,  <larsxschneider@gmail.com> wrote:
>> 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.
> 
> Hmm, are these tests going to succeed only after patch 2/2 is applied?
> If so, the order of these patches is backward since you want each
> patch to be able to stand on its own and not introduce any sort of
> breakage.

Yes, these tests succeed only after 2/2. I think I saw this approach somewhere in the Git history. I thought it would ease the reviewing process: show the problem in the first commit, fix it in a subsequent commit. However, I understand your point as 1/2 would break the build.

What is the preferred way by the Git community? Combine patch and test in one commit or a patch commit followed by a test commit? I would prefer to have everything in one commit.
- Lars
Eric Sunshine· Sep 20, 2015, 22:29 UTC · re: Lars Schneider · lore

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

On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 17 quoted lines
> On 20 Sep 2015, at 23:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Sep 20, 2015 at 12:22 PM,  <larsxschneider@gmail.com> wrote:
>>> 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.
>>
>> Hmm, are these tests going to succeed only after patch 2/2 is applied?
>> If so, the order of these patches is backward since you want each
>> patch to be able to stand on its own and not introduce any sort of
>> breakage.
>
> Yes, these tests succeed only after 2/2. I think I saw this approach
> somewhere in the Git history. I thought it would ease the reviewing
> process: show the problem in the first commit, fix it in a
> subsequent commit.  However, I understand your point as 1/2 would
> break the build.

Yes, people sometimes do that, however, the patch which demonstrates the problem uses test_expect_failure, and the follow-up patch which fixes the problem flips it to test_expect_success.

> What is the preferred way by the Git community? Combine patch and
> test in one commit or a patch commit followed by a test commit? I
> would prefer to have everything in one commit.

If the tests are in a separate patch, Junio seems to prefer adding them after the problem is fixes; the idea being that tests are added to ensure that some future change doesn't break the feature, as opposed to showing that your patch fixes a bug.

Whether or not to combine the fix with the new tests often depends upon the length of the patches and how easy or hard it is to review them. In this case, the fix itself is fairly short, but the tests are slightly lengthy, so there may not be a clear cut answer. As a reviewer, I tend to prefer smaller patches, however, this situation doesn't demand it, so use your best judgment.

Luke Diamand· Sep 21, 2015, 07:47 UTC · re: Eric Sunshine · lore

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

On 20/09/15 23:29, Eric Sunshine wrote:
Show 18 quoted lines
> On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>
>> What is the preferred way by the Git community? Combine patch and
>> test in one commit or a patch commit followed by a test commit? I
>> would prefer to have everything in one commit.
>
> If the tests are in a separate patch, Junio seems to prefer adding
> them after the problem is fixes; the idea being that tests are added
> to ensure that some future change doesn't break the feature, as
> opposed to showing that your patch fixes a bug.
>
> Whether or not to combine the fix with the new tests often depends
> upon the length of the patches and how easy or hard it is to review
> them. In this case, the fix itself is fairly short, but the tests are
> slightly lengthy, so there may not be a clear cut answer. As a
> reviewer, I tend to prefer smaller patches, however, this situation
> doesn't demand it, so use your best judgment.

I think in the past we have a test added that demonstrates the problem, with "test_expect_failure", followed by the fix, which also flips the test to "test_expect_success".

Luke
Junio C Hamano· Sep 21, 2015, 17:41 UTC · re: Lars Schneider · lore

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

Lars Schneider <larsxschneider@gmail.com> writes:
Show 21 quoted lines
> On 20 Sep 2015, at 23:16, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
>> On Sun, Sep 20, 2015 at 12:22 PM,  <larsxschneider@gmail.com> wrote:
>>> 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.
>> 
>> Hmm, are these tests going to succeed only after patch 2/2 is applied?
>> If so, the order of these patches is backward since you want each
>> patch to be able to stand on its own and not introduce any sort of
>> breakage.
> Yes, these tests succeed only after 2/2. I think I saw this approach
> somewhere in the Git history. I thought it would ease the reviewing
> process: show the problem in the first commit, fix it in a subsequent
> commit.
> However, I understand your point as 1/2 would break the build.
>
> What is the preferred way by the Git community? Combine patch and test
> in one commit or a patch commit followed by a test commit? I would
> prefer to have everything in one commit.

A single patch is fine and usually preferable when the patch does not span all over the tree.

Luke Diamand· Sep 21, 2015, 07:49 UTC · re: larsxschneider@gmail.com · lore

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

On 20/09/15 17:22, larsxschneider@gmail.com wrote:
> From: Lars Schneider <larsxschneider@gmail.com>
When I run this, I get errors reported on the sed usage:

t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable: sed -i.bak "$ d" depot/file1,v && t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable: sed -i.bak "$ d" depot/file1,v &&

Luke
Show 45 quoted lines
>
> 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/t9824-git-p4-handle-utf16-without-bom.sh | 49 ++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>   create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh
>
> diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh b/t/t9824-git-p4-handle-utf16-without-bom.sh
> new file mode 100755
> index 0000000..517f6da
> --- /dev/null
> +++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
> @@ -0,0 +1,49 @@
> +#!/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 -i.bak "$ d" depot/file1,v &&
This line is the problem I think.
Show 25 quoted lines
> +		printf "@$UTF16@" >>depot/file1,v &&
> +		p4d -jrF checkpoint.1
> +	)
> +'
> +
> +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 &&
> +	(
> +		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
>
Lars Schneider· Sep 21, 2015, 09:05 UTC · re: Luke Diamand · lore

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

On 21 Sep 2015, at 09:49, Luke Diamand <luke@diamand.org> wrote:
Show 7 quoted lines
> On 20/09/15 17:22, larsxschneider@gmail.com wrote:
>> From: Lars Schneider <larsxschneider@gmail.com>
> 
> When I run this, I get errors reported on the sed usage:
> 
> t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable:             sed -i.bak "$ d" depot/file1,v &&
> t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable:             sed -i.bak "$ d" depot/file1,v &&
I tried it on OS X 10.9.5 and on Ubuntu Linux 14.04.1 (sed version 4.2.2).

The “-i” option is mentioned in the GNU sed docs here: https://www.gnu.org/software/sed/manual/sed.html

The OS X sed man page indeed lists “-i” as non-standard option: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/sed.1.html

What OS/sed version are you using?

Thanks, Lars

Show 78 quoted lines
> 
> 
> Luke
> 
> 
>> 
>> 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/t9824-git-p4-handle-utf16-without-bom.sh | 49 ++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>  create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh
>> 
>> diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> new file mode 100755
>> index 0000000..517f6da
>> --- /dev/null
>> +++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> @@ -0,0 +1,49 @@
>> +#!/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 -i.bak "$ d" depot/file1,v &&
> 
> This line is the problem I think.
> 
> 
>> +		printf "@$UTF16@" >>depot/file1,v &&
>> +		p4d -jrF checkpoint.1
>> +	)
>> +'
>> +
>> +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 &&
>> +	(
>> +		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
Torsten Bögershausen· Sep 21, 2015, 09:14 UTC · re: Lars Schneider · lore

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

On 09/21/2015 11:05 AM, Lars Schneider wrote:
Show 14 quoted lines
> I tried it on OS X 10.9.5 and on Ubuntu Linux 14.04.1 (sed version 4.2.2).
>
> The “-i” option is mentioned in the GNU sed docs here:
> https://www.gnu.org/software/sed/manual/sed.html
>
> The OS X sed man page indeed lists “-i” as non-standard option:
> https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/sed.1.html
>
> What OS/sed version are you using?
>
> Thanks,
> Lars
>
>

sed -i is not portable. (I think I missed that in the review :-()

The gnu version of sed supports "-i", but the POSIX doesn't: <http://pubs.opengroup.org/onlinepubs/007904975/utilities/sed.html>

Junio C Hamano· Sep 21, 2015, 17:43 UTC · re: Lars Schneider · lore

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

Lars Schneider <larsxschneider@gmail.com> writes:
> What OS/sed version are you using?
You should go with POSIX.1
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
larsxschneider@gmail.com· Sep 20, 2015, 16:22 UTC · re: larsxschneider@gmail.com · lore
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 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)
Show changes to git-p4.py +16 −11
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

-- 
2.5.1

← back to recent threads