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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2018, 19:59 UTC
Message-ID: <xmqqmuzjzmfi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180307173026.30058-10-lars.schneider@autodesk.com>

lars.schneider@autodesk.com writes:

> +static int check_roundtrip(const char* enc_name)

The asterisk sticks to the variable, not type.

Show 23 quoted lines

> +{
> +	/*
> +	 * 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.