Web lists-archives.com

Re: [PATCH v10 9/9] convert: add round trip check based on 'core.checkRoundtripEncoding'




lars.schneider@xxxxxxxxxxxx writes:

> +static int check_roundtrip(const char* enc_name)

The asterisk sticks to the variable, not type.

> +{
> +	/*
> +	 * check_roundtrip_encoding contains a string of space and/or
> +	 * comma separated encodings (eg. "UTF-16, ASCII, CP1125").
> +	 * Search for the given encoding in that string.
> +	 */
> +	const char *found = strcasestr(check_roundtrip_encoding, enc_name);
> +	const char *next;
> +	int len;
> +	if (!found)
> +		return 0;
> +	next = found + strlen(enc_name);
> +	len = strlen(check_roundtrip_encoding);
> +	return (found && (
> +			/*
> +			 * check that the found encoding is at the
> +			 * beginning of check_roundtrip_encoding or
> +			 * that it is prefixed with a space or comma
> +			 */
> +			found == check_roundtrip_encoding || (
> +				found > check_roundtrip_encoding &&
> +				(*(found-1) == ' ' || *(found-1) == ',')
> +			)

The second line is unneeded, as we know a non-NULL found either
points at check_roundtrip_encoding or somewhere to the right, and
the first test already checked the "points exactly at" case.

This is defined to be a comma separated list, so it is unnecessary
to accept <cre,en> == <"FOO, SHIFT-JIS, BAR", "SHIFT-JIS">; if you
allow SP, perhaps "isspace(found[-1]) || found[-1] == ','" to also
allow HT may also be appropriate.  I think "comma or whitespace
separated list" is fine; in any case, the comment near the beginning
of this function does not match new text in Documentation/config.txt
added by this patch.