{"thread":{"id":"54710","subject":"[PATCH v2] refspec: make @ a synonym of HEAD","startedAt":"2020-11-25T00:11:15Z","lastAt":"2020-11-26T00:01:29Z","messageCount":6,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"410733","messageId":"20201125001102.111025-1-felipe.contreras@gmail.com","threadId":"54710","inReplyTo":null,"subject":"[PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-25T00:11:02Z","receivedAt":"2020-11-25T00:11:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Since commit 9ba89f484e git learned how to push to a remote branch using\nthe source @, for example:\n\n git push origin @:$dst\n\nHowever, if the right-hand side is missing, the push fails:\n\n git push origin @\n\nIt is obvious what is the desired behavior, and allowing the push makes\nthings more consistent.\n\nAdditionally, @:master now has the same semantics as HEAD:master.\n\nSigned-off-by: Felipe Contreras \n---\n refspec.c | 5 ++++-\n t/t5511-refspec.sh | 2 ++\n t/t5516-fetch-push.sh | 18 ++++++++++++++++++\n 3 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/refspec.c b/refspec.c\nindex c49347c2d7..adbfb3283a 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -71,7 +71,10 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet\n \t}\n \n \titem->pattern = is_glob;\n-\titem->src = xstrndup(lhs, llen);\n+\tif (llen == 1 && *lhs == '@')\n+\t\titem->src = xstrdup(\"HEAD\");\n+\telse\n+\t\titem->src = xstrndup(lhs, llen);\n \tflags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);\n \n \tif (item->negative) {\ndiff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh\nindex f541f30bc2..f808649de4 100755\n--- a/t/t5511-refspec.sh\n+++ b/t/t5511-refspec.sh\n@@ -58,6 +58,8 @@ test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'\t\tinvalid\n \n test_refspec push 'HEAD'\n test_refspec fetch 'HEAD'\n+test_refspec push '@'\n+test_refspec fetch '@'\n test_refspec push 'refs/heads/ nitfol'\t\t\t\tinvalid\n test_refspec fetch 'refs/heads/ nitfol'\t\t\t\tinvalid\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex d11382f769..1ca6b9641d 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -483,6 +483,15 @@ test_expect_success 'push HEAD with non-existent, incomplete dest' '\n \n '\n \n+test_expect_success 'push @ with non-existent, incomplete dest' '\n+\n+\tmk_test testrepo &&\n+\tgit checkout master &&\n+\tgit push testrepo @:branch &&\n+\tcheck_push_result testrepo $the_commit heads/branch\n+\n+'\n+\n test_expect_success 'push with config remote.*.push = HEAD' '\n \n \tmk_test testrepo heads/local &&\n@@ -501,6 +510,15 @@ test_expect_success 'push with config remote.*.push = HEAD' '\n \tcheck_push_result testrepo $the_first_commit heads/local\n '\n \n+test_expect_success 'push with @' '\n+\n+\tmk_test testrepo heads/master &&\n+\tgit checkout master &&\n+\tgit push testrepo @ &&\n+\tcheck_push_result testrepo $the_commit heads/master\n+\n+'\n+\n test_expect_success 'push with remote.pushdefault' '\n \tmk_test up_repo heads/master &&\n \tmk_test down_repo heads/master &&\n-- \n2.29.2\n\n"},{"id":"410737","messageId":"xmqqblfmgtlg.fsf@gitster.c.googlers.com","threadId":"54710","inReplyTo":"20201125001102.111025-1-felipe.contreras@gmail.com","subject":"Re: [PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T00:36:59Z","receivedAt":"2020-11-25T00:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh\n> index f541f30bc2..f808649de4 100755\n> --- a/t/t5511-refspec.sh\n> +++ b/t/t5511-refspec.sh\n> @@ -58,6 +58,8 @@ test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'\t\tinvalid\n> \n> test_refspec push 'HEAD'\n> test_refspec fetch 'HEAD'\n> +test_refspec push '@'\n> +test_refspec fetch '@'\n\nOK.\n\n> +test_expect_success 'push @ with non-existent, incomplete dest' '\n> +\n> +\tmk_test testrepo &&\n> +\tgit checkout master &&\n> +\tgit push testrepo @:branch &&\n> +\tcheck_push_result testrepo $the_commit heads/branch\n> +\n> +'\n> +\n> test_expect_success 'push with config remote.*.push = HEAD' '\n> \n> \tmk_test testrepo heads/local &&\n> @@ -501,6 +510,15 @@ test_expect_success 'push with config remote.*.push = HEAD' '\n> \tcheck_push_result testrepo $the_first_commit heads/local\n> '\n> \n> +test_expect_success 'push with @' '\n> +\n> +\tmk_test testrepo heads/master &&\n> +\tgit checkout master &&\n> +\tgit push testrepo @ &&\n> +\tcheck_push_result testrepo $the_commit heads/master\n> +\n> +'\n\nThis is OK, but shouldn't this be placed before the tests with\nvarious configuration? Something along the lines of the attached,\nbut with the body of the loop properly reindented, would also give\nus a better test coverage at the same time.\n\n t/t5516-fetch-push.sh | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git c/t/t5516-fetch-push.sh w/t/t5516-fetch-push.sh\nindex d11382f769..0b015a8d60 100755\n--- c/t/t5516-fetch-push.sh\n+++ w/t/t5516-fetch-push.sh\n@@ -436,24 +436,27 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '\n \n '\n \n-test_expect_success 'push with HEAD' '\n+for HEAD in HEAD @\n+do\n+\n+test_expect_success \"push with $HEAD\" '\n \n \tmk_test testrepo heads/master &&\n \tgit checkout master &&\n-\tgit push testrepo HEAD &&\n+\tgit push testrepo $HEAD &&\n \tcheck_push_result testrepo $the_commit heads/master\n \n '\n \n-test_expect_success 'push with HEAD nonexisting at remote' '\n+test_expect_success \"push with $HEAD nonexisting at remote\" '\n \n \tmk_test testrepo heads/master &&\n \tgit checkout -b local master &&\n-\tgit push testrepo HEAD &&\n+\tgit push testrepo $HEAD &&\n \tcheck_push_result testrepo $the_commit heads/local\n '\n \n-test_expect_success 'push with +HEAD' '\n+test_expect_success \"push with +$HEAD\" '\n \n \tmk_test testrepo heads/master &&\n \tgit checkout master &&\n@@ -464,25 +467,27 @@ test_expect_success 'push with +HEAD' '\n \tcheck_push_result testrepo $the_commit heads/local &&\n \n \t# Without force rewinding should fail\n-\tgit reset --hard HEAD^ &&\n-\ttest_must_fail git push testrepo HEAD &&\n+\tgit reset --hard $HEAD^ &&\n+\ttest_must_fail git push testrepo $HEAD &&\n \tcheck_push_result testrepo $the_commit heads/local &&\n \n \t# With force rewinding should succeed\n-\tgit push testrepo +HEAD &&\n+\tgit push testrepo +$HEAD &&\n \tcheck_push_result testrepo $the_first_commit heads/local\n \n '\n \n-test_expect_success 'push HEAD with non-existent, incomplete dest' '\n+test_expect_success \"push $HEAD with non-existent, incomplete dest\" '\n \n \tmk_test testrepo &&\n \tgit checkout master &&\n-\tgit push testrepo HEAD:branch &&\n+\tgit push testrepo $HEAD:branch &&\n \tcheck_push_result testrepo $the_commit heads/branch\n \n '\n \n+done\n+\n test_expect_success 'push with config remote.*.push = HEAD' '\n \n \tmk_test testrepo heads/local &&\n"},{"id":"410739","messageId":"CAMP44s2ChcCjhjksS0s5BoYznqLAoXyFvSkP4GxSCh_ALusOtQ@mail.gmail.com","threadId":"54710","inReplyTo":"xmqqblfmgtlg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-25T00:43:29Z","receivedAt":"2020-11-25T00:43:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano wrote:\n> Felipe Contreras writes:\n\n> > +test_expect_success 'push with @' '\n> > +\n> > + mk_test testrepo heads/master &&\n> > + git checkout master &&\n> > + git push testrepo @ &&\n> > + check_push_result testrepo $the_commit heads/master\n> > +\n> > +'\n>\n> This is OK, but shouldn't this be placed before the tests with\n> various configuration? Something along the lines of the attached,\n> but with the body of the loop properly reindented, would also give\n> us a better test coverage at the same time.\n\nI don't see much value in those tests, since I don't see how if one\npasses another one would fail. But I guess it cannot hurt.\n\n-- \nFelipe Contreras\n"},{"id":"410745","messageId":"xmqq360ygq2g.fsf@gitster.c.googlers.com","threadId":"54710","inReplyTo":"CAMP44s2ChcCjhjksS0s5BoYznqLAoXyFvSkP4GxSCh_ALusOtQ@mail.gmail.com","subject":"Re: [PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T01:53:11Z","receivedAt":"2020-11-25T01:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>\n>> > +test_expect_success 'push with @' '\n>> > +\n>> > + mk_test testrepo heads/master &&\n>> > + git checkout master &&\n>> > + git push testrepo @ &&\n>> > + check_push_result testrepo $the_commit heads/master\n>> > +\n>> > +'\n>>\n>> This is OK, but shouldn't this be placed before the tests with\n>> various configuration? Something along the lines of the attached,\n>> but with the body of the loop properly reindented, would also give\n>> us a better test coverage at the same time.\n>\n> I don't see much value in those tests, since I don't see how if one\n> passes another one would fail. But I guess it cannot hurt.\n\nThat can only be said based on the knowledge of the implementation\ndetail of the code immediately after this patch gets applied. Any\nfuture change to the code for whatever reason (e.g. refactoring) can\nmake the current assumption invalid.\n\nAs the proposed log message says,\n\n Since commit 9ba89f484e git learned how to push to a remote branch using\n the source @, for example:\n\n git push origin @:$dst\n\n However, if the right-hand side is missing, the push fails:\n\n git push origin @\n\nwe care about both of these forms working, not just the singleton\nform, so it is not just \"not hurt\", but is actively a good thing, to\nprotect both forms from future breakage. After all, that is why we\nhave tests.\n\nThanks.\n\n\n"},{"id":"410834","messageId":"CAMP44s2cpPgg55XHTr=cRqRcrMOkU=qcrM=RpWYwVJJdd9V4JA@mail.gmail.com","threadId":"54710","inReplyTo":"xmqq360ygq2g.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-25T23:48:29Z","receivedAt":"2020-11-25T23:48:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Nov 24, 2020 at 7:53 PM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> > On Tue, Nov 24, 2020 at 6:37 PM Junio C Hamano wrote:\n> >> Felipe Contreras writes:\n> >\n> >> > +test_expect_success 'push with @' '\n> >> > +\n> >> > + mk_test testrepo heads/master &&\n> >> > + git checkout master &&\n> >> > + git push testrepo @ &&\n> >> > + check_push_result testrepo $the_commit heads/master\n> >> > +\n> >> > +'\n> >>\n> >> This is OK, but shouldn't this be placed before the tests with\n> >> various configuration? Something along the lines of the attached,\n> >> but with the body of the loop properly reindented, would also give\n> >> us a better test coverage at the same time.\n> >\n> > I don't see much value in those tests, since I don't see how if one\n> > passes another one would fail. But I guess it cannot hurt.\n>\n> That can only be said based on the knowledge of the implementation\n> detail of the code immediately after this patch gets applied. Any\n> future change to the code for whatever reason (e.g. refactoring) can\n> make the current assumption invalid.\n\nIt's not just the current implementation of the code; it's any\nimplementation of the code (in my opinion).\n\n> As the proposed log message says,\n>\n> Since commit 9ba89f484e git learned how to push to a remote branch using\n> the source @, for example:\n>\n> git push origin @:$dst\n>\n> However, if the right-hand side is missing, the push fails:\n>\n> git push origin @\n>\n> we care about both of these forms working, not just the singleton\n> form, so it is not just \"not hurt\", but is actively a good thing, to\n> protect both forms from future breakage. After all, that is why we\n> have tests.\n\nBoth forms were already tested.\n\nWhat you suggested adds three more tests: 3. +@ form, 4. @ not present\non the remote, 5. @ in remote.*.push. If the first two pass, I cannot\nsee any implementation that would fail the other three (okay, maybe\nthe +@ one).\n\nAnyway, I had to improve the current tests to make your suggestion\nwork, and what once was a single simple patch now became a series that\nis mostly shuffling around test code. At some point the aphorism\n\"don't let the perfect be the enemy of the good\" has to apply though.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"410836","messageId":"xmqqlfepasvg.fsf@gitster.c.googlers.com","threadId":"54710","inReplyTo":"CAMP44s2cpPgg55XHTr=cRqRcrMOkU=qcrM=RpWYwVJJdd9V4JA@mail.gmail.com","subject":"Re: [PATCH v2] refspec: make @ a synonym of HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-26T00:01:23Z","receivedAt":"2020-11-26T00:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Tue, Nov 24, 2020 at 7:53 PM Junio C Hamano wrote:\n>> ...\n>> we care about both of these forms working, not just the singleton\n>> form, so it is not just \"not hurt\", but is actively a good thing, to\n>> protect both forms from future breakage. After all, that is why we\n>> have tests.\n>\n> Both forms were already tested.\n\nYeah, I did realize it in my review of reviews I do after sending\nall the messages yesterday (not just on this topic), so there is no\nstrong reason to duplicate all the tests that involve HEAD.\n\nThanks for pushing back on this point.\n"}]}