Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>>
>>> Through the years the functionality to handle @{-N} and @{u} has moved
>>> around the code, and as a result, code that once made sense, doesn't any
>>> more.
>>>
>>> There is no need to call this function recursively with the branch of
>>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>>
>>> However, there's one corner-case where @{-N} resolves to a detached
>>> HEAD, in which case we wouldn't get any ref back.
>>>
>>> So we parse the nth-prior manually, and deal with it depending on
>>> weather it's a SHA-1, or a ref.
>>> ...
>>
>> s/weather/whether/;
>>
>>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>> if (len && str[len-1] == '}') {
>>> for (at = len-4; at >= 0; at--) {
>>> if (str[at] == '@' && str[at+1] == '{') {
>>> + if (at == 0 && str[2] == '-') {
>>> + nth_prior = 1;
>>> + continue;
>>> + }
>>
>> Does this have to be inside the loop?
>
> Yes, the whole purpose is to avoid reflog_len to be set.
What I meant was the "<nothing>@{-" check, which happens only at==0.
if (!memcmp(str, "@{-", 3) && len > 3)
nth_prior = 1;
else
for (at = len - 4; at; at--) {
... look for and break at the first "@{" ...
}
or something.
>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
>> again (the log message hints this but it wasn't all that clear),
>
> I thought it was clear we would let dwim_{ref,log} do the job:
Yes, the reason I did not immediately think of that is because I
knew @{-N} was expensive (need to read reflog backwards) and didn't
think anybody would redo the code to deliberately do that twice ;-)
>> Also, a few points this patch highlights in the code before the
>> change:
>>
>> - If we were on a branch with 40-hex name at nth prior checkout,
>> would we mistake it as being detached at the commit?
>>
>> - If we were on a branch 'foo' at nth prior checkout, would our
>> previous get_sha1_1() have made us mistake it as referring to a
>> tag 'foo' with the same name if it exists?
>
> I don't know, but I suspect there's no change after this patch.
Yes, didn't I say "the code before the change" above?
These two correctness issues look more important issues to me, with
or without the restructure patch (in other words, they are
independent).
--
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