Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2017, 19:08 UTC
Message-ID: <xmqq8tp6x8b6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

Show 22 quoted lines

> Instead of replacing the whole string, we would expand it accordingly using:
>
> if (*name == '-') {
>   if (len == 1) {
>     name = "@{-1}";
>     len = 5;
>   } else {
>     struct strbuf changed_argument = STRBUF_INIT;
>
>     strbuf_addstr(&changed_argument, "@{-1}");
>     strbuf_addstr(&changed_argument, name + 1);
>
>     strbuf_setlen(&changed_argument, strlen(name) + 4);
>
>     name = strbuf_detach(&changed_argument, NULL);
>   }
> }
>
> Junio's comments on a previous version of the patch which used this same
> approach but inside setup_revisions [1]
>
> [1]: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>

What I said is that when we know we got "-", there is no reason to replace it with and textually parse "@{-1}".

Show 6 quoted lines

> +	if (*name == '-' && len == 1) {
> +		name = "@{-1}";
> +		len = 5;
> +	}
> +
>  	ret = get_sha1_basic(name, len, sha1, lookup_flags);

If we look at get_sha1_basic(), it obviously is not prepared to understand "-" as "@{-1}", and the primary obstacle is that the underlying interpret_nth_prior_checkout() does two things. It expects to take "@{-<num>}" as a string, and the first half parses the <num> into "long nth". The latter half then finds the nth prior checkout. We probably should factor out the latter half into a separate function find_nth_prior_checkout() that takes "long nth" as input, and call it from interpret_nth_prior_checkout(), as a preparatory step. Once it is done, get_sha1_basic() can notice that it was fed (len == 1 && str[0] == '-') and make a direct call to find_nth_prior_checkout() without going through the "pass '@{-1}' as text, have interpret_nth_prior_checkout() to parse it to recover 1", which is a roundabout way to do what you want to do.

Having said all that, I do not think the remainder of the code is prepared to take "-", not yet anyway [*1*], so turning "-" into "@{-1}" this patch does before it calls get_sha1_basic(), while it is not an ideal final state, is probably an acceptable milestone to stop at.

It is a separate matter if this patch is sufficient to produce correct results, though. I haven't studied the callers of this change to make sure yet, and may find bugs in this approach later.

[Footnote]

*1* For example, the existing callsite in get_sha1_basic() that
    calls interpret_nth_prior_checkout() does not replace "str" with
    what was returned when the HEAD is not detached.  The callpath
    then depends on dwim_ref() to also understand "@{-1}" it got
    from the caller.  If we really want to keep what came from the
    end user as-is so that error message can include it, we'd need
    to teach dwim_ref() about the new "-" convention.  The extent of
    necessary change will become a lot larger.  On the other hand,
    if we allow error messages and reports to use a real refname
    instead of parrotting exactly what the user gave us, I think we
    may be able to arrange to replace str/len in get_sha1_basic()
    when we call interpret/find_nth_prior_checkout() and get a ref,
    without having to teach the new "-" convention all over the
    place.