From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Thu, 02 May 2013 19:59:29 GMT
Subject: Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()
Message-ID: <CAMP44s3-TM1PW6qKAFRf=Sdojw3RNscM_VJZfBy3C7xHZst_Wg@mail.gmail.com>
In-Reply-To: <1367524078-13266-1-git-send-email-artagnon@gmail.com>

On Thu, May 2, 2013 at 2:47 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> A small suggestion.  Squash this in if you like; optionally submit it
> as a separate part.
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 6428001..109ab41 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -448,11 +448,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         if (len && str[len-1] == '}') {
>                 for (at = len-2; at >= 0; at--) {
>                         if (str[at] == '@' && str[at+1] == '{') {
> -                               if (at == 0 && str[2] == '-') {
> -                                       nth_prior = 1;
> -                                       continue;
> -                               }
> -                               if (!upstream_mark(str + at, len - at)) {
> +                               if (str[at+2] == '-') {
> +                                       if (at != 0)
> +                                               return -1;
> +                                       else
> +                                               nth_prior = 1;
> +                               } else if (!upstream_mark(str + at, len - at)) {

Generally I don't like this style, it's prone to multiple indentation levels.

                        if (str[at] == '@' && str[at+1] == '{') {
-                               if (at == 0 && str[2] == '-') {
+                               if (str[at+2] == '-') {
+                                       if (at != 0)
+                                               /* @{-N} not at start */
+                                               return -1;
                                        nth_prior = 1;
                                        continue;
                                }

Otherwise makes sense, but I would do it as a separate patch.

-- 
Felipe Contreras