Re: [PATCH] precompose-utf8: do not call checks for non-ascii "utf8"

From: Torsten Bögershausen <tboegi@web.de>
Date: Aug 20, 2012, 19:46 UTC
Message-ID: <5032940A.4040005@web.de>
In-Reply-To: <7vobm5jusz.fsf_-_@alter.siamese.dyndns.org>

On 08/20/2012 08:13 PM, Junio C Hamano wrote:

Show 61 quoted lines

> As suggested by Linus, this function is not checking UTF-8-ness of the
> string; it only is seeing if it is pure US-ASCII.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>   * Just for completeness, this on top.
>
>   compat/precompose_utf8.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index 3190d50..8cf5955 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -13,20 +13,20 @@ typedef char *iconv_ibp;
>   static const char *repo_encoding = "UTF-8";
>   static const char *path_encoding = "UTF-8-MAC";
>
> -static size_t has_utf8(const char *s, size_t maxlen, size_t *strlen_c)
> +static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c)
>   {
> -	const uint8_t *utf8p = (const uint8_t *)s;
> +	const uint8_t *ptr = (const uint8_t *)s;
>   	size_t strlen_chars = 0;
>   	size_t ret = 0;
>
> -	if (!utf8p || !*utf8p)
> +	if (!ptr || !*ptr)
>   		return 0;
>
> -	while (*utf8p && maxlen) {
> -		if (*utf8p & 0x80)
> +	while (*ptr && maxlen) {
> +		if (*ptr & 0x80)
>   			ret++;
>   		strlen_chars++;
> -		utf8p++;
> +		ptr++;
>   		maxlen--;
>   	}
>   	if (strlen_c)
> @@ -77,7 +77,7 @@ void precompose_argv(int argc, const char **argv)
>   	while (i < argc) {
>   		size_t namelen;
>   		oldarg = argv[i];
> -		if (has_utf8(oldarg, (size_t)-1, &namelen)) {
> +		if (has_non_ascii(oldarg, (size_t)-1, &namelen)) {
>   			newarg = reencode_string_iconv(oldarg, namelen, ic_precompose);
>   			if (newarg)
>   				argv[i] = newarg;
> @@ -130,7 +130,7 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
>   		prec_dir->dirent_nfc->d_ino  = res->d_ino;
>   		prec_dir->dirent_nfc->d_type = res->d_type;
>
> -		if ((precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, NULL)) {
> +		if ((precomposed_unicode == 1) && has_non_ascii(res->d_name, (size_t)-1, NULL)) {
>   			if (prec_dir->ic_precompose == (iconv_t)-1) {
>   				die("iconv_open(%s,%s) failed, but needed:\n"
>   						"    precomposed unicode is not supported.\n"
>

Thanks Junio, that partly obsoletes the patch I'm working on. And as I didn't manage to catch up, may I send the result of my review?

a) in readdir we die() when iconv_open() fails, and we could/should do that in void precompose_argv() as well?

b) Should die("txt") be converted into die(_("txt")) to be able to translate the message?

c) We can remove the save/restore of errno at one place, similar to this:

@@ -106,11 +106,8 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)
                 free(prec_dir->dirent_nfc);
                 free(prec_dir);
                 return NULL;
-       } else {
-               int ret_errno = errno;
+       } else
                 prec_dir->ic_precompose = (iconv_t)-1;
-               errno = ret_errno;
-       }