Felipe Contreras wrote:
> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artag...@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.

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

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

Reply via email to