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

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

[Kyle Meyer]

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(-)

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

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

[Jeff King]

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. :)

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

[Johannes Schindelin]

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 <kyle@kyleam.com>

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

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

[Kyle Meyer]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Jeff King]

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

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

[Kyle Meyer]

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(-)

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

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

[Kyle Meyer]

Jeff King <peff@peff.net> 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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 <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.