@matheusbsbs / matheus-bagliano / commits / f7b5ff607f

git-p4: improve encoding handling to support inconsistent encodings

git-p4 is designed to run correctly under python2.7 and python3, but its functional behavior wrt importing user-entered text differs across these environments: Under python2, git-p4 "naively" writes the Perforce bytestream into git metadata (and does not set an "encoding" header on the commits); this means that any non-utf-8 byte sequences end up creating invalidly-encoded commit metadata in git. Under python3, git-p4 attempts to decode the Perforce bytestream as utf-8 data, and fails badly (with an unhelpful error) when non-utf-8 data is encountered. Perforce clients (especially p4v) encourage user entry of changelist descriptions (and user full names) in OS-local encoding, and store the resulting bytestream to the server unmodified - such that different clients can end up creating mutually-unintelligible messages. The most common inconsistency, in many Perforce environments, is likely to be utf-8 (typical in linux) vs cp-1252 (typical in windows). Make the changelist-description- and user-fullname-handling code python-runtime-agnostic, introducing three "strategies" selectable via config: - 'passthrough', behaving as previously under python2, - 'strict', behaving as previously under python3, and - 'fallback', favoring utf-8 but supporting a secondary encoding when utf-8 decoding fails, and finally escaping high-range bytes if the decoding with the secondary encoding also fails. Keep the python2 default behavior as-is ('legacy' strategy), but switch the python3 default strategy to 'fallback' with default fallback encoding 'cp1252'. Also include tests exercising these encoding strategies, documentation for the new config, and improve the user-facing error messages when decoding does fail. Signed-off-by: Tao Klerks <tao@klerks.biz> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Tao Klerks committed Apr 30, 2022 at 19:26 UTC f7b5ff607fa1a62a480399afb6ccb9691735bf79

5 files changed +572 -18

Documentation/git-p4.txt

+36 -1

@@ -636,7 +636,42 @@ git-p4.pathEncoding::

636 Git expects paths encoded as UTF-8. Use this config to tell git-p4

637 what encoding Perforce had used for the paths. This encoding is used

638 to transcode the paths to UTF-8. As an example, Perforce on Windows

639 - often uses "cp1252" to encode path names.

639 + often uses "cp1252" to encode path names. If this option is passed

640 + into a p4 clone request, it is persisted in the resulting new git

641 + repo.

642 +

643 +git-p4.metadataDecodingStrategy::

644 + Perforce keeps the encoding of a changelist descriptions and user

645 + full names as stored by the client on a given OS. The p4v client

646 + uses the OS-local encoding, and so different users can end up storing

647 + different changelist descriptions or user full names in different

648 + encodings, in the same depot.

649 + Git tolerates inconsistent/incorrect encodings in commit messages

650 + and author names, but expects them to be specified in utf-8.

651 + git-p4 can use three different decoding strategies in handling the

652 + encoding uncertainty in Perforce: 'passthrough' simply passes the

653 + original bytes through from Perforce to git, creating usable but

654 + incorrectly-encoded data when the Perforce data is encoded as

655 + anything other than utf-8. 'strict' expects the Perforce data to be

656 + encoded as utf-8, and fails to import when this is not true.

657 + 'fallback' attempts to interpret the data as utf-8, and otherwise

658 + falls back to using a secondary encoding - by default the common

659 + windows encoding 'cp-1252' - with upper-range bytes escaped if

660 + decoding with the fallback encoding also fails.

661 + Under python2 the default strategy is 'passthrough' for historical

662 + reasons, and under python3 the default is 'fallback'.

663 + When 'strict' is selected and decoding fails, the error message will

664 + propose changing this config parameter as a workaround. If this

665 + option is passed into a p4 clone request, it is persisted into the

666 + resulting new git repo.

667 +

668 +git-p4.metadataFallbackEncoding::

669 + Specify the fallback encoding to use when decoding Perforce author

670 + names and changelists descriptions using the 'fallback' strategy

671 + (see git-p4.metadataDecodingStrategy). The fallback encoding will

672 + only be used when decoding as utf-8 fails. This option defaults to

673 + cp1252, a common windows encoding. If this option is passed into a

674 + p4 clone request, it is persisted into the resulting new git repo.

675

676 git-p4.largeFileSystem::

677 Specify the system that is used for large (binary) files. Please note

git-p4.py

+107 -16

@@ -15,6 +15,7 @@

15 # pylint: disable=too-many-statements,too-many-instance-attributes

16 # pylint: disable=too-many-branches,too-many-nested-blocks

17 #

18 +import struct

19 import sys

20 if sys.version_info.major < 3 and sys.version_info.minor < 7:

21 sys.stderr.write("git-p4: requires Python 2.7 or later.\n")

@@ -54,6 +55,9 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'

55 # The block size is reduced automatically if required

56 defaultBlockSize = 1<<20

58 +defaultMetadataDecodingStrategy = 'passthrough' if sys.version_info.major == 2 else 'fallback'

59 +defaultFallbackMetadataEncoding = 'cp1252'

60 +

61 p4_access_checked = False

63 re_ko_keywords = re.compile(br'\$(Id|Header)(:[^$\n]+)?\$')

@@ -203,6 +207,70 @@ else:

207 def encode_text_stream(s):

208 return s.encode('utf_8') if isinstance(s, unicode) else s

209

210 +

211 +class MetadataDecodingException(Exception):

212 + def __init__(self, input_string):

213 + self.input_string = input_string

214 +

215 + def __str__(self):

216 + return """Decoding perforce metadata failed!

217 +The failing string was:

218 +---

219 +{}

220 +---

221 +Consider setting the git-p4.metadataDecodingStrategy config option to

222 +'fallback', to allow metadata to be decoded using a fallback encoding,

223 +defaulting to cp1252.""".format(self.input_string)

224 +

225 +

226 +encoding_fallback_warning_issued = False

227 +encoding_escape_warning_issued = False

228 +def metadata_stream_to_writable_bytes(s):

229 + encodingStrategy = gitConfig('git-p4.metadataDecodingStrategy') or defaultMetadataDecodingStrategy

230 + fallbackEncoding = gitConfig('git-p4.metadataFallbackEncoding') or defaultFallbackMetadataEncoding

231 + if not isinstance(s, bytes):

232 + return s.encode('utf_8')

233 + if encodingStrategy == 'passthrough':

234 + return s

235 + try:

236 + s.decode('utf_8')

237 + return s

238 + except UnicodeDecodeError:

239 + if encodingStrategy == 'fallback' and fallbackEncoding:

240 + global encoding_fallback_warning_issued

241 + global encoding_escape_warning_issued

242 + try:

243 + if not encoding_fallback_warning_issued:

244 + print("\nCould not decode value as utf-8; using configured fallback encoding %s: %s" % (fallbackEncoding, s))

245 + print("\n(this warning is only displayed once during an import)")

246 + encoding_fallback_warning_issued = True

247 + return s.decode(fallbackEncoding).encode('utf_8')

248 + except Exception as exc:

249 + if not encoding_escape_warning_issued:

250 + print("\nCould not decode value with configured fallback encoding %s; escaping bytes over 127: %s" % (fallbackEncoding, s))

251 + print("\n(this warning is only displayed once during an import)")

252 + encoding_escape_warning_issued = True

253 + escaped_bytes = b''

254 + # bytes and strings work very differently in python2 vs python3...

255 + if str is bytes:

256 + for byte in s:

257 + byte_number = struct.unpack('>B', byte)[0]

258 + if byte_number > 127:

259 + escaped_bytes += b'%'

260 + escaped_bytes += hex(byte_number)[2:].upper()

261 + else:

262 + escaped_bytes += byte

263 + else:

264 + for byte_number in s:

265 + if byte_number > 127:

266 + escaped_bytes += b'%'

267 + escaped_bytes += hex(byte_number).upper().encode()[2:]

268 + else:

269 + escaped_bytes += bytes([byte_number])

270 + return escaped_bytes

271 +

272 + raise MetadataDecodingException(s)

273 +

274 def decode_path(path):

275 """Decode a given string (bytes or otherwise) using configured path encoding options

276 """

@@ -702,11 +770,12 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

770 if bytes is not str:

771 # Decode unmarshalled dict to use str keys and values, except for:

772 # - `data` which may contain arbitrary binary data

705 - # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text

773 + # - `desc` or `FullName` which may contain non-UTF8 encoded text handled below, eagerly converted to bytes

774 + # - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text, handled by decode_path()

775 decoded_entry = {}

776 for key, value in entry.items():

777 key = key.decode()

709 - if isinstance(value, bytes) and not (key in ('data', 'path', 'clientFile') or key.startswith('depotFile')):

778 + if isinstance(value, bytes) and not (key in ('data', 'desc', 'FullName', 'path', 'clientFile') or key.startswith('depotFile')):

779 value = value.decode()

780 decoded_entry[key] = value

781 # Parse out data if it's an error response

@@ -716,6 +785,10 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

785 if skip_info:

786 if 'code' in entry and entry['code'] == 'info':

787 continue

788 + if 'desc' in entry:

789 + entry['desc'] = metadata_stream_to_writable_bytes(entry['desc'])

790 + if 'FullName' in entry:

791 + entry['FullName'] = metadata_stream_to_writable_bytes(entry['FullName'])

792 if cb is not None:

793 cb(entry)

794 else:

@@ -1435,7 +1508,13 @@ class P4UserMap:

1508 for output in p4CmdList(["users"]):

1509 if "User" not in output:

1510 continue

1438 - self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"

1511 + # "FullName" is bytes. "Email" on the other hand might be bytes

1512 + # or unicode string depending on whether we are running under

1513 + # python2 or python3. To support

1514 + # git-p4.metadataDecodingStrategy=fallback, self.users dict values

1515 + # are always bytes, ready to be written to git.

1516 + emailbytes = metadata_stream_to_writable_bytes(output["Email"])

1517 + self.users[output["User"]] = output["FullName"] + b" <" + emailbytes + b">"

1518 self.emails[output["Email"]] = output["User"]

1519

1520 mapUserConfigRegex = re.compile(r"^\s*(\S+)\s*=\s*(.+)\s*<(\S+)>\s*$", re.VERBOSE)

@@ -1445,26 +1524,28 @@ class P4UserMap:

1524 user = mapUser[0][0]

1525 fullname = mapUser[0][1]

1526 email = mapUser[0][2]

1448 - self.users[user] = fullname + " <" + email + ">"

1527 + fulluser = fullname + " <" + email + ">"

1528 + self.users[user] = metadata_stream_to_writable_bytes(fulluser)

1529 self.emails[email] = user

1530

1451 - s = ''

1531 + s = b''

1532 for (key, val) in self.users.items():

1453 - s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))

1533 + keybytes = metadata_stream_to_writable_bytes(key)

1534 + s += b"%s\t%s\n" % (keybytes.expandtabs(1), val.expandtabs(1))

1535

1455 - open(self.getUserCacheFilename(), 'w').write(s)

1536 + open(self.getUserCacheFilename(), 'wb').write(s)

1537 self.userMapFromPerforceServer = True

1538

1539 def loadUserMapFromCache(self):

1540 self.users = {}

1541 self.userMapFromPerforceServer = False

1542 try:

1462 - cache = open(self.getUserCacheFilename(), 'r')

1543 + cache = open(self.getUserCacheFilename(), 'rb')

1544 lines = cache.readlines()

1545 cache.close()

1546 for line in lines:

1466 - entry = line.strip().split("\t")

1467 - self.users[entry[0]] = entry[1]

1547 + entry = line.strip().split(b"\t")

1548 + self.users[entry[0].decode('utf_8')] = entry[1]

1549 except IOError:

1550 self.getUserMapFromPerforceServer()

1551

@@ -3020,7 +3101,8 @@ class P4Sync(Command, P4UserMap):

3101 if userid in self.users:

3102 return self.users[userid]

3103 else:

3023 - return "%s <a@b>" % userid

3104 + userid_bytes = metadata_stream_to_writable_bytes(userid)

3105 + return b"%s <a@b>" % userid_bytes

3106

3107 def streamTag(self, gitStream, labelName, labelDetails, commit, epoch):

3108 """ Stream a p4 tag.

@@ -3043,9 +3125,10 @@ class P4Sync(Command, P4UserMap):

3125 email = self.make_email(owner)

3126 else:

3127 email = self.make_email(self.p4UserId())

3046 - tagger = "%s %s %s" % (email, epoch, self.tz)

3128

3048 - gitStream.write("tagger %s\n" % tagger)

3129 + gitStream.write("tagger ")

3130 + gitStream.write(email)

3131 + gitStream.write(" %s %s\n" % (epoch, self.tz))

3132

3133 print("labelDetails=",labelDetails)

3134 if 'Description' in labelDetails:

@@ -3138,12 +3221,12 @@ class P4Sync(Command, P4UserMap):

3221 self.gitStream.write("commit %s\n" % branch)

3222 self.gitStream.write("mark :%s\n" % details["change"])

3223 self.committedChanges.add(int(details["change"]))

3141 - committer = ""

3224 if author not in self.users:

3225 self.getUserMapFromPerforceServer()

3144 - committer = "%s %s %s" % (self.make_email(author), epoch, self.tz)

3226

3146 - self.gitStream.write("committer %s\n" % committer)

3227 + self.gitStream.write("committer ")

3228 + self.gitStream.write(self.make_email(author))

3229 + self.gitStream.write(" %s %s\n" % (epoch, self.tz))

3230

3231 self.gitStream.write("data <<EOT\n")

3232 self.gitStream.write(details["desc"])

@@ -4055,6 +4138,14 @@ class P4Clone(P4Sync):

4138 if self.useClientSpec_from_options:

4139 system(["git", "config", "--bool", "git-p4.useclientspec", "true"])

4140

4141 + # persist any git-p4 encoding-handling config options passed in for clone:

4142 + if gitConfig('git-p4.metadataDecodingStrategy'):

4143 + system(["git", "config", "git-p4.metadataDecodingStrategy", gitConfig('git-p4.metadataDecodingStrategy')])

4144 + if gitConfig('git-p4.metadataFallbackEncoding'):

4145 + system(["git", "config", "git-p4.metadataFallbackEncoding", gitConfig('git-p4.metadataFallbackEncoding')])

4146 + if gitConfig('git-p4.pathEncoding'):

4147 + system(["git", "config", "git-p4.pathEncoding", gitConfig('git-p4.pathEncoding')])

4148 +

4149 return True

4150

4151 class P4Unshelve(Command):

t/lib-git-p4.sh

+2 -1

@@ -142,10 +142,11 @@ start_p4d () {

142

143 p4_add_user () {

144 name=$1 &&

145 + fullname="${2:-Dr. $1}"

146 p4 user -f -i <<-EOF

147 User: $name

148 Email: $name@example.com

148 - FullName: Dr. $name

149 + FullName: $fullname

150 EOF

151 }

152

t/t9835-git-p4-metadata-encoding-python2.sh new

+213

@@ -0,0 +1,213 @@

1 +#!/bin/sh

2 +

3 +test_description='git p4 metadata encoding

4 +

5 +This test checks that the import process handles inconsistent text

6 +encoding in p4 metadata (author names, commit messages, etc) without

7 +failing, and produces maximally sane output in git.'

8 +

9 +. ./lib-git-p4.sh

10 +

11 +python_target_version='2'

12 +

13 +###############################

14 +## SECTION REPEATED IN t9836 ##

15 +###############################

16 +

17 +# Please note: this test calls "git-p4.py" rather than "git-p4", because the

18 +# latter references a specific path so we can't easily force it to run under

19 +# the python version we need to.

20 +

21 +python_major_version=$(python -V 2>&1 | cut -c 8)

22 +python_target_binary=$(which python$python_target_version)

23 +if ! test "$python_major_version" = "$python_target_version" && test "$python_target_binary"

24 +then

25 + mkdir temp_python

26 + PATH="$(pwd)/temp_python:$PATH" && export PATH

27 + ln -s $python_target_binary temp_python/python

28 +fi

29 +

30 +python_major_version=$(python -V 2>&1 | cut -c 8)

31 +if ! test "$python_major_version" = "$python_target_version"

32 +then

33 + skip_all="skipping python$python_target_version-specific git p4 tests; python$python_target_version not available"

34 + test_done

35 +fi

36 +

37 +remove_user_cache () {

38 + rm "$HOME/.gitp4-usercache.txt" || true

39 +}

40 +

41 +test_expect_success 'start p4d' '

42 + start_p4d

43 +'

44 +

45 +test_expect_success 'init depot' '

46 + (

47 + cd "$cli" &&

48 +

49 + p4_add_user "utf8_author" "ǣuthor" &&

50 + P4USER=utf8_author &&

51 + touch file1 &&

52 + p4 add file1 &&

53 + p4 submit -d "first CL has some utf-8 tǣxt" &&

54 +

55 + p4_add_user "latin1_author" "$(echo æuthor |

56 + iconv -f utf8 -t latin1)" &&

57 + P4USER=latin1_author &&

58 + touch file2 &&

59 + p4 add file2 &&

60 + p4 submit -d "$(echo second CL has some latin-1 tæxt |

61 + iconv -f utf8 -t latin1)" &&

62 +

63 + p4_add_user "cp1252_author" "$(echo æuthœr |

64 + iconv -f utf8 -t cp1252)" &&

65 + P4USER=cp1252_author &&

66 + touch file3 &&

67 + p4 add file3 &&

68 + p4 submit -d "$(echo third CL has sœme cp-1252 tæxt |

69 + iconv -f utf8 -t cp1252)" &&

70 +

71 + p4_add_user "cp850_author" "$(echo Åuthor |

72 + iconv -f utf8 -t cp850)" &&

73 + P4USER=cp850_author &&

74 + touch file4 &&

75 + p4 add file4 &&

76 + p4 submit -d "$(echo fourth CL hÅs some cp850 text |

77 + iconv -f utf8 -t cp850)"

78 + )

79 +'

80 +

81 +test_expect_success 'clone non-utf8 repo with strict encoding' '

82 + test_when_finished cleanup_git &&

83 + test_when_finished remove_user_cache &&

84 + test_must_fail git -c git-p4.metadataDecodingStrategy=strict p4.py clone --dest="$git" //depot@all 2>err &&

85 + grep "Decoding perforce metadata failed!" err

86 +'

87 +

88 +test_expect_success 'check utf-8 contents with passthrough strategy' '

89 + test_when_finished cleanup_git &&

90 + test_when_finished remove_user_cache &&

91 + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest="$git" //depot@all &&

92 + (

93 + cd "$git" &&

94 + git log >actual &&

95 + grep "some utf-8 tǣxt" actual &&

96 + grep "ǣuthor" actual

97 + )

98 +'

99 +

100 +test_expect_success 'check latin-1 contents corrupted in git with passthrough strategy' '

101 + test_when_finished cleanup_git &&

102 + test_when_finished remove_user_cache &&

103 + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest="$git" //depot@all &&

104 + (

105 + cd "$git" &&

106 + git log >actual &&

107 + badly_encoded_in_git=$(echo "some latin-1 tæxt" | iconv -f utf8 -t latin1) &&

108 + grep "$badly_encoded_in_git" actual &&

109 + bad_author_in_git="$(echo æuthor | iconv -f utf8 -t latin1)" &&

110 + grep "$bad_author_in_git" actual

111 + )

112 +'

113 +

114 +test_expect_success 'check utf-8 contents with fallback strategy' '

115 + test_when_finished cleanup_git &&

116 + test_when_finished remove_user_cache &&

117 + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest="$git" //depot@all &&

118 + (

119 + cd "$git" &&

120 + git log >actual &&

121 + grep "some utf-8 tǣxt" actual &&

122 + grep "ǣuthor" actual

123 + )

124 +'

125 +

126 +test_expect_success 'check latin-1 contents with fallback strategy' '

127 + test_when_finished cleanup_git &&

128 + test_when_finished remove_user_cache &&

129 + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest="$git" //depot@all &&

130 + (

131 + cd "$git" &&

132 + git log >actual &&

133 + grep "some latin-1 tæxt" actual &&

134 + grep "æuthor" actual

135 + )

136 +'

137 +

138 +test_expect_success 'check cp-1252 contents with fallback strategy' '

139 + test_when_finished cleanup_git &&

140 + test_when_finished remove_user_cache &&

141 + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest="$git" //depot@all &&

142 + (

143 + cd "$git" &&

144 + git log >actual &&

145 + grep "sœme cp-1252 tæxt" actual &&

146 + grep "æuthœr" actual

147 + )

148 +'

149 +

150 +test_expect_success 'check cp850 contents parsed with correct fallback' '

151 + test_when_finished cleanup_git &&

152 + test_when_finished remove_user_cache &&

153 + git -c git-p4.metadataDecodingStrategy=fallback -c git-p4.metadataFallbackEncoding=cp850 p4.py clone --dest="$git" //depot@all &&

154 + (

155 + cd "$git" &&

156 + git log >actual &&

157 + grep "hÅs some cp850 text" actual &&

158 + grep "Åuthor" actual

159 + )

160 +'

161 +

162 +test_expect_success 'check cp850-only contents escaped when cp1252 is fallback' '

163 + test_when_finished cleanup_git &&

164 + test_when_finished remove_user_cache &&

165 + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest="$git" //depot@all &&

166 + (

167 + cd "$git" &&

168 + git log >actual &&

169 + grep "h%8Fs some cp850 text" actual &&

170 + grep "%8Futhor" actual

171 + )

172 +'

173 +

174 +test_expect_success 'check cp-1252 contents on later sync after clone with fallback strategy' '

175 + test_when_finished cleanup_git &&

176 + test_when_finished remove_user_cache &&

177 + git -c git-p4.metadataDecodingStrategy=fallback p4.py clone --dest="$git" //depot@all &&

178 + (

179 + cd "$cli" &&

180 + P4USER=cp1252_author &&

181 + touch file10 &&

182 + p4 add file10 &&

183 + p4 submit -d "$(echo later CL has sœme more cp-1252 tæxt |

184 + iconv -f utf8 -t cp1252)"

185 + ) &&

186 + (

187 + cd "$git" &&

188 +

189 + git p4.py sync --branch=master &&

190 +

191 + git log p4/master >actual &&

192 + grep "sœme more cp-1252 tæxt" actual &&

193 + grep "æuthœr" actual

194 + )

195 +'

196 +

197 +############################

198 +## / END REPEATED SECTION ##

199 +############################

200 +

201 +test_expect_success 'passthrough (latin-1 contents corrupted in git) is the default with python2' '

202 + test_when_finished cleanup_git &&

203 + test_when_finished remove_user_cache &&

204 + git -c git-p4.metadataDecodingStrategy=passthrough p4.py clone --dest="$git" //depot@all &&

205 + (

206 + cd "$git" &&

207 + git log >actual &&

208 + badly_encoded_in_git=$(echo "some latin-1 tæxt" | iconv -f utf8 -t latin1) &&

209 + grep "$badly_encoded_in_git" actual

210 + )

211 +'

212 +

213 +test_done

t/t9836-git-p4-metadata-encoding-python3.sh new

+214

@@ -0,0 +1,214 @@