Felipe Contreras <felipe.contre...@gmail.com> writes: > On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra > <artag...@gmail.com> wrote: >> The @-parsing loop unnecessarily checks for the sequence "@{" from >> len - 2 unnecessarily. We can safely check from len - 4: write out a >> comment justifying this. >> >> Signed-off-by: Ramkumar Ramachandra <artag...@gmail.com> >> --- >> sha1_name.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/sha1_name.c b/sha1_name.c >> index 3820f28..be1d12c 100644 >> --- a/sha1_name.c >> +++ b/sha1_name.c >> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, >> unsigned char *sha1) >> /* basic@{time or number or -number} format to query ref-log */ >> reflog_len = at = 0; >> if (len && str[len-1] == '}') { >> - for (at = len-2; at >= 0; at--) { >> + /* str = @} >> + * ^ >> + * len - 2; expression is senseless >> + * >> + * str = @{} >> + * ^ >> + * len - 3; expression is still senseless >> + * >> + * str = @{.} >> + * ^ >> + * len - 4 where . is any character; expression >> + * is worth investigating >> + * >> + * Therefore, if str ends with }, search three >> + * characters earlier for @{ >> + */ > > I think this comment is overkill. > >> + for (at = len - 4; at >= 0; at--) { > > The change seems OK to me, but there's no need to explain where you > are starting, and if there's a need: > > /* start from where reflogs can start: @{.} */ > > Does the trick nicely.
As the fact that nobody noticed nor bothered with the two-byte optimization opportunity shows that this is trickier than trivial, I agree with both of you that this change deserves an in-code comment. Start checking at len - 4, because there has to be at least one byte inside "@{.}" for it to be worth checking. would be sufficient. The 16-line comment is way overkill. Not that I think this change really matters, though. >> if (str[at] == '@' && str[at+1] == '{') { >> if (!upstream_mark(str + at, len - at)) { >> reflog_len = (len-1) - (at+2); >> -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html