Siddharth Kannan <kannan.siddharth12@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
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.