Re: [PATCH 2/6] git-p4: handle utf16 filetype properly

From: Stefano Lattarini <stefano.lattarini@gmail.com>
Date: Oct 16, 2011, 09:52 UTC
Message-ID: <201110161152.32335.stefano.lattarini@gmail.com>
In-Reply-To: <20111015155641.GC29436@arf.padd.com>

Hello Pete.

One more "outsider nit" here ...

On Saturday 15 October 2011, Pete Wyckoff wrote:

Show 30 quoted lines

>
> [SNIP]
> 
> Add a test case to check utf16 handling, and +k and +ko handling.
>
> [SNIP]
>
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> new file mode 100755
> index 0000000..cf07e6d
> --- /dev/null
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -0,0 +1,108 @@
> +#!/bin/sh
> +
> +test_description='git-p4 p4 filetype tests'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> +	start_p4d
> +'
> +
> +test_expect_success 'utf-16 file create' '
> +	(
> +		cd "$cli" &&
> +
> +		# p4 saves this verbatim
> +		echo -e "three\nline\ntext" >f-ascii &&
>

Not portable to (at least) solaris /usr/xpg4/bin/sh and /bin/ksh:

 $ /bin/ksh -c 'echo -e "three\nline\ntext"'
 -e three
 line
 text

In fact, use of options and/or escape sequences in "echo" arguments is highly unportable (see the entry for `echo' in the "Limitations of Shell Builtins" section of the autoconf manual); your best bet is to use printf, which is more portable and well-behaved (at least on systems that are not musuem pieces).

Show 10 quoted lines

>
> [SNIP]
>
> +
> +build_smush() {
> +	cat >k_smush.py <<-EOF &&
> +	import re, sys
> +	sys.stdout.write(re.sub(r'(?i)\\\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\\\$', r'$\1$', sys.stdin.read()))
> +	EOF
>

This is a basically a stylistic nit, so fell free to disregard it completely, but ... wouldn't it be simpler to quote the "EOF" at the here-doc beginning, so that you don't have to escape all the backslashes in the here-doc itself? Similarly for other later usages.

Regards,
  Stefano