threads / patch / 54710

v2refspec: make @ a synonym of HEAD

Subject: [PATCH v2] refspec: make @ a synonym of HEAD

## tl;dr

6 messages between Nov 25, 2020 and Nov 26, 2020. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Felipe Contreras· Nov 25, 2020, 00:11 UTC · lore

Since commit 9ba89f484e git learned how to push to a remote branch using the source @, for example:

  git push origin @:$dst
However, if the right-hand side is missing, the push fails:
  git push origin @

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

Additionally, @:master now has the same semantics as HEAD:master.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 refspec.c             |  5 ++++-
 t/t5511-refspec.sh    |  2 ++
 t/t5516-fetch-push.sh | 18 ++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)
Show changes to 3 files +24 −1

refspec.c, t/t5511-refspec.sh, t/t5516-fetch-push.sh

diff --git a/refspec.c b/refspec.c
index c49347c2d7..adbfb3283a 100644
--- a/refspec.c
+++ b/refspec.c
@@ -71,7 +71,10 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
 	}
 
 	item->pattern = is_glob;
-	item->src = xstrndup(lhs, llen);
+	if (llen == 1 && *lhs == '@')
+		item->src = xstrdup("HEAD");
+	else
+		item->src = xstrndup(lhs, llen);
 	flags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);
 
 	if (item->negative) {
diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
index f541f30bc2..f808649de4 100755
--- a/t/t5511-refspec.sh
+++ b/t/t5511-refspec.sh
@@ -58,6 +58,8 @@ test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'		invalid
 
 test_refspec push 'HEAD'
 test_refspec fetch 'HEAD'
+test_refspec push '@'
+test_refspec fetch '@'
 test_refspec push 'refs/heads/ nitfol'				invalid
 test_refspec fetch 'refs/heads/ nitfol'				invalid
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index d11382f769..1ca6b9641d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -483,6 +483,15 @@ test_expect_success 'push HEAD with non-existent, incomplete dest' '
 
 '
 
+test_expect_success 'push @ with non-existent, incomplete dest' '
+
+	mk_test testrepo &&
+	git checkout master &&
+	git push testrepo @:branch &&
+	check_push_result testrepo $the_commit heads/branch
+
+'
+
 test_expect_success 'push with config remote.*.push = HEAD' '
 
 	mk_test testrepo heads/local &&
@@ -501,6 +510,15 @@ test_expect_success 'push with config remote.*.push = HEAD' '
 	check_push_result testrepo $the_first_commit heads/local
 '
 
+test_expect_success 'push with @' '
+
+	mk_test testrepo heads/master &&
+	git checkout master &&
+	git push testrepo @ &&
+	check_push_result testrepo $the_commit heads/master
+
+'
+
 test_expect_success 'push with remote.pushdefault' '
 	mk_test up_repo heads/master &&
 	mk_test down_repo heads/master &&

-- 
2.29.2
Junio C Hamano· Nov 25, 2020, 00:36 UTC · re: Felipe Contreras · lore

Re: [PATCH v2] refspec: make @ a synonym of HEAD

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 10 quoted lines
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index f541f30bc2..f808649de4 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -58,6 +58,8 @@ test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'		invalid
>  
>  test_refspec push 'HEAD'
>  test_refspec fetch 'HEAD'
> +test_refspec push '@'
> +test_refspec fetch '@'
OK.
Show 24 quoted lines
> +test_expect_success 'push @ with non-existent, incomplete dest' '
> +
> +	mk_test testrepo &&
> +	git checkout master &&
> +	git push testrepo @:branch &&
> +	check_push_result testrepo $the_commit heads/branch
> +
> +'
> +
>  test_expect_success 'push with config remote.*.push = HEAD' '
>  
>  	mk_test testrepo heads/local &&
> @@ -501,6 +510,15 @@ test_expect_success 'push with config remote.*.push = HEAD' '
>  	check_push_result testrepo $the_first_commit heads/local
>  '
>  
> +test_expect_success 'push with @' '
> +
> +	mk_test testrepo heads/master &&
> +	git checkout master &&
> +	git push testrepo @ &&
> +	check_push_result testrepo $the_commit heads/master
> +
> +'

This is OK, but shouldn't this be placed before the tests with various configuration? Something along the lines of the attached, but with the body of the loop properly reindented, would also give us a better test coverage at the same time.

 t/t5516-fetch-push.sh | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
Show changes to diff +15 −10
diff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh
index d11382f769..0b015a8d60 100755
--- c/t/t5516-fetch-push.sh
+++ w/t/t5516-fetch-push.sh
@@ -436,24 +436,27 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
-test_expect_success 'push with HEAD' '
+for HEAD in HEAD @
+do
+
+test_expect_success "push with $HEAD" '
 
 	mk_test testrepo heads/master &&
 	git checkout master &&
-	git push testrepo HEAD &&
+	git push testrepo $HEAD &&
 	check_push_result testrepo $the_commit heads/master
 
 '
 
-test_expect_success 'push with HEAD nonexisting at remote' '
+test_expect_success "push with $HEAD nonexisting at remote" '
 
 	mk_test testrepo heads/master &&
 	git checkout -b local master &&
-	git push testrepo HEAD &&
+	git push testrepo $HEAD &&
 	check_push_result testrepo $the_commit heads/local
 '
 
-test_expect_success 'push with +HEAD' '
+test_expect_success "push with +$HEAD" '
 
 	mk_test testrepo heads/master &&
 	git checkout master &&
@@ -464,25 +467,27 @@ test_expect_success 'push with +HEAD' '
 	check_push_result testrepo $the_commit heads/local &&
 
 	# Without force rewinding should fail
-	git reset --hard HEAD^ &&
-	test_must_fail git push testrepo HEAD &&
+	git reset --hard $HEAD^ &&
+	test_must_fail git push testrepo $HEAD &&
 	check_push_result testrepo $the_commit heads/local &&
 
 	# With force rewinding should succeed
-	git push testrepo +HEAD &&
+	git push testrepo +$HEAD &&
 	check_push_result testrepo $the_first_commit heads/local
 
 '
 
-test_expect_success 'push HEAD with non-existent, incomplete dest' '
+test_expect_success "push $HEAD with non-existent, incomplete dest" '
 
 	mk_test testrepo &&
 	git checkout master &&
-	git push testrepo HEAD:branch &&
+	git push testrepo $HEAD:branch &&
 	check_push_result testrepo $the_commit heads/branch
 
 '
 
+done
+
 test_expect_success 'push with config remote.*.push = HEAD' '
 
 	mk_test testrepo heads/local &&
Felipe Contreras· Nov 25, 2020, 00:43 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] refspec: make @ a synonym of HEAD

On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 13 quoted lines
> > +test_expect_success 'push with @' '
> > +
> > +     mk_test testrepo heads/master &&
> > +     git checkout master &&
> > +     git push testrepo @ &&
> > +     check_push_result testrepo $the_commit heads/master
> > +
> > +'
>
> This is OK, but shouldn't this be placed before the tests with
> various configuration?  Something along the lines of the attached,
> but with the body of the loop properly reindented, would also give
> us a better test coverage at the same time.

I don't see much value in those tests, since I don't see how if one passes another one would fail. But I guess it cannot hurt.

-- 
Felipe Contreras
Junio C Hamano· Nov 25, 2020, 01:53 UTC · re: Felipe Contreras · lore

Re: [PATCH v2] refspec: make @ a synonym of HEAD

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 19 quoted lines
> On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> > +test_expect_success 'push with @' '
>> > +
>> > +     mk_test testrepo heads/master &&
>> > +     git checkout master &&
>> > +     git push testrepo @ &&
>> > +     check_push_result testrepo $the_commit heads/master
>> > +
>> > +'
>>
>> This is OK, but shouldn't this be placed before the tests with
>> various configuration?  Something along the lines of the attached,
>> but with the body of the loop properly reindented, would also give
>> us a better test coverage at the same time.
>
> I don't see much value in those tests, since I don't see how if one
> passes another one would fail. But I guess it cannot hurt.

That can only be said based on the knowledge of the implementation detail of the code immediately after this patch gets applied. Any future change to the code for whatever reason (e.g. refactoring) can make the current assumption invalid.

As the proposed log message says,
    Since commit 9ba89f484e git learned how to push to a remote branch using
    the source @, for example:
      git push origin @:$dst
    However, if the right-hand side is missing, the push fails:
      git push origin @

we care about both of these forms working, not just the singleton form, so it is not just "not hurt", but is actively a good thing, to protect both forms from future breakage. After all, that is why we have tests.

Thanks.
Felipe Contreras· Nov 25, 2020, 23:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] refspec: make @ a synonym of HEAD

On Tue, Nov 24, 2020 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> > +test_expect_success 'push with @' '
> >> > +
> >> > +     mk_test testrepo heads/master &&
> >> > +     git checkout master &&
> >> > +     git push testrepo @ &&
> >> > +     check_push_result testrepo $the_commit heads/master
> >> > +
> >> > +'
> >>
> >> This is OK, but shouldn't this be placed before the tests with
> >> various configuration?  Something along the lines of the attached,
> >> but with the body of the loop properly reindented, would also give
> >> us a better test coverage at the same time.
> >
> > I don't see much value in those tests, since I don't see how if one
> > passes another one would fail. But I guess it cannot hurt.
>
> That can only be said based on the knowledge of the implementation
> detail of the code immediately after this patch gets applied.  Any
> future change to the code for whatever reason (e.g. refactoring) can
> make the current assumption invalid.

It's not just the current implementation of the code; it's any implementation of the code (in my opinion).

Show 15 quoted lines
> As the proposed log message says,
>
>     Since commit 9ba89f484e git learned how to push to a remote branch using
>     the source @, for example:
>
>       git push origin @:$dst
>
>     However, if the right-hand side is missing, the push fails:
>
>       git push origin @
>
> we care about both of these forms working, not just the singleton
> form, so it is not just "not hurt", but is actively a good thing, to
> protect both forms from future breakage.  After all, that is why we
> have tests.
Both forms were already tested.

What you suggested adds three more tests: 3. +@ form, 4. @ not present on the remote, 5. @ in remote.*.push. If the first two pass, I cannot see any implementation that would fail the other three (okay, maybe the +@ one).

Anyway, I had to improve the current tests to make your suggestion work, and what once was a single simple patch now became a series that is mostly shuffling around test code. At some point the aphorism "don't let the perfect be the enemy of the good" has to apply though.

Cheers.
-- 
Felipe Contreras
Junio C Hamano· Nov 26, 2020, 00:01 UTC · re: Felipe Contreras · lore

Re: [PATCH v2] refspec: make @ a synonym of HEAD

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 8 quoted lines
> On Tue, Nov 24, 2020 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> we care about both of these forms working, not just the singleton
>> form, so it is not just "not hurt", but is actively a good thing, to
>> protect both forms from future breakage.  After all, that is why we
>> have tests.
>
> Both forms were already tested.

Yeah, I did realize it in my review of reviews I do after sending all the messages yesterday (not just on this topic), so there is no strong reason to duplicate all the tests that involve HEAD.

Thanks for pushing back on this point.

← back to recent threads