# [PATCH] branch_get_push: do not segfault when HEAD is detached 9 messages from 2017-01-06 to 2017-01-09. Participants: Kyle Meyer, Jeff King, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/44821 ## Kyle Meyer, 2017-01-06 04:56 Subject: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <20170106045623.21118-1-kyle@kyleam.com> URL: https://gitlist.dev/e/20170106045623.21118-1-kyle%40kyleam.com ``` Move the detached HEAD check from branch_get_push_1() to branch_get_push() to avoid setting branch->push_tracking_ref when branch is NULL. Signed-off-by: Kyle Meyer --- remote.c | 6 +++--- t/t1514-rev-parse-push.sh | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/remote.c b/remote.c index ad6c5424e..d5eaec737 100644 --- a/remote.c +++ b/remote.c @@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) { struct remote *remote; - if (!branch) - return error_buf(err, _("HEAD does not point to a branch")); - remote = remote_get(pushremote_for_branch(branch, NULL)); if (!remote) return error_buf(err, @@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) const char *branch_get_push(struct branch *branch, struct strbuf *err) { + if (!branch) + return error_buf(err, _("HEAD does not point to a branch")); + if (!branch->push_tracking_ref) branch->push_tracking_ref = branch_get_push_1(branch, err); return branch->push_tracking_ref; diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh index 7214f5b33..90c639ae1 100755 --- a/t/t1514-rev-parse-push.sh +++ b/t/t1514-rev-parse-push.sh @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' ' resolve topic@{push} refs/remotes/origin/magic/topic ' +test_expect_success 'resolving @{push} fails with a detached HEAD' ' + git checkout HEAD^{} && + test_when_finished "git checkout -" && + test_must_fail git rev-parse @{push} +' + test_done -- 2.11.0 ``` ## Jeff King, 2017-01-06 06:18 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <20170106061832.o465olp3drnsont3@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170106061832.o465olp3drnsont3%40sigill.intra.peff.net In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com> ``` On Thu, Jan 05, 2017 at 11:56:23PM -0500, Kyle Meyer wrote: > Move the detached HEAD check from branch_get_push_1() to > branch_get_push() to avoid setting branch->push_tracking_ref when > branch is NULL. Yep, I think this is the right fix. > diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh > index 7214f5b33..90c639ae1 100755 > --- a/t/t1514-rev-parse-push.sh > +++ b/t/t1514-rev-parse-push.sh > @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' ' > resolve topic@{push} refs/remotes/origin/magic/topic > ' > > +test_expect_success 'resolving @{push} fails with a detached HEAD' ' > + git checkout HEAD^{} && > + test_when_finished "git checkout -" && > + test_must_fail git rev-parse @{push} > +' Looks good. Thanks. -Peff PS Looks like this is your first patch. Welcome. :) ``` ## Johannes Schindelin, 2017-01-06 13:41 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1701061438300.3469%40virtualbox In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com> ``` Hi Kyle, On Thu, 5 Jan 2017, Kyle Meyer wrote: > Move the detached HEAD check from branch_get_push_1() to > branch_get_push() to avoid setting branch->push_tracking_ref when > branch is NULL. > > Signed-off-by: Kyle Meyer Good point. > diff --git a/remote.c b/remote.c > index ad6c5424e..d5eaec737 100644 > --- a/remote.c > +++ b/remote.c > @@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) > { > struct remote *remote; > > - if (!branch) > - return error_buf(err, _("HEAD does not point to a branch")); > - > remote = remote_get(pushremote_for_branch(branch, NULL)); > if (!remote) > return error_buf(err, > @@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) > > const char *branch_get_push(struct branch *branch, struct strbuf *err) > { > + if (!branch) > + return error_buf(err, _("HEAD does not point to a branch")); > + > if (!branch->push_tracking_ref) > branch->push_tracking_ref = branch_get_push_1(branch, err); This is the only caller of branch_get_push_1(), so all is good. > diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh > index 7214f5b33..90c639ae1 100755 > --- a/t/t1514-rev-parse-push.sh > +++ b/t/t1514-rev-parse-push.sh > @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' ' > resolve topic@{push} refs/remotes/origin/magic/topic > ' > > +test_expect_success 'resolving @{push} fails with a detached HEAD' ' > + git checkout HEAD^{} && I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing scripts seem to agree with me: $ git grep -c HEAD^0 junio/pu -- t/ junio/pu:t/t1450-fsck.sh:1 junio/pu:t/t1507-rev-parse-upstream.sh:2 junio/pu:t/t2020-checkout-detach.sh:5 junio/pu:t/t3200-branch.sh:1 junio/pu:t/t3203-branch-output.sh:3 junio/pu:t/t3400-rebase.sh:1 junio/pu:t/t3404-rebase-interactive.sh:1 junio/pu:t/t5407-post-rewrite-hook.sh:2 junio/pu:t/t5505-remote.sh:1 junio/pu:t/t5510-fetch.sh:1 junio/pu:t/t5533-push-cas.sh:3 junio/pu:t/t6035-merge-dir-to-symlink.sh:3 junio/pu:t/t7201-co.sh:2 junio/pu:t/t7402-submodule-rebase.sh:1 junio/pu:t/t9105-git-svn-commit-diff.sh:1 junio/pu:t/t9107-git-svn-migrate.sh:1 $ git grep -c HEAD^{} junio/pu -- t/ junio/pu:t/t3200-branch.sh:3 Maybe use HEAD^0 just for consistency? Ciao, Johannes ``` ## Kyle Meyer, 2017-01-07 01:09 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <87inprllpv.fsf@kyleam.com> URL: https://gitlist.dev/e/87inprllpv.fsf%40kyleam.com In-Reply-To: ``` Johannes Schindelin writes: [...] >> +test_expect_success 'resolving @{push} fails with a detached HEAD' ' >> + git checkout HEAD^{} && > > I seem to recall that we prefer HEAD^0 over HEAD^{} and the existing > scripts seem to agree with me: > > $ git grep -c HEAD^0 junio/pu -- t/ > junio/pu:t/t1450-fsck.sh:1 > junio/pu:t/t1507-rev-parse-upstream.sh:2 > junio/pu:t/t2020-checkout-detach.sh:5 > junio/pu:t/t3200-branch.sh:1 > junio/pu:t/t3203-branch-output.sh:3 > junio/pu:t/t3400-rebase.sh:1 > junio/pu:t/t3404-rebase-interactive.sh:1 > junio/pu:t/t5407-post-rewrite-hook.sh:2 > junio/pu:t/t5505-remote.sh:1 > junio/pu:t/t5510-fetch.sh:1 > junio/pu:t/t5533-push-cas.sh:3 > junio/pu:t/t6035-merge-dir-to-symlink.sh:3 > junio/pu:t/t7201-co.sh:2 > junio/pu:t/t7402-submodule-rebase.sh:1 > junio/pu:t/t9105-git-svn-commit-diff.sh:1 > junio/pu:t/t9107-git-svn-migrate.sh:1 > > $ git grep -c HEAD^{} junio/pu -- t/ > junio/pu:t/t3200-branch.sh:3 > > Maybe use HEAD^0 just for consistency? Yes, thanks for pointing that out. -- Kyle ``` ## Jeff King, 2017-01-07 01:11 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <20170107011138.uuy6ob234kyy3y4e@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170107011138.uuy6ob234kyy3y4e%40sigill.intra.peff.net In-Reply-To: <87inprllpv.fsf@kyleam.com> ``` On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote: > > $ git grep -c HEAD^{} junio/pu -- t/ > > junio/pu:t/t3200-branch.sh:3 > > > > Maybe use HEAD^0 just for consistency? > > Yes, thanks for pointing that out. The other option is just "git checkout --detach", which is also used in the test suite. I tend to prefer it because it's a little more obvious to a reader. -Peff ``` ## Kyle Meyer, 2017-01-07 01:12 Subject: [PATCH v2] branch_get_push: do not segfault when HEAD is detached Message-ID: <20170107011215.29691-1-kyle@kyleam.com> URL: https://gitlist.dev/e/20170107011215.29691-1-kyle%40kyleam.com In-Reply-To: <20170106045623.21118-1-kyle@kyleam.com> ``` Move the detached HEAD check from branch_get_push_1() to branch_get_push() to avoid setting branch->push_tracking_ref when branch is NULL. Signed-off-by: Kyle Meyer --- remote.c | 6 +++--- t/t1514-rev-parse-push.sh | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/remote.c b/remote.c index ad6c5424e..d5eaec737 100644 --- a/remote.c +++ b/remote.c @@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) { struct remote *remote; - if (!branch) - return error_buf(err, _("HEAD does not point to a branch")); - remote = remote_get(pushremote_for_branch(branch, NULL)); if (!remote) return error_buf(err, @@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) const char *branch_get_push(struct branch *branch, struct strbuf *err) { + if (!branch) + return error_buf(err, _("HEAD does not point to a branch")); + if (!branch->push_tracking_ref) branch->push_tracking_ref = branch_get_push_1(branch, err); return branch->push_tracking_ref; diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh index 7214f5b33..623a32aa6 100755 --- a/t/t1514-rev-parse-push.sh +++ b/t/t1514-rev-parse-push.sh @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' ' resolve topic@{push} refs/remotes/origin/magic/topic ' +test_expect_success 'resolving @{push} fails with a detached HEAD' ' + git checkout HEAD^0 && + test_when_finished "git checkout -" && + test_must_fail git rev-parse @{push} +' + test_done -- 2.11.0 ``` ## Kyle Meyer, 2017-01-07 01:19 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <87bmvjll8m.fsf@kyleam.com> URL: https://gitlist.dev/e/87bmvjll8m.fsf%40kyleam.com In-Reply-To: <20170107011138.uuy6ob234kyy3y4e@sigill.intra.peff.net> ``` Jeff King writes: > On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote: > >> > $ git grep -c HEAD^{} junio/pu -- t/ >> > junio/pu:t/t3200-branch.sh:3 >> > >> > Maybe use HEAD^0 just for consistency? >> >> Yes, thanks for pointing that out. > > The other option is just "git checkout --detach", which is also used in > the test suite. I tend to prefer it because it's a little more obvious > to a reader. True, that does seem clearer. Seems I should've waited a bit before sending out v2. -- Kyle ``` ## Jeff King, 2017-01-07 01:31 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: <20170107013126.4ub726mf73y36of3@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170107013126.4ub726mf73y36of3%40sigill.intra.peff.net In-Reply-To: <87bmvjll8m.fsf@kyleam.com> ``` On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote: > > The other option is just "git checkout --detach", which is also used in > > the test suite. I tend to prefer it because it's a little more obvious > > to a reader. > > True, that does seem clearer. Seems I should've waited a bit before > sending out v2. I think it's OK either way. Junio can also mark it up while applying, too, if he has a preference. -Peff ``` ## Junio C Hamano, 2017-01-09 10:32 Subject: Re: [PATCH] branch_get_push: do not segfault when HEAD is detached Message-ID: URL: https://gitlist.dev/e/xmqqy3ykwmlb.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170107013126.4ub726mf73y36of3@sigill.intra.peff.net> ``` Jeff King writes: > On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote: > >> > The other option is just "git checkout --detach", which is also used in >> > the test suite. I tend to prefer it because it's a little more obvious >> > to a reader. >> >> True, that does seem clearer. Seems I should've waited a bit before >> sending out v2. > > I think it's OK either way. Junio can also mark it up while applying, > too, if he has a preference. I prefer ^0 in scripts, simply because it is shorter, works with any version of Git that knows how to detach, even the ones before --detach was introduced. I offhand do not recall which between ^0 and ^{} came earlier, but in practice I saw nobody write the latter, so Dscho's suggestion is definitely an improvement. I do also care about "git checkout --detach" to keep working correctly, but as long as we have dedicated tests to ensure that, we'd be fine. In the case of the test being discussed, we assume either should work correctly and the point of the test is not about ensuring that ^0 or --detach works, so either is OK. Anyway, thanks for a patch and a review. ```