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

[Junio C Hamano <gitster@pobox.com>]

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

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

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