Re: [PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop

Felipe Contreras [off-list ref] writes:

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
[off-list ref] wrote:

quoted

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 <redacted>
---
 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.

quoted

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

quoted

                        if (str[at] == '@' && str[at+1] == '{') {
                                if (!upstream_mark(str + at, len - at)) {
                                        reflog_len = (len-1) - (at+2);
--