Felipe Contreras wrote:
> On Wed, May 1, 2013 at 2:40 PM, Ramkumar Ramachandra <artagnon@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html