Re: [PATCH v4] git-p4: add config git-p4.pathEncoding

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2015, 16:30 UTC
Message-ID: <xmqqtwrcokaz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441205853-52065-2-git-send-email-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 10 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
>
> Perforce keeps the encoding of a path as given by the originating OS.
> Git expects paths encoded as UTF-8. Add a config 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 my Windows
> box uses “cp1252” to encode path names.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---

Thanks.

Show 7 quoted lines

> +git-p4.pathEncoding::
> +	Perforce keeps the encoding of a path as given by the originating OS.
> +	Git expects paths encoded as UTF-8. Use this config 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 my Windows
> +	box uses “cp1252” to encode path names.
> +

The log message is sort of personal statement, but "my Windows" smells out of place here, as there is no clear writer in the documentation. Perhaps rephrase it to:

    For example, Perforce often uses "cp1252" to encode path names
    on Windows box.

or something?

Show 16 quoted lines

> diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
> new file mode 100755
> index 0000000..3a1779a
> --- /dev/null
> +++ b/t/t9822-git-p4-path-encoding.sh
> @@ -0,0 +1,65 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories with non ASCII paths'
> +
> +. ./lib-git-p4.sh
> +
> +UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
> +ISO8859_ESCAPED="\141\55\344\137\157\55\366\137\165\55\374\56\164\170\164"
> +
> +# You can generate the ISO8859_ESCAPED with the following command:

Please don't. You don't want to encode ".txt" and other things that are ASCII, which you didn't encode in the original UTF8_ESCAPED.

> +# printf "$UTF8_ESCAPED" | \
> +# iconv -f utf-8 -t iso8859-1 | \
> +# xxd -ps -u -c 1 | xargs bash -c 'for v; do echo "ibase=16; obase=8; $v" | bc; done' bash | \
> +# tr "\n" "\\"

Besides, you somehow came up with UTF8_ESCAPED with a procedure that is not documented (and it does avoid hiding obvious things like ".txt" in the backslashes). I do not think it is necessary or even a good idea to give a procedure that is not very portable (xxd? bash?) and does not produce what we want to see, only for ISO8859.