Re: [PATCH] refspec: make @ a valid refspec

From: Jeff King <peff@peff.net>
Date: Nov 24, 2020, 07:42 UTC
Message-ID: <X7y5Wxu1eY4k6753@coredump.intra.peff.net>
In-Reply-To: <20201122164641.2091160-1-felipe.contreras@gmail.com>

On Sun, Nov 22, 2020 at 10:46:41AM -0600, Felipe Contreras wrote:

Show 8 quoted lines

> Since commit 9ba89f484e git learned how to push to a remote branch using
> the source @, for example:
> 
>   git push origin @:master
> 
> However, if the right-hand side is missing, the push fails:
> 
>   git push origin @

Yeah, I think this probably makes sense. I find the "@:master" refspec a bit weird, but it is a natural consequence of parsing "@" on the LHS as an arbitrary object to lookup (not a ref).

I did find the implementation a little curious:

Show 5 quoted lines

> -	item->src = xstrndup(lhs, llen);
> +	if (llen == 1 && *lhs == '@')
> +		item->src = xstrdup("HEAD");
> +	else
> +		item->src = xstrndup(lhs, llen);

We already know we are parsing the LHS, so why must we expand a bare @, whereas we do not in "@:master". The answer is that the "@" is not expanded until later, and parse_refspec() gets unhappy that "@" is also the implicit value for the RHS.

This also means that:

  git push origin @:foo

will now work when "foo" doesn't exist on the remote (if we find no match on the remote, we guess how it should be fully qualified; one of our heuristics involves seeing that HEAD points to a branch, and therefore the other side should be a branch, too).

> It is obvious what is the desired behavior, and allowing the push makes
> things more consistent.

This same code is used by fetch, as well. There "git fetch origin @:master" does not work at all now. This would make that work (to fetch the remote HEAD into master), along with "git fetch origin @" (into FETCH_HEAD only). Both seem sensible, and I cannot think of any other reasonable meaning for them.

Show 5 quoted lines

> ---
>  refspec.c             | 5 ++++-
>  t/t5511-refspec.sh    | 2 ++
>  t/t5516-fetch-push.sh | 9 +++++++++
>  3 files changed, 15 insertions(+), 1 deletion(-)

Would we want to cover the extra cases I mentioned above (@:foo, and the two fetches), as well?

I wondered if there was a good place to mention this in the refspec documentation, but it may just be an obvious fallout of the "@ is a shortcut for HEAD" definition in gitrevisions(7). The only change is that we're resolving the shortcut sooner so that more code can take advantage of it.

-Peff