{"thread":{"id":"44821","subject":"[PATCH] branch_get_push: do not segfault when HEAD is detached","startedAt":"2017-01-06T05:07:33Z","lastAt":"2017-01-09T10:32:23Z","messageCount":9,"participants":["Kyle Meyer","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308842","messageId":"20170106045623.21118-1-kyle@kyleam.com","threadId":"44821","inReplyTo":null,"subject":"[PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-01-06T04:56:23Z","receivedAt":"2017-01-06T05:07:33Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Move the detached HEAD check from branch_get_push_1() to\nbranch_get_push() to avoid setting branch->push_tracking_ref when\nbranch is NULL.\n\nSigned-off-by: Kyle Meyer \n---\n remote.c | 6 +++---\n t/t1514-rev-parse-push.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..d5eaec737 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n {\n \tstruct remote *remote;\n \n-\tif (!branch)\n-\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n-\n \tremote = remote_get(pushremote_for_branch(branch, NULL));\n \tif (!remote)\n \t\treturn error_buf(err,\n@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n \n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n+\tif (!branch)\n+\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n+\n \tif (!branch->push_tracking_ref)\n \t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n \treturn branch->push_tracking_ref;\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex 7214f5b33..90c639ae1 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '\n \tresolve topic@{push} refs/remotes/origin/magic/topic\n '\n \n+test_expect_success 'resolving @{push} fails with a detached HEAD' '\n+\tgit checkout HEAD^{} &&\n+\ttest_when_finished \"git checkout -\" &&\n+\ttest_must_fail git rev-parse @{push}\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"308843","messageId":"20170106061832.o465olp3drnsont3@sigill.intra.peff.net","threadId":"44821","inReplyTo":"20170106045623.21118-1-kyle@kyleam.com","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-06T06:18:32Z","receivedAt":"2017-01-06T06:19:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2017 at 11:56:23PM -0500, Kyle Meyer wrote:\n\n> Move the detached HEAD check from branch_get_push_1() to\n> branch_get_push() to avoid setting branch->push_tracking_ref when\n> branch is NULL.\n\nYep, I think this is the right fix.\n\n> diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\n> index 7214f5b33..90c639ae1 100755\n> --- a/t/t1514-rev-parse-push.sh\n> +++ b/t/t1514-rev-parse-push.sh\n> @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '\n> \tresolve topic@{push} refs/remotes/origin/magic/topic\n> '\n> \n> +test_expect_success 'resolving @{push} fails with a detached HEAD' '\n> +\tgit checkout HEAD^{} &&\n> +\ttest_when_finished \"git checkout -\" &&\n> +\ttest_must_fail git rev-parse @{push}\n> +'\n\nLooks good. Thanks.\n\n-Peff\n\nPS Looks like this is your first patch. Welcome. :)\n"},{"id":"308853","messageId":"alpine.DEB.2.20.1701061438300.3469@virtualbox","threadId":"44821","inReplyTo":"20170106045623.21118-1-kyle@kyleam.com","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-06T13:41:42Z","receivedAt":"2017-01-06T13:42:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kyle,\n\nOn Thu, 5 Jan 2017, Kyle Meyer wrote:\n\n> Move the detached HEAD check from branch_get_push_1() to\n> branch_get_push() to avoid setting branch->push_tracking_ref when\n> branch is NULL.\n> \n> Signed-off-by: Kyle Meyer \n\nGood point.\n\n> diff --git a/remote.c b/remote.c\n> index ad6c5424e..d5eaec737 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n> {\n> \tstruct remote *remote;\n> \n> -\tif (!branch)\n> -\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> -\n> \tremote = remote_get(pushremote_for_branch(branch, NULL));\n> \tif (!remote)\n> \t\treturn error_buf(err,\n> @@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n> \n> const char *branch_get_push(struct branch *branch, struct strbuf *err)\n> {\n> +\tif (!branch)\n> +\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n> +\n> \tif (!branch->push_tracking_ref)\n> \t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n\nThis is the only caller of branch_get_push_1(), so all is good.\n\n> diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\n> index 7214f5b33..90c639ae1 100755\n> --- a/t/t1514-rev-parse-push.sh\n> +++ b/t/t1514-rev-parse-push.sh\n> @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '\n> \tresolve topic@{push} refs/remotes/origin/magic/topic\n> '\n> \n> +test_expect_success 'resolving @{push} fails with a detached HEAD' '\n> +\tgit checkout HEAD^{} &&\n\nI seem to recall that we prefer HEAD^0 over HEAD^{} and the existing\nscripts seem to agree with me:\n\n$ git grep -c HEAD^0 junio/pu -- t/\njunio/pu:t/t1450-fsck.sh:1\njunio/pu:t/t1507-rev-parse-upstream.sh:2\njunio/pu:t/t2020-checkout-detach.sh:5\njunio/pu:t/t3200-branch.sh:1\njunio/pu:t/t3203-branch-output.sh:3\njunio/pu:t/t3400-rebase.sh:1\njunio/pu:t/t3404-rebase-interactive.sh:1\njunio/pu:t/t5407-post-rewrite-hook.sh:2\njunio/pu:t/t5505-remote.sh:1\njunio/pu:t/t5510-fetch.sh:1\njunio/pu:t/t5533-push-cas.sh:3\njunio/pu:t/t6035-merge-dir-to-symlink.sh:3\njunio/pu:t/t7201-co.sh:2\njunio/pu:t/t7402-submodule-rebase.sh:1\njunio/pu:t/t9105-git-svn-commit-diff.sh:1\njunio/pu:t/t9107-git-svn-migrate.sh:1\n\n$ git grep -c HEAD^{} junio/pu -- t/\njunio/pu:t/t3200-branch.sh:3\n\nMaybe use HEAD^0 just for consistency?\n\nCiao,\nJohannes\n"},{"id":"308906","messageId":"87inprllpv.fsf@kyleam.com","threadId":"44821","inReplyTo":"alpine.DEB.2.20.1701061438300.3469@virtualbox","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-01-07T01:09:32Z","receivedAt":"2017-01-07T01:09:43Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Johannes Schindelin writes:\n\n[...]\n\n>> +test_expect_success 'resolving @{push} fails with a detached HEAD' '\n>> +\tgit checkout HEAD^{} &&\n>\n> I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing\n> scripts seem to agree with me:\n>\n> $ git grep -c HEAD^0 junio/pu -- t/\n> junio/pu:t/t1450-fsck.sh:1\n> junio/pu:t/t1507-rev-parse-upstream.sh:2\n> junio/pu:t/t2020-checkout-detach.sh:5\n> junio/pu:t/t3200-branch.sh:1\n> junio/pu:t/t3203-branch-output.sh:3\n> junio/pu:t/t3400-rebase.sh:1\n> junio/pu:t/t3404-rebase-interactive.sh:1\n> junio/pu:t/t5407-post-rewrite-hook.sh:2\n> junio/pu:t/t5505-remote.sh:1\n> junio/pu:t/t5510-fetch.sh:1\n> junio/pu:t/t5533-push-cas.sh:3\n> junio/pu:t/t6035-merge-dir-to-symlink.sh:3\n> junio/pu:t/t7201-co.sh:2\n> junio/pu:t/t7402-submodule-rebase.sh:1\n> junio/pu:t/t9105-git-svn-commit-diff.sh:1\n> junio/pu:t/t9107-git-svn-migrate.sh:1\n>\n> $ git grep -c HEAD^{} junio/pu -- t/\n> junio/pu:t/t3200-branch.sh:3\n>\n> Maybe use HEAD^0 just for consistency?\n\nYes, thanks for pointing that out.\n\n--\nKyle\n"},{"id":"308907","messageId":"20170107011138.uuy6ob234kyy3y4e@sigill.intra.peff.net","threadId":"44821","inReplyTo":"87inprllpv.fsf@kyleam.com","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-07T01:11:38Z","receivedAt":"2017-01-07T01:11:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:\n\n> > $ git grep -c HEAD^{} junio/pu -- t/\n> > junio/pu:t/t3200-branch.sh:3\n> >\n> > Maybe use HEAD^0 just for consistency?\n> \n> Yes, thanks for pointing that out.\n\nThe other option is just \"git checkout --detach\", which is also used in\nthe test suite. I tend to prefer it because it's a little more obvious\nto a reader.\n\n-Peff\n"},{"id":"308908","messageId":"20170107011215.29691-1-kyle@kyleam.com","threadId":"44821","inReplyTo":"20170106045623.21118-1-kyle@kyleam.com","subject":"[PATCH v2] branch_get_push: do not segfault when HEAD is detached","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-01-07T01:12:15Z","receivedAt":"2017-01-07T01:12:35Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Move the detached HEAD check from branch_get_push_1() to\nbranch_get_push() to avoid setting branch->push_tracking_ref when\nbranch is NULL.\n\nSigned-off-by: Kyle Meyer \n---\n remote.c | 6 +++---\n t/t1514-rev-parse-push.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..d5eaec737 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n {\n \tstruct remote *remote;\n \n-\tif (!branch)\n-\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n-\n \tremote = remote_get(pushremote_for_branch(branch, NULL));\n \tif (!remote)\n \t\treturn error_buf(err,\n@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)\n \n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n+\tif (!branch)\n+\t\treturn error_buf(err, _(\"HEAD does not point to a branch\"));\n+\n \tif (!branch->push_tracking_ref)\n \t\tbranch->push_tracking_ref = branch_get_push_1(branch, err);\n \treturn branch->push_tracking_ref;\ndiff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh\nindex 7214f5b33..623a32aa6 100755\n--- a/t/t1514-rev-parse-push.sh\n+++ b/t/t1514-rev-parse-push.sh\n@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '\n \tresolve topic@{push} refs/remotes/origin/magic/topic\n '\n \n+test_expect_success 'resolving @{push} fails with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\ttest_must_fail git rev-parse @{push}\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"308912","messageId":"87bmvjll8m.fsf@kyleam.com","threadId":"44821","inReplyTo":"20170107011138.uuy6ob234kyy3y4e@sigill.intra.peff.net","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2017-01-07T01:19:53Z","receivedAt":"2017-01-07T01:19:59Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:\n>\n>> > $ git grep -c HEAD^{} junio/pu -- t/\n>> > junio/pu:t/t3200-branch.sh:3\n>> >\n>> > Maybe use HEAD^0 just for consistency?\n>>\n>> Yes, thanks for pointing that out.\n>\n> The other option is just \"git checkout --detach\", which is also used in\n> the test suite. I tend to prefer it because it's a little more obvious\n> to a reader.\n\nTrue, that does seem clearer. Seems I should've waited a bit before\nsending out v2.\n\n--\nKyle\n"},{"id":"308914","messageId":"20170107013126.4ub726mf73y36of3@sigill.intra.peff.net","threadId":"44821","inReplyTo":"87bmvjll8m.fsf@kyleam.com","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-07T01:31:26Z","receivedAt":"2017-01-07T01:31:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote:\n\n> > The other option is just \"git checkout --detach\", which is also used in\n> > the test suite. I tend to prefer it because it's a little more obvious\n> > to a reader.\n> \n> True, that does seem clearer. Seems I should've waited a bit before\n> sending out v2.\n\nI think it's OK either way. Junio can also mark it up while applying,\ntoo, if he has a preference.\n\n-Peff\n"},{"id":"309016","messageId":"xmqqy3ykwmlb.fsf@gitster.mtv.corp.google.com","threadId":"44821","inReplyTo":"20170107013126.4ub726mf73y36of3@sigill.intra.peff.net","subject":"Re: [PATCH] branch_get_push: do not segfault when HEAD is detached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T10:32:00Z","receivedAt":"2017-01-09T10:32:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote:\n>\n>> > The other option is just \"git checkout --detach\", which is also used in\n>> > the test suite. I tend to prefer it because it's a little more obvious\n>> > to a reader.\n>> \n>> True, that does seem clearer. Seems I should've waited a bit before\n>> sending out v2.\n>\n> I think it's OK either way. Junio can also mark it up while applying,\n> too, if he has a preference.\n\nI prefer ^0 in scripts, simply because it is shorter, works\nwith any version of Git that knows how to detach, even the ones\nbefore --detach was introduced. I offhand do not recall which\nbetween ^0 and ^{} came earlier, but in practice I\nsaw nobody write the latter, so Dscho's suggestion is definitely an\nimprovement.\n\nI do also care about \"git checkout --detach\" to keep working\ncorrectly, but as long as we have dedicated tests to ensure that,\nwe'd be fine. In the case of the test being discussed, we assume\neither should work correctly and the point of the test is not about\nensuring that ^0 or --detach works, so either is OK.\n\nAnyway, thanks for a patch and a review.\n\n"}]}