Re: [PATCH v2 1/1] gpg-interface: add minTrustLevel as a configuration option

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 24, 2019, 19:02 UTC
Message-ID: <xmqqblrx5yxu.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191222003123.10555-2-hji@dyntopia.com>

Hans Jerry Illikainen <hji@dyntopia.com> writes:

Show 12 quoted lines

> +				/* Do we have trust level? */
> +				if (sigcheck_gpg_status[i].flags & GPG_STATUS_TRUST_LEVEL) {
> +					/*
> +					 * GPG v1 and v2 differs in how the
> +					 * TRUST_ lines are written.  Some
> +					 * trust lines contain no additional
> +					 * space-separated information for v1.
> +					 */
> +					next = strchr(line, ' ');
> +					if (!next)
> +						next = strchrnul(line, '\n');
> +					trust = xmemdupz(line, next - line);

I wonder if telling strcspn() to stop at either SP or LF is more in line with the existing codebase [*1*] and/or more readable. It would make this part to:

		size_t trust_size = strcspn(line, " \n");
		trust = xmemdupz(line, trust_size);

without the need to use or update the 'next' variable, if I am not mistaken?

By the way, while we are looking at this patch, I notice that, throughout the function, the use of variable 'next' feels rather misleading, at least to me.

When I see a loop that iterates over a block of lines, and a variable 'line' is used to point at the beginning of the current line at the beginning of each iteration and the code in the iteration updates a pointer 'next', I'd expect 'next' (or perhaps 'next+1') to become the new value of 'line' when the current round of the iteration ends (i.e. the name 'next' would stand for 'here is where we expect the next line to start'). But the code we see in this function uses it for 'here is the end of the current _token_ on the line', primarily so that it can do something to the byte range (line,<end-of-token>) and it never gets used as 'now we are done with the line, let's move on to the next line'.

This matters because it makes it unclear to decide if the above two lines I gave as a counter-proposal is sufficient, or if it also needs to say "next = line + trust_size" to keep 'next' up-to-date. The name of the varirable implies it should be, but the way the code uses 'next' says it is a throw-away variable whose value does not matter once we have done with the end of the current token.

I wonder if the code becomes less misleading if we either (1) renamed 'next' to a name that hints more strongly that it is not the 'next' line but the end of the current token we are interested in, or (2) get rid of the pointer and instead counted size of the current token we are interested in, or perhaps both?

This is not the fault of this patch, but I just mention it before I forget.

Thanks.