Re: [PATCH v2] Extend sample pre-commit hook to check for non ascii filenames

[Jakub Narebski <jnareb@gmail.com>]

<Insert standard Dscho disclaimer here...> ;-)

In short: good idea, don't be discouraged by criticism...

On Thu, 14 May 2009, Heiko Voigt wrote:

> At the moment non-ascii encodings of filenames are not portably converted
> between different filesystems by git. This will most likely change in the
> future but to allow repositories to be portable among different file/operating
> systems this check is enabled by default.

By the way, you might consider choosing shorter line length for commits,
something around 70-76 characters per line; otherwise it is harder to
reply to without linewrapping. 80 characters that you used is, IMHO,
absolute maximum, and it is good that you kept to it.

> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---

> +# If you want to allow non-ascii filenames set this variable to true.
> +allownonascii=$(git config hooks.allownonascii)
> +
> +function is_ascii () {
> +    test -z "$(cat | sed -e "s/[\ -~]*//g")"
> +    return $?
> +}

>From CodingGuidelines for shell scripts:
 - We do not write the noiseword "function" in front of shell
   functions.

(in short: do not use bash-specific features... unless, of course,
you are modifying bash-completion script).

Second, it would be nice to have comment about how to use this
function (as it does not check file given by its argument, but
rather its standard input). And perhaps also a comment that it
works because ASCII printable characters begin with ' ' space
(does it have to be escaped?) and end with '~' tilde[2].

Third, isn't it useless use of 'cat'[3]? And wouldn't it be better
to use 'tr' to either delete printable characters and check for
anything left (as you do; BTW. wouldn't "return test ..." be simpler?),
or use 'tr' to count non portable characters?

[1] http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
[2] http://en.wikipedia.org/wiki/ASCII#ASCII_printable_characters
[3] http://partmaps.org/era/unix/award.html#cat

> +
> +if [ "$allownonascii" != "true" ]
> +then
> +	# until git can handle non-ascii filenames gracefully
> +	# prevent them to be added into the repository
> +	if ! git diff --cached --name-only --diff-filter=A -z \
> +			| tr "\0" "\n" | is_ascii; then
> +		echo "Non-ascii filenames are not allowed !"
> +		echo "Please rename the file ..."
> +		exit 1
> +	fi
> +fi
> +
>  if git-rev-parse --verify HEAD 2>/dev/null
>  then
>  	against=HEAD
> -- 
> 1.6.3
> 
> 
> 
> 

-- 
Jakub Narebski
Poland