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

From: Lars Schneider <redacted>

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

From: Lars Schneider <redacted>

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 <redacted>
---
 git-p4.py | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 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

-- 
2.5.1

From: Lars Schneider <redacted>

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 <redacted>
---
 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 &&
+		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

On Sun, Sep 20, 2015 at 12:22 PM,  [off-list ref] 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.

On 20 Sep 2015, at 23:16, Eric Sunshine [off-list ref] wrote:

On Sun, Sep 20, 2015 at 12:22 PM,  [off-list ref] wrote:

quoted

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

On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider
[off-list ref] wrote:

On 20 Sep 2015, at 23:16, Eric Sunshine [off-list ref] wrote:

quoted

On Sun, Sep 20, 2015 at 12:22 PM,  [off-list ref] wrote:

quoted

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.

On 20/09/15 23:29, Eric Sunshine wrote:

On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider
[off-list ref] wrote:

quoted

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

On 20/09/15 17:22, larsxschneider@gmail.com wrote:

From: Lars Schneider <redacted>

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

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

On 21 Sep 2015, at 09:49, Luke Diamand [off-list ref] wrote:

On 20/09/15 17:22, larsxschneider@gmail.com wrote:

quoted

From: Lars Schneider <redacted>

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

Luke

quoted

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 <redacted>
---
 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.

quoted

+		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

On 09/21/2015 11:05 AM, Lars Schneider wrote:

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>