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?
> @@ -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), so
even though we already learned the branch name in buf here and
discard it, we will eventually discover the same information later.
That is somewhat contrived, and I am not so sure if that is a good
reorganization.
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?
The former needs a fix to either writing of reflog or reading by
interpret_nth_prior_checkout() so that we can tell these cases apart
more reliably. Then the latter can be solved by splicing
refs/heads/ in front of the branch name before recursively calling
get_sha1_1() in the original code (and similar fix can be
forward-ported to this patch).
Incidentally, I think if we prefix refs/heads/ in front and feed
that to dwim_ref/log, we would also avoid running @{-N} twice (which
I suspect might be more expensive than a simple recursion, as it
needs to go through the reflog file), which may be a nice side
effect of such a fix we would get for free.
> +
> + if (!len && reflog_len)
> /* allow "@{...}" to mean the current branch reflog */
> refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> - } else if (reflog_len)
> + else if (reflog_len)
> refs_found = dwim_log(str, len, sha1, &real_ref);
> else
> refs_found = dwim_ref(str, len, sha1, &real_ref);
--
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