Re: [PATCH] git p4 fix for failure to decode p4 errors

[Nikolay Shustov <nikolay.shustov@gmail.com>]

ping, pretty please? :-)

On 3/19/25 23:20, Nikolay Shustov via GitGitGadget wrote:
> From: Nikolay Shustov <Nikolay.Shustov@gmail.com>
>
> Fixes the git p4 failure happening when Perforce command returns error
> containing byte stream of characters with high bit set. In such situations
> git p4 implementatino fails to decode this byte stream into utf-8 string.
>
> Design:
> Make use of existing decoding fallback strategy, described by
> git-p4.metadataDecodingStrategy and git-p4.metadataFallbackEncoding
> settings in the logic that decodes the Perforce command error bytes.
>
> Details:
> - Moved p4 metadata transcoding logic from
>    metadata_stream_to_writable_bytes(..) into a new MetadataTranscoder class.
> - Enhcanced the implementation to use git-p4.metadataDecodingStrategy and
>    git-p4.metadataFallbackEncoding settings for p4 errors decoding.
> - Added test.
>
> Signed-off-by: Nikolay Shustov <Nikolay.Shustov@gmail.com>
> ---
>      git p4 fix for failure to decode p4 errors
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1926%2Fnshustov%2Fgit-p4-error-decoding-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1926/nshustov/git-p4-error-decoding-v1
> Pull-Request: https://github.com/git/git/pull/1926
>
>   git-p4.py                        | 135 ++++++++++++++++++-------------
>   t/meson.build                    |   1 +
>   t/t9837-git-p4-error-encoding.sh |  53 ++++++++++++
>   t/t9837/git-p4-error-python3.py  |  15 ++++
>   4 files changed, 149 insertions(+), 55 deletions(-)
>   create mode 100755 t/t9837-git-p4-error-encoding.sh
>   create mode 100644 t/t9837/git-p4-error-python3.py
>
> diff --git a/git-p4.py b/git-p4.py
> index c0ca7becaf4..72a4c55f99e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -234,67 +234,91 @@ else:
>   
>   
>   class MetadataDecodingException(Exception):
> -    def __init__(self, input_string):
> +    def __init__(self, input_string, error=None):
>           self.input_string = input_string
> +        self.error = error
>   
>       def __str__(self):
> -        return """Decoding perforce metadata failed!
> +        message = """Decoding perforce metadata failed!
>   The failing string was:
>   ---
>   {}
>   ---
>   Consider setting the git-p4.metadataDecodingStrategy config option to
>   'fallback', to allow metadata to be decoded using a fallback encoding,
> -defaulting to cp1252.""".format(self.input_string)
> +defaulting to cp1252."""
> +        if verbose and self.error is not None:
> +            message += """
> +---
> +Error:
> +---
> +{}"""
> +        return message.format(self.input_string, self.error)
>   
>   
> -encoding_fallback_warning_issued = False
> -encoding_escape_warning_issued = False
> -def metadata_stream_to_writable_bytes(s):
> -    encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy
> -    fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding
> -    if not isinstance(s, bytes):
> -        return s.encode('utf_8')
> -    if encodingStrategy == 'passthrough':
> -        return s
> -    try:
> -        s.decode('utf_8')
> -        return s
> -    except UnicodeDecodeError:
> -        if encodingStrategy == 'fallback' and fallbackEncoding:
> -            global encoding_fallback_warning_issued
> -            global encoding_escape_warning_issued
> -            try:
> -                if not encoding_fallback_warning_issued:
> -                    print("\nCould not decode value as utf-8; using configured fallback encoding %s: %s" % (fallbackEncoding, s))
> -                    print("\n(this warning is only displayed once during an import)")
> -                    encoding_fallback_warning_issued = True
> -                return s.decode(fallbackEncoding).encode('utf_8')
> -            except Exception as exc:
> -                if not encoding_escape_warning_issued:
> -                    print("\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s" % (fallbackEncoding, s))
> -                    print("\n(this warning is only displayed once during an import)")
> -                    encoding_escape_warning_issued = True
> -                escaped_bytes = b''
> -                # bytes and strings work very differently in python2 vs python3...
> -                if str is bytes:
> -                    for byte in s:
> -                        byte_number = struct.unpack('>B', byte)[0]
> -                        if byte_number > 127:
> -                            escaped_bytes += b'%'
> -                            escaped_bytes += hex(byte_number)[2:].upper()
> -                        else:
> -                            escaped_bytes += byte
> -                else:
> -                    for byte_number in s:
> -                        if byte_number > 127:
> -                            escaped_bytes += b'%'
> -                            escaped_bytes += hex(byte_number).upper().encode()[2:]
> -                        else:
> -                            escaped_bytes += bytes([byte_number])
> -                return escaped_bytes
> +class MetadataTranscoder:
> +    def __init__(self, default_metadata_decoding_strategy, default_fallback_metadata_encoding):
> +        self.decoding_fallback_warning_issued = False
> +        self.decoding_escape_warning_issued = False
> +        self.decodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or default_metadata_decoding_strategy
> +        self.fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or default_fallback_metadata_encoding
> +
> +    def decode_metadata(self, s, error_from_fallback=True):
> +        try:
> +            return [s.decode('utf_8'), 'utf_8']
> +        except UnicodeDecodeError as decode_exception:
> +            error = decode_exception
> +            if self.decodingStrategy == 'fallback' and self.fallbackEncoding:
> +                try:
> +                    if not self.decoding_fallback_warning_issued:
> +                        print("\nCould not decode value as utf-8; using configured fallback encoding %s: %s" % (self.fallbackEncoding, s))
> +                        print("\n(this warning is only displayed once during an import)")
> +                        self.decoding_fallback_warning_issued = True
> +                    return [s.decode(self.fallbackEncoding), self.fallbackEncoding]
> +                except Exception as decode_exception:
> +                    if not error_from_fallback:
> +                        return [s, None]
> +                    error = decode_exception
> +            raise MetadataDecodingException(s, error)
> +
> +    def metadata_stream_to_writable_bytes(self, s):
> +        if not isinstance(s, bytes):
> +            return s.encode('utf_8')
> +        if self.decodingStrategy == 'passthrough':
> +            return s
> +
> +        [text, encoding] = self.decode_metadata(s, False)
> +        if encoding == 'utf_8':
> +            # s is of utf-8 already
> +            return s
> +
> +        if encoding is None:
> +            # could not decode s, even with fallback encoding
> +            if not self.decoding_escape_warning_issued:
> +                print("\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s" % (self.fallbackEncoding, s))
> +                print("\n(this warning is only displayed once during an import)")
> +                self.decoding_escape_warning_issued = True
> +            escaped_bytes = b''
> +            # bytes and strings work very differently in python2 vs python3...
> +            if str is bytes:
> +                for byte in s:
> +                    byte_number = struct.unpack('>B', byte)[0]
> +                    if byte_number > 127:
> +                        escaped_bytes += b'%'
> +                        escaped_bytes += hex(byte_number)[2:].upper()
> +                    else:
> +                        escaped_bytes += byte
> +            else:
> +                for byte_number in s:
> +                    if byte_number > 127:
> +                        escaped_bytes += b'%'
> +                        escaped_bytes += hex(byte_number).upper().encode()[2:]
> +                    else:
> +                        escaped_bytes += bytes([byte_number])
> +            return escaped_bytes
>   
> -        raise MetadataDecodingException(s)
> +        # were able to decode but not to utf-8
> +        return text.encode('utf_8')
>   
>   
>   def decode_path(path):
> @@ -898,14 +922,14 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
>                       decoded_entry[key] = value
>                   # Parse out data if it's an error response
>                   if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:
> -                    decoded_entry['data'] = decoded_entry['data'].decode()
> +                    decoded_entry['data'] = metadataTranscoder.decode_metadata(decoded_entry['data'])
>                   entry = decoded_entry
>               if skip_info:
>                   if 'code' in entry and entry['code'] == 'info':
>                       continue
>               for key in p4KeysContainingNonUtf8Chars():
>                   if key in entry:
> -                    entry[key] = metadata_stream_to_writable_bytes(entry[key])
> +                    entry[key] = metadataTranscoder.metadata_stream_to_writable_bytes(entry[key])
>               if cb is not None:
>                   cb(entry)
>               else:
> @@ -1718,7 +1742,7 @@ class P4UserMap:
>               # python2 or python3. To support
>               # git-p4.metadataDecodingStrategy=fallback, self.users dict values
>               # are always bytes, ready to be written to git.
> -            emailbytes = metadata_stream_to_writable_bytes(output["Email"])
> +            emailbytes = metadataTranscoder.metadata_stream_to_writable_bytes(output["Email"])
>               self.users[output["User"]] = output["FullName"] + b" <" + emailbytes + b">"
>               self.emails[output["Email"]] = output["User"]
>   
> @@ -1730,12 +1754,12 @@ class P4UserMap:
>                   fullname = mapUser[0][1]
>                   email = mapUser[0][2]
>                   fulluser = fullname + " <" + email + ">"
> -                self.users[user] = metadata_stream_to_writable_bytes(fulluser)
> +                self.users[user] = metadataTranscoder.metadata_stream_to_writable_bytes(fulluser)
>                   self.emails[email] = user
>   
>           s = b''
>           for (key, val) in self.users.items():
> -            keybytes = metadata_stream_to_writable_bytes(key)
> +            keybytes = metadataTranscoder.metadata_stream_to_writable_bytes(key)
>               s += b"%s\t%s\n" % (keybytes.expandtabs(1), val.expandtabs(1))
>   
>           open(self.getUserCacheFilename(), 'wb').write(s)
> @@ -3349,7 +3373,7 @@ class P4Sync(Command, P4UserMap):
>           if userid in self.users:
>               return self.users[userid]
>           else:
> -            userid_bytes = metadata_stream_to_writable_bytes(userid)
> +            userid_bytes = metadataTranscoder.metadata_stream_to_writable_bytes(userid)
>               return b"%s <a@b>" % userid_bytes
>   
>       def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):
> @@ -4561,6 +4585,7 @@ commands = {
>       "unshelve": P4Unshelve,
>   }
>   
> +metadataTranscoder = MetadataTranscoder(defaultMetadataDecodingStrategy, defaultFallbackMetadataEncoding)
>   
>   def main():
>       if len(sys.argv[1:]) == 0:
> diff --git a/t/meson.build b/t/meson.build
> index a59da26be3f..656424fdff3 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1090,6 +1090,7 @@ integration_tests = [
>     't9834-git-p4-file-dir-bug.sh',
>     't9835-git-p4-metadata-encoding-python2.sh',
>     't9836-git-p4-metadata-encoding-python3.sh',
> +  't9837-git-p4-error-encoding.sh',
>     't9850-shell.sh',
>     't9901-git-web--browse.sh',
>     't9902-completion.sh',
> diff --git a/t/t9837-git-p4-error-encoding.sh b/t/t9837-git-p4-error-encoding.sh
> new file mode 100755
> index 00000000000..1ea774afb1b
> --- /dev/null
> +++ b/t/t9837-git-p4-error-encoding.sh
> @@ -0,0 +1,53 @@
> +#!/bin/sh
> +
> +test_description='git p4 error encoding
> +
> +This test checks that the import process handles inconsistent text
> +encoding in p4 error messages without failing'
> +
> +. ./lib-git-p4.sh
> +
> +###############################
> +## SECTION REPEATED IN t9835 ##
> +###############################
> +
> +# These tests require Perforce with non-unicode setup.
> +out=$(2>&1 P4CHARSET=utf8 p4 client -o)
> +if test $? -eq 0
> +then
> +	skip_all="skipping git p4 error encoding tests; Perforce is setup with unicode"
> +	test_done
> +fi
> +
> +# These tests are specific to Python 3. Write a custom script that executes
> +# git-p4 directly with the Python 3 interpreter to ensure that we use that
> +# version even if Git was compiled with Python 2.
> +python_target_binary=$(which python3)
> +if test -n "$python_target_binary"
> +then
> +	mkdir temp_python
> +	PATH="$(pwd)/temp_python:$PATH"
> +	export PATH
> +
> +	write_script temp_python/git-p4-python3 <<-EOF
> +	exec "$python_target_binary" "$(git --exec-path)/git-p4" "\$@"
> +	EOF
> +fi
> +
> +git p4-python3 >err
> +if ! grep 'valid commands' err
> +then
> +	skip_all="skipping python3 git p4 tests; python3 not available"
> +	test_done
> +fi
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'see if Perforce error with characters not convertable to utf-8 will be processed correctly' '
> +	test_when_finished cleanup_git &&
> +	$python_target_binary "$TEST_DIRECTORY"/t9837/git-p4-error-python3.py "$TEST_DIRECTORY"
> +'
> +
> +test_done
> diff --git a/t/t9837/git-p4-error-python3.py b/t/t9837/git-p4-error-python3.py
> new file mode 100644
> index 00000000000..fb65aee386e
> --- /dev/null
> +++ b/t/t9837/git-p4-error-python3.py
> @@ -0,0 +1,15 @@
> +import os
> +import sys
> +from  importlib.machinery import SourceFileLoader
> +
> +def main():
> +    if len(sys.argv[1:]) != 1:
> +        print("Expected test directory name")
> +
> +    gitp4_path = sys.argv[1] + "/../git-p4.py"
> +    gitp4 = SourceFileLoader("gitp4", gitp4_path).load_module()
> +    gitp4.p4CmdList(["edit", b'\xFEfile'])
> +
> +if __name__ == '__main__':
> +    main()
> +
>
> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e