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

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

[larsxschneider]

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

[PATCH v3 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/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

[PATCH v3 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 ++++++++++++++++-----------
 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

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

[Eric Sunshine]

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.

> 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

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

[Lars Schneider]

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.

- Lars

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

[Eric Sunshine]

On Sun, Sep 20, 2015 at 5:34 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> 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.

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

[Luke Diamand]

On 20/09/15 23:29, Eric Sunshine wrote:
> 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

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

[Luke Diamand]

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


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

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

[Lars Schneider]

On 21 Sep 2015, at 09:49, Luke Diamand <luke@diamand.org> wrote:

> 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


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

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

[Torsten Bögershausen]

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>

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

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

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

[Junio C Hamano]

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