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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 1, 2013, 17:57 UTC
Message-ID: <CAMP44s1j6GdQBjgCdBrG3xvXr_XifCTRZWJAQUrzts-MzFg--g@mail.gmail.com>
In-Reply-To: <1367425235-14998-3-git-send-email-artagnon@gmail.com>

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 34 quoted lines

> 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 <artagnon@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.

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

-- 
Felipe Contreras