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

From: Luke Diamand <luke@diamand.org>
Date: Sep 15, 2015, 06:43 UTC
Message-ID: <55F7BE2A.80609@diamand.org>
In-Reply-To: <1442249728-89494-3-git-send-email-larsxschneider@gmail.com>

On 14/09/15 17:55, larsxschneider@gmail.com wrote:

> 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

Sorry - what's a BOM? I'm assuming it's not a Bill of Materials?

Do we know the mechanism by which we end up in this state?

Show 8 quoted lines

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

Show 40 quoted lines

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

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

Show 8 quoted lines

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

>
>           if type_base == "apple":
>               # Apple filetype files will be streamed as a concatenation of
>

Luke