Re: [PATCH] mailinfo: use starts_with() when checking scissors

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

Jeff King <peff@peff.net> writes:

> I think this is an improvement in safety and readability, and worth
> applying. I'd also be fine going further and using skip_prefix(), which
> would let us drop the magic-number "2", though:
>
>   - as Junio said in that earlier thread, we hope people aren't
>     encouraged to add more versions of scissors here anyway).
>
>   - I am not 100% sure that the increment of "c" in the conditional
>     is not also relying on that "2", as well (i.e., it is incrementing
>     one, and then the for-loop increments one). There's a non-trivial
>     risk of introducing off-by-one errors or even more subtlety here. :)
>
> So I'm OK leaving that. Thanks for resurrecting this cleanup.

You are correct to point out that the increment of "c" relies on 2.
We increment by one here, and let the loop control to increment
another, to skip over these two bytes.

The posted patch _might_ make it easier to read, but I do not think
it improves safety at all.  At the point of doing memcmp(), we know
that *c is not NUL and because we are dealing with NUL-terminated
string, we know we can check c[1] (otherwise we wouldn't even be
able to see if c is pointing at the end of string), so we check c[0]
and c[1] against four variants of two-byte scissors patterns.  The
current code uses memcmp() of 2 bytes, which is perfectly safe under
the condition, and starts_with() would also be equally safe.

If we were to teach new scissors sequence that is longer than two
bytes, then starts_with() would start becoming safer, but that will
not happen, so...