On Wed, Nov 25, 2015 at 03:16:49AM +0100, René Scharfe wrote:
> > 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. :)
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html