From: Ben Keene Date: Thu, 05 Dec 2019 20:23:40 GMT Subject: Re: [PATCH v4 08/11] git-p4: p4CmdList - support Unicode encoding Message-ID: In-Reply-To: On 12/5/2019 8:55 AM, Junio C Hamano wrote: > "Ben Keene via GitGitGadget" writes: > >> From: Ben Keene >> >> The p4CmdList is a commonly used function in the git-p4 code. It is used to execute a command in P4 and return the results of the call in a list. > Somewhere in the midway of the series, the log message starts using > all-caps AS_STRING and AS_BYTES to describe some specific things, > and it would help readers if the first one of these steps explain > what they mean (I am guessing AS_STRING is an unicode object in both > Python 2 and 3, and AS_BYTES is a plain vanilla string in Python 2, > or something like that?). I rewrote almost the entire commit message. Hopefully this will clarify the code. >> Change this code to take a new optional parameter, encode_data that will optionally convert the data AS_STRING() that isto be returned by the function. > s/isto/is to/; > > This sentence is a bit hard to read. > > This change does not make the function optionally convert the input > we feed to the p4 command---it only changes the values in the > command output. But the readers cannot tell that easily until > reading to the very end of the sentence, i.e. "returned by the > function", as written. > > We probably want to be a bit more explicit to say what gets > converted; perhaps renaming the parameter to encode_cmd_output may > help. I renamed the parameter as suggested. >> Change the code so that the key will always be encoded AS_STRING() > s/key/key of the returned hash/ or something to clarify what key you > are talking about. > >> Data that is passed for standard input (stdin) should be AS_BYTES() to ensure unicode text that is supplied will be written out as bytes. > "Data that is passed to the standard input stream of the p4 process" > to clarify whose standard input you are talking about (iow, "git p4" > also has and it may use its standard input, but this function does > not muck with it). >