| Message ID | 20201125001102.111025-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] refspec: make @ a synonym of HEAD | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > 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. > +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(-) 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 &&
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.
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. 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.
On Tue, Nov 24, 2020 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote: > > 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). > 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 <felipe.contreras@gmail.com> writes: > 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.
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 &&
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(-)