Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

From: Junio C Hamano <gitster@pobox.com>
Date: May 8, 2013, 18:18 UTC
Message-ID: <7vbo8lfi8y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1367963711-8722-11-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 13 quoted lines

> 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/;

Show 8 quoted lines

> @@ -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?

Show 24 quoted lines

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

Show 9 quoted lines

> +
> +	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);