Re: [PATCH v3] git-p4: add "--path-encoding" option

From: Lars Schneider <larsxschneider@gmail.com>
Date: Sep 1, 2015, 13:42 UTC
Message-ID: <523D2C52-893E-4A54-9DD1-2DDBCEA29960@gmail.com>
In-Reply-To: <xmqqk2sbt5kv.fsf@gitster.mtv.corp.google.com>

On 01 Sep 2015, at 01:13, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
> 
> Here is a space for you to describe what it does and why it is a
> good idea to have it.

How about this:

Perforce keeps the encoding of a path as given by the originating OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4 what encoding Perforce had used for the paths. This encoding is used to transcode the paths to UTF-8. As an example, Perforce on Windows uses “cp1252” to encode path names.

Show 9 quoted lines

> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> Documentation/git-p4.txt        |  5 +++++
>> git-p4.py                       |  6 ++++++
>> t/t9821-git-p4-path-encoding.sh | 38 ++++++++++++++++++++++++++++++++++++++
> 
> I'll move this to 9822, as 9821 is taken by another git-p4 test,
> while queuing.

OK. I wasn’t sure how this is handled. Just for my understanding: As soon as a TC number is occupied in one of the official branches (master/next/pu/maint) then the next number should be used, right?

Show 19 quoted lines

> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 82aa5d6..14bb79c 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -252,6 +252,11 @@ Git repository:
>> 	Use a client spec to find the list of interesting files in p4.
>> 	See the "CLIENT SPEC" section below.
>> 
>> +--path-encoding <encoding>::
>> +	The encoding to use when reading p4 client paths. With this option
>> +	non ASCII paths are properly stored in Git. For example, the encoding
>> +	'cp1252' is often used on Windows systems.
>> +
> 
> Is this something that needs to be consistently given while
> interacting with the same P4 depot (in which case you may want to
> allow this to be given only once, record the value in the config and
> never allow it to be updated once it is set, or something)?

Good idea! I will add this. However, I really wonder why nobody tripped over this rather obvious bug already.

@Luke: Can you double check my fix?

Two general questions:

(1) I saw that the path encoding fix is already on “pu”. What is more convenient for you? Do you want to get a [PATCH v4] with one commit including the changes mentioned above or do you want me to keep the v3 commit and add a v4 commit on top?

(2) Is there a CI setup for Git somewhere accessible on the Internet? How about building and testing Git on Travis (https://travis-ci.org/git/git)?

Thanks, Lars