On 15 Sep 2015, at 08:43, Luke Diamand <l...@diamand.org> wrote:

> On 14/09/15 17:55, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@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
> 
> Sorry - what's a BOM? I'm assuming it's not a Bill of Materials?
BOM stands for Byte Order Mark. The UTF-16 BOM is a two byte sequence at the 
beginning of a UTF-16 file. It is not part of the actual content. It is only 
used to define the encoding of the remaining file. FEFF stands for UTF-16 
big-endian encoding and FFFE for little-endian encoding.

More info here: http://www.unicode.org/faq/utf_bom.html#bom1


> Do we know the mechanism by which we end up in this state?
Unfortunately no. I tried hard to reproduce the error with “conventional” 
methods. As you can see I ended up manipulating the P4 database…

However, it looks like this error happens in the wild, too:
https://stackoverflow.com/questions/5156909/translation-of-file-content-failed-error-in-perforce
https://stackoverflow.com/questions/887006/perforce-translation-of-file-content-failed-error


>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> 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.
> 
> Does that mean that the error will only be detected in verbose mode? That 
> doesn't seem right!
Correct. I don’t like this either but I also don’t want to make huge changes to 
git-p4.
You can see the root problem here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L110-L114

Any idea how to approach that best?


> 
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@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:
> 
> Would it be better to specify which kind of Exception you are catching? Looks 
> like you could get OSError, ValueError and CalledProcessError; it's the last 
> of these you want (I think).
I think it is just a plain exception. See here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L111


> 
>> +                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 ]
> 
> The indentation on this bit doesn't look right to me.
I believe it is exactly how it was:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L2397-L2399


In general, what is the appropriate way to reference code in this email list? 
Are GitHub links OK?


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

Reply via email to