Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 1, 2013, 22:26 UTC
Message-ID: <CALkWK0kJj2J6gg-M+CcLdaM1cXhnLGUWCYwUqc+XGdDDyhwo2Q@mail.gmail.com>
In-Reply-To: <CAMP44s2sRsJR9xxty9F4c7-G3HQrK3N=-o7KBSpG5TYpdhu-9w@mail.gmail.com>

Felipe Contreras wrote:

Show 9 quoted lines

> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> You don't think I already tried that?  There is no way to sensibly
>> reorganize the old logic sensibly, in a way that doesn't break
>> anything.
>
> See, I tried to split your patch into logical changes, so I started with this:
>
> --- a/sha1_name.c
> +++ b/sha1_name.c

Thanks; I was finding this hard to do.  I'll try to continue from here.

Show 9 quoted lines

> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -473,7 +473,7 @@ static int get_sha1_basic(const char *str, int
> len, unsigned char *sha1)
>                                 return -1;
>                         }
>                         /* allow "@{...}" to mean the current branch reflog */
> -                       refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);

Yeah, I noticed this just a few minutes ago. We really should have tests testing @{N} against HEAD@{N}.

> Of course, this would be easy to see if you had bothered to split your
> patch into logical changes, but you didn't, so the change is lost in a
> mess. This is why it's not recommended to do that.

Right.  I'll try to redo this as multiple parts.

Show 5 quoted lines

>> There's no need to associate one comment with one line of code.
>> People can see clearly see the failure case following it.
>
> Is that the way you defend your comments? People can see that the
> comment is wrong?

In that case, all the comments are wrong. Even the ones about @{N} and @{-N}, because we never really check @{\d+} or @{-\d+}. Would you like to make the comments more painful to read and write?

> 2) @{-1}@{-1} now doesn't return an error
>
> 3) @{-1}{0} returns an invalid object

Thanks for the tests!  I'll look into the problem.