Re: [PATCH] wt-status: use strncmp() for length-limited string comparison

From: Jeff King <peff@peff.net>
Date: Nov 25, 2015, 09:15 UTC
Message-ID: <20151125091503.GA1779@sigill.intra.peff.net>
In-Reply-To: <56551A11.9030809@web.de>

On Wed, Nov 25, 2015 at 03:16:49AM +0100, René Scharfe wrote:

Show 7 quoted lines

> > Hmm. I think this is mostly harmless, as a comparison like:
> > 
> >    memcmp("HEAD and more", "HEAD", strlen("HEAD"))
> [...]
> 
> Yes, except it should be strlen("HEAD and more") in your example code;
> with strlen("HEAD") it would compare just 4 bytes and return 0.

Whoops, yeah. Thank you for figuring out what I meant. :)

Show 21 quoted lines

> Using one more variable isn't that bad, as long as it gets a fitting
> name.  Or we could reuse "end" (I'm not worrying about scanning "HEAD"
> twice very much):
> 
> diff --git a/wt-status.c b/wt-status.c
> index 435fc28..96a731e 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
>  	target += strlen(" to ");
>  	strbuf_reset(&cb->buf);
>  	hashcpy(cb->nsha1, nsha1);
> -	for (end = target; *end && *end != '\n'; end++)
> -		;
> -	if (!memcmp(target, "HEAD", end - target)) {
> +	if (skip_prefix(target, "HEAD", &end) && (!*end || *end == '\n')) {
>  		/* HEAD is relative. Resolve it to the right reflog entry. */
>  		strbuf_addstr(&cb->buf,
>  			      find_unique_abbrev(nsha1, DEFAULT_ABBREV));
>  		return 1;
>  	}

Yeah, I think parsing left-to-right like this makes things much more obvious. And regarding scanning HEAD twice, I think we already do that (we find the trailing newline first in the current code). Though I agree that is absurd premature optimization.

> +	for (end = target; *end && *end != '\n'; end++)
> +		;

This loop (which I know you just moved, not wrote) is basically strchrnul, isn't it? That might be more readable.

-Peff