threads / patch / 44821

patchbranch_get_push: do not segfault when HEAD is detached

Subject: [PATCH] branch_get_push: do not segfault when HEAD is detached

## tl;dr

9 messages between Jan 6, 2017 and Jan 9, 2017. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Kyle Meyer· Jan 6, 2017, 04:56 UTC · lore

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 <kyle@kyleam.com>
---
 remote.c                  | 6 +++---
 t/t1514-rev-parse-push.sh | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

remote.c, t/t1514-rev-parse-push.sh

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· Jan 6, 2017, 06:18 UTC · re: Kyle Meyer · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

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.
Show 13 quoted lines
> 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· Jan 6, 2017, 13:41 UTC · re: Kyle Meyer · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

Hi Kyle,
On Thu, 5 Jan 2017, Kyle Meyer wrote:
Show 5 quoted lines
> 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 <kyle@kyleam.com>
Good point.
Show 23 quoted lines
> 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.
Show 10 quoted lines
> 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· Jan 7, 2017, 01:09 UTC · re: Johannes Schindelin · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
[...]
Show 28 quoted lines
>> +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· Jan 7, 2017, 01:11 UTC · re: Kyle Meyer · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

On Fri, Jan 06, 2017 at 08:09:32PM -0500, Kyle Meyer wrote:
Show 6 quoted lines
> > $ 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· Jan 7, 2017, 01:19 UTC · re: Jeff King · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> 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· Jan 7, 2017, 01:31 UTC · re: Kyle Meyer · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote:
Show 6 quoted lines
> > 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· Jan 9, 2017, 10:32 UTC · re: Jeff King · lore

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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 <commit>^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 <commit>^0 and <commit>^{} 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 <commit>^0 or --detach works, so either is OK.

Anyway, thanks for a patch and a review.
Kyle Meyer· Jan 7, 2017, 01:12 UTC · re: Kyle Meyer · lore

[PATCH v2] branch_get_push: do not segfault when HEAD is detached

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 <kyle@kyleam.com>
---
 remote.c                  | 6 +++---
 t/t1514-rev-parse-push.sh | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

remote.c, t/t1514-rev-parse-push.sh

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

← back to recent threads