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

[Junio C Hamano]

Lars Schneider  writes:

> On 20 Sep 2015, at 23:16, Eric Sunshine  wrote:
>
>> On Sun, Sep 20, 2015 at 12:22 PM,   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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Lars Schneider  writes:

> What OS/sed version are you using?

You should go with POSIX.1

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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:


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

> On 20/09/15 17:22, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider 
> 
> 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 
>> ---
>>  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 000..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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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, larsxschnei...@gmail.com wrote:

From: Lars Schneider 


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



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
 wrote:
> On 20 Sep 2015, at 23:16, Eric Sunshine  wrote:
>> On Sun, Sep 20, 2015 at 12:22 PM,   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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

> On Sun, Sep 20, 2015 at 12:22 PM,   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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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,   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 
> ---
> 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 000..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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[larsxschneider]

From: Lars Schneider 

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html