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.
>> @@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>> if (len && ambiguous_path(str, len))
>> return -1;
>>
>> - if (!len && reflog_len) {
>> + if (nth_prior) {
>> struct strbuf buf = STRBUF_INIT;
>> - int ret;
>> - /* try the @{-N} syntax for n-th checkout */
>> - ret = interpret_branch_name(str, &buf);
>> - if (ret > 0)
>> - /* substitute this branch name and restart */
>> - return get_sha1_1(buf.buf, buf.len, sha1, 0);
>> - else if (ret == 0)
>> - return -1;
>> + int detached;
>> +
>> + if (interpret_nth_prior_checkout(str, &buf) > 0) {
>> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
>> + strbuf_release(&buf);
>> + if (detached)
>> + return 0;
>> + }
>> + }
>
> Earlier, if @{-N} resolved to a detached head, we just fed it to
> get_sha1_1(). If it resolved to a concrete refname, we also fed it
> to get_sha1_1(). We ended up calling ourselves again and did the
> right thing either way.
>
> The new code bypasses the recursive call when we get a detached head
> back, because we know that calling get_sha1_1() with the 40-hex will
> eventually take us back to this codepath, and immediately return
> when it sees get_sha1_hex() succeeds.
>
> What happens when str @{-N} leaves a concrete refname in buf.buf?
> The branch name is lost with strbuf_release(), and then where do we
> go from here? Continuing down from here would run dwim_ref/log on
> str which is still @{-N}, no?
>
> 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:
---
There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.
---
> That is somewhat contrived, and I am not so sure if that is a good
> reorganization.
But much less contrived than before, because the code that deals with
@{-N} is in one place, instead of sprinkled all over as many
corner-cases, and there's no recursion.
> 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.
--
Felipe Contreras
--
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