[PATCH] refspec: allow @ on the left-hand side of refspecs

[PATCH] refspec: allow @ on the left-hand side of refspecs

[brian m. carlson]

The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
and this is documented accordingly in gitrevisions(7).  The push
documentation describes the source portion of a refspec as "any
arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
left-hand side of a refspec, since we attempt to check for it being a
valid ref name and fail (since it is not).

Teach the refspec machinery about this alias and silently substitute
"HEAD" when we see "@".  This handles the fact that HEAD is a symref and
preserves its special behavior.  We need not handle other arbitrary
object ID expressions (such as "@^") when pushing because the revision
machinery already handles that for us.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
I probably type "git push upstream HEAD" from five to thirty times a
day, many of those where I typo "HEAD", so I decided to implement the
shorter form.  This design handles @ as HEAD in both fetch and push,
whereas alternate solutions would not.

check_refname_format explicitly rejects "@"; I tried at first to simply
ignore that with a flag, but we end up calling that from several other
places in the codebase and rejecting it and all of those places would
have needed updating.

I thought about putting the if/else logic in a function, but since it's
just four lines, I decided not to.  However, if people think it would be
tidier, I can do so.

Note that the test portion of the patch is best read with git diff -w;
the current version is very noisy.

 refspec.c             |   6 ++-
 t/t5516-fetch-push.sh | 104 +++++++++++++++++++++---------------------
 2 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/refspec.c b/refspec.c
index e8010dce0c..57c2f65104 100644
--- a/refspec.c
+++ b/refspec.c
@@ -62,8 +62,12 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
 		return 0;
 	}
 
+	if (llen == 1 && lhs[0] == '@')
+		item->src = xstrdup("HEAD");
+	else
+		item->src = xstrndup(lhs, llen);
+
 	item->pattern = is_glob;
-	item->src = xstrndup(lhs, llen);
 	flags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);
 
 	if (fetch) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index a5077d8b7c..cbccbd2f8d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -436,70 +436,72 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
-test_expect_success 'push with HEAD' '
+for ref in HEAD @
+do
+	test_expect_success "push with $ref" '
 
-	mk_test testrepo heads/master &&
-	git checkout master &&
-	git push testrepo HEAD &&
-	check_push_result testrepo $the_commit heads/master
+		mk_test testrepo heads/master &&
+		git checkout master &&
+		git push testrepo $ref &&
+		check_push_result testrepo $the_commit heads/master
 
-'
+	'
 
-test_expect_success 'push with HEAD nonexisting at remote' '
+	test_expect_success "push with $ref nonexisting at remote" '
 
-	mk_test testrepo heads/master &&
-	git checkout -b local master &&
-	git push testrepo HEAD &&
-	check_push_result testrepo $the_commit heads/local
-'
+		mk_test testrepo heads/master &&
+		git checkout -B local master &&
+		git push testrepo $ref &&
+		check_push_result testrepo $the_commit heads/local
+	'
 
-test_expect_success 'push with +HEAD' '
+	test_expect_success "push with +$ref" '
 
-	mk_test testrepo heads/master &&
-	git checkout master &&
-	git branch -D local &&
-	git checkout -b local &&
-	git push testrepo master local &&
-	check_push_result testrepo $the_commit heads/master &&
-	check_push_result testrepo $the_commit heads/local &&
+		mk_test testrepo heads/master &&
+		git checkout master &&
+		git branch -D local &&
+		git checkout -b local &&
+		git push testrepo master local &&
+		check_push_result testrepo $the_commit heads/master &&
+		check_push_result testrepo $the_commit heads/local &&
 
-	# Without force rewinding should fail
-	git reset --hard HEAD^ &&
-	test_must_fail git push testrepo HEAD &&
-	check_push_result testrepo $the_commit heads/local &&
+		# Without force rewinding should fail
+		git reset --hard HEAD^ &&
+		test_must_fail git push testrepo $ref &&
+		check_push_result testrepo $the_commit heads/local &&
 
-	# With force rewinding should succeed
-	git push testrepo +HEAD &&
-	check_push_result testrepo $the_first_commit heads/local
+		# With force rewinding should succeed
+		git push testrepo +$ref &&
+		check_push_result testrepo $the_first_commit heads/local
 
-'
+	'
 
-test_expect_success 'push HEAD with non-existent, incomplete dest' '
+	test_expect_success "push $ref with non-existent, incomplete dest" '
 
-	mk_test testrepo &&
-	git checkout master &&
-	git push testrepo HEAD:branch &&
-	check_push_result testrepo $the_commit heads/branch
+		mk_test testrepo &&
+		git checkout master &&
+		git push testrepo $ref:branch &&
+		check_push_result testrepo $the_commit heads/branch
 
-'
+	'
 
-test_expect_success 'push with config remote.*.push = HEAD' '
-
-	mk_test testrepo heads/local &&
-	git checkout master &&
-	git branch -f local $the_commit &&
-	(
-		cd testrepo &&
-		git checkout local &&
-		git reset --hard $the_first_commit
-	) &&
-	test_config remote.there.url testrepo &&
-	test_config remote.there.push HEAD &&
-	test_config branch.master.remote there &&
-	git push &&
-	check_push_result testrepo $the_commit heads/master &&
-	check_push_result testrepo $the_first_commit heads/local
-'
+	test_expect_success "push with config remote.*.push = $ref" '
+		mk_test testrepo heads/local &&
+		git checkout master &&
+		git branch -f local $the_commit &&
+		(
+			cd testrepo &&
+			git checkout local &&
+			git reset --hard $the_first_commit
+		) &&
+		test_config remote.there.url testrepo &&
+		test_config remote.there.push $ref &&
+		test_config branch.master.remote there &&
+		git push &&
+		check_push_result testrepo $the_commit heads/master &&
+		check_push_result testrepo $the_first_commit heads/local
+	'
+done
 
 test_expect_success 'push with remote.pushdefault' '
 	mk_test up_repo heads/master &&

Re: [PATCH] refspec: allow @ on the left-hand side of refspecs

[Brandon Williams]

On 07/29, brian m. carlson wrote:
> The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
> and this is documented accordingly in gitrevisions(7).  The push
> documentation describes the source portion of a refspec as "any
> arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
> left-hand side of a refspec, since we attempt to check for it being a
> valid ref name and fail (since it is not).
> 
> Teach the refspec machinery about this alias and silently substitute
> "HEAD" when we see "@".  This handles the fact that HEAD is a symref and
> preserves its special behavior.  We need not handle other arbitrary
> object ID expressions (such as "@^") when pushing because the revision
> machinery already handles that for us.

So this claims that using "@^" should work despite not accounting for it
explicitly or am I misreading?  Unless I'm mistaken, it looks like we
don't really support arbitrary rev syntax in refspecs since "HEAD^"
doesn't work either.

> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> I probably type "git push upstream HEAD" from five to thirty times a
> day, many of those where I typo "HEAD", so I decided to implement the
> shorter form.  This design handles @ as HEAD in both fetch and push,
> whereas alternate solutions would not.

I'm always a fan of finding shortcuts and reducing how much I type, so
thank you :)

> 
> check_refname_format explicitly rejects "@"; I tried at first to simply
> ignore that with a flag, but we end up calling that from several other
> places in the codebase and rejecting it and all of those places would
> have needed updating.
> 
> I thought about putting the if/else logic in a function, but since it's
> just four lines, I decided not to.  However, if people think it would be
> tidier, I can do so.
> 
> Note that the test portion of the patch is best read with git diff -w;
> the current version is very noisy.
> 
>  refspec.c             |   6 ++-
>  t/t5516-fetch-push.sh | 104 +++++++++++++++++++++---------------------
>  2 files changed, 58 insertions(+), 52 deletions(-)
> 
> diff --git a/refspec.c b/refspec.c
> index e8010dce0c..57c2f65104 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -62,8 +62,12 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
>  		return 0;
>  	}
>  
> +	if (llen == 1 && lhs[0] == '@')
> +		item->src = xstrdup("HEAD");
> +	else
> +		item->src = xstrndup(lhs, llen);
> +

This is probably the easiest place to put the aliasing logic so I don't
have any issue with including it here.

-- 
Brandon Williams

Re: [PATCH] refspec: allow @ on the left-hand side of refspecs

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

On Mon, Jul 30, 2018 at 10:50:51AM -0700, Brandon Williams wrote:
> On 07/29, brian m. carlson wrote:
> > The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
> > and this is documented accordingly in gitrevisions(7).  The push
> > documentation describes the source portion of a refspec as "any
> > arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
> > left-hand side of a refspec, since we attempt to check for it being a
> > valid ref name and fail (since it is not).
> > 
> > Teach the refspec machinery about this alias and silently substitute
> > "HEAD" when we see "@".  This handles the fact that HEAD is a symref and
> > preserves its special behavior.  We need not handle other arbitrary
> > object ID expressions (such as "@^") when pushing because the revision
> > machinery already handles that for us.
> 
> So this claims that using "@^" should work despite not accounting for it
> explicitly or am I misreading?  Unless I'm mistaken, it looks like we
> don't really support arbitrary rev syntax in refspecs since "HEAD^"
> doesn't work either.

Correct, it does indeed work, at least for me:

genre ok % git push castro HEAD^:refs/heads/temp
Total 0 (delta 0), reused 0 (delta 0)
To https://git.crustytoothpaste.net/git/bmc/git.git
 * [new branch]            HEAD^ -> temp

genre ok % git push castro @^:refs/heads/temp
Total 0 (delta 0), reused 0 (delta 0)
To https://git.crustytoothpaste.net/git/bmc/git.git
 * [new branch]            @^ -> temp

Note that in this case, I had to specify a full ref since it didn't
exist on the remote and the left side wasn't a ref name.

Now it doesn't work for fetches, only pushes.  Only the left side of a
push refspec can be an arbitrary expression.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH] refspec: allow @ on the left-hand side of refspecs

[Brandon Williams]

On 07/30, brian m. carlson wrote:
> On Mon, Jul 30, 2018 at 10:50:51AM -0700, Brandon Williams wrote:
> > On 07/29, brian m. carlson wrote:
> > > The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
> > > and this is documented accordingly in gitrevisions(7).  The push
> > > documentation describes the source portion of a refspec as "any
> > > arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
> > > left-hand side of a refspec, since we attempt to check for it being a
> > > valid ref name and fail (since it is not).
> > > 
> > > Teach the refspec machinery about this alias and silently substitute
> > > "HEAD" when we see "@".  This handles the fact that HEAD is a symref and
> > > preserves its special behavior.  We need not handle other arbitrary
> > > object ID expressions (such as "@^") when pushing because the revision
> > > machinery already handles that for us.
> > 
> > So this claims that using "@^" should work despite not accounting for it
> > explicitly or am I misreading?  Unless I'm mistaken, it looks like we
> > don't really support arbitrary rev syntax in refspecs since "HEAD^"
> > doesn't work either.
> 
> Correct, it does indeed work, at least for me:
> 
> genre ok % git push castro HEAD^:refs/heads/temp
> Total 0 (delta 0), reused 0 (delta 0)
> To https://git.crustytoothpaste.net/git/bmc/git.git
>  * [new branch]            HEAD^ -> temp
> 
> genre ok % git push castro @^:refs/heads/temp
> Total 0 (delta 0), reused 0 (delta 0)
> To https://git.crustytoothpaste.net/git/bmc/git.git
>  * [new branch]            @^ -> temp
> 
> Note that in this case, I had to specify a full ref since it didn't
> exist on the remote and the left side wasn't a ref name.

That's what I was missing, a full refspec! Thanks for the illustration.

> 
> Now it doesn't work for fetches, only pushes.  Only the left side of a
> push refspec can be an arbitrary expression.
> -- 
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204



-- 
Brandon Williams