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

[Junio C Hamano <gitster@pobox.com>]

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

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