[PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[Raphael Kubo da Costa]

The code handling %(upstream:track) and %(upstream:trackshort) assumed
it always had a valid branch that had been sanitized earlier in
populate_value(), and thus did not check the return value of the call to
stat_tracking_info().

While there is indeed some sanitization code that basically corresponds
to stat_tracking_info() returning 0 (no base branch set), the function
can also return -1 when the base branch did exist but has since then
been deleted.

In this case, num_ours and num_theirs had undefined values and a call to
`git for-each-ref --format="%(upstream:track)"` could print spurious
values such as

  [behind -111794512]
  [ahead 38881640, behind 5103867]

even for repositories with one single commit.

We now properly verify stat_tracking_info()'s return value and do not
print anything if it returns -1. This behavior also matches the
documentation ("has no effect if the ref does not have tracking
information associated with it").

Signed-off-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
---
 builtin/for-each-ref.c  | 11 +++++++++--
 t/t6300-for-each-ref.sh | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 603a90e..52e6323 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -717,7 +717,10 @@ static void populate_value(struct refinfo *ref)
 				 starts_with(name, "upstream")) {
 				char buf[40];
 
-				stat_tracking_info(branch, &num_ours, &num_theirs);
+				if (stat_tracking_info(branch, &num_ours,
+						       &num_theirs) != 1)
+					continue;
+
 				if (!num_ours && !num_theirs)
 					v->s = "";
 				else if (!num_ours) {
@@ -735,7 +738,11 @@ static void populate_value(struct refinfo *ref)
 			} else if (!strcmp(formatp, "trackshort") &&
 				   starts_with(name, "upstream")) {
 				assert(branch);
-				stat_tracking_info(branch, &num_ours, &num_theirs);
+
+				if (stat_tracking_info(branch, &num_ours,
+							&num_theirs) != 1)
+					continue;
+
 				if (!num_ours && !num_theirs)
 					v->s = "=";
 				else if (!num_ours)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index bda354c..775c255 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
 '
 
 cat >expected <<EOF
+
+
+EOF
+
+test_expect_success 'Check that :track[short] works when upstream is gone' '
+	git branch --track to_delete master &&
+	git branch --track parent_gone to_delete &&
+	git branch -D to_delete &&
+	git for-each-ref --format="%(upstream:track)" refs/heads/parent_gone >actual &&
+	git for-each-ref --format="%(upstream:trackshort)" refs/heads/parent_gone >>actual &&
+	git branch -D parent_gone &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
 $(git rev-parse --short HEAD)
 EOF
 
-- 
2.1.4

Re: [PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[Eric Sunshine]

On Fri, Jan 2, 2015 at 10:44 AM, Raphael Kubo da Costa
<raphael.kubo.da.costa@intel.com> wrote:
> The code handling %(upstream:track) and %(upstream:trackshort) assumed
> it always had a valid branch that had been sanitized earlier in
> populate_value(), and thus did not check the return value of the call to
> stat_tracking_info().
>
> While there is indeed some sanitization code that basically corresponds
> to stat_tracking_info() returning 0 (no base branch set), the function
> can also return -1 when the base branch did exist but has since then
> been deleted.
>
> In this case, num_ours and num_theirs had undefined values and a call to
> `git for-each-ref --format="%(upstream:track)"` could print spurious
> values such as
>
>   [behind -111794512]
>   [ahead 38881640, behind 5103867]
>
> even for repositories with one single commit.
>
> We now properly verify stat_tracking_info()'s return value and do not
> print anything if it returns -1. This behavior also matches the
> documentation ("has no effect if the ref does not have tracking
> information associated with it").
>
> Signed-off-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
> ---
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index bda354c..775c255 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
>  '
>
>  cat >expected <<EOF
> +
> +
> +EOF
> +
> +test_expect_success 'Check that :track[short] works when upstream is gone' '
> +       git branch --track to_delete master &&
> +       git branch --track parent_gone to_delete &&
> +       git branch -D to_delete &&
> +       git for-each-ref --format="%(upstream:track)" refs/heads/parent_gone >actual &&
> +       git for-each-ref --format="%(upstream:trackshort)" refs/heads/parent_gone >>actual &&

Minor comment: If any of the above code fails...

> +       git branch -D parent_gone &&

then this cleanup will not be performed. To ensure cleanup even if the
test fails, employ test_when_finished().

> +       test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
>  $(git rev-parse --short HEAD)
>  EOF
>
> --
> 2.1.4

[PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[Raphael Kubo da Costa]

The code handling %(upstream:track) and %(upstream:trackshort) assumed
it always had a valid branch that had been sanitized earlier in
populate_value(), and thus did not check the return value of the call to
stat_tracking_info().

While there is indeed some sanitization code that basically corresponds
to stat_tracking_info() returning 0 (no base branch set), the function
can also return -1 when the base branch did exist but has since then
been deleted.

In this case, num_ours and num_theirs had undefined values and a call to
`git for-each-ref --format="%(upstream:track)"` could print spurious
values such as

  [behind -111794512]
  [ahead 38881640, behind 5103867]

even for repositories with one single commit.

We now properly verify stat_tracking_info()'s return value and do not
print anything if it returns -1. This behavior also matches the
documentation ("has no effect if the ref does not have tracking
information associated with it").

Signed-off-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
---
v2: Use `test_when_finished' to clean up the "parent_gone" branch.

 builtin/for-each-ref.c  | 11 +++++++++--
 t/t6300-for-each-ref.sh | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 603a90e..52e6323 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -717,7 +717,10 @@ static void populate_value(struct refinfo *ref)
 				 starts_with(name, "upstream")) {
 				char buf[40];
 
-				stat_tracking_info(branch, &num_ours, &num_theirs);
+				if (stat_tracking_info(branch, &num_ours,
+						       &num_theirs) != 1)
+					continue;
+
 				if (!num_ours && !num_theirs)
 					v->s = "";
 				else if (!num_ours) {
@@ -735,7 +738,11 @@ static void populate_value(struct refinfo *ref)
 			} else if (!strcmp(formatp, "trackshort") &&
 				   starts_with(name, "upstream")) {
 				assert(branch);
-				stat_tracking_info(branch, &num_ours, &num_theirs);
+
+				if (stat_tracking_info(branch, &num_ours,
+							&num_theirs) != 1)
+					continue;
+
 				if (!num_ours && !num_theirs)
 					v->s = "=";
 				else if (!num_ours)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index bda354c..df9c3bd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
 '
 
 cat >expected <<EOF
+
+
+EOF
+
+test_expect_success 'Check that :track[short] works when upstream is gone' '
+	git branch --track to_delete master &&
+	git branch --track parent_gone to_delete &&
+	git branch -D to_delete &&
+	git for-each-ref --format="%(upstream:track)" refs/heads/parent_gone >actual &&
+	git for-each-ref --format="%(upstream:trackshort)" refs/heads/parent_gone >>actual &&
+	test_when_finished "git branch -D parent_gone" &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
 $(git rev-parse --short HEAD)
 EOF
 
-- 
2.1.4

Re: [PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[Raphael Kubo da Costa]

Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> writes:

> Subject: Re: [PATCH] for-each-ref: Always check stat_tracking_info()'s
> return value.

Sorry, this was supposed to be "PATCH v2". The subject's wrong, but the
contents are fine.

> ---
> v2: Use `test_when_finished' to clean up the "parent_gone" branch.
>
>  builtin/for-each-ref.c  | 11 +++++++++--
>  t/t6300-for-each-ref.sh | 15 +++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)

Re: [PATCH] for-each-ref: Always check stat_tracking_info()'s return value.

[Eric Sunshine]

On Fri, Jan 2, 2015 at 3:28 PM, Raphael Kubo da Costa
<raphael.kubo.da.costa@intel.com> wrote:
> The code handling %(upstream:track) and %(upstream:trackshort) assumed
> it always had a valid branch that had been sanitized earlier in
> populate_value(), and thus did not check the return value of the call to
> stat_tracking_info().
>
> While there is indeed some sanitization code that basically corresponds
> to stat_tracking_info() returning 0 (no base branch set), the function
> can also return -1 when the base branch did exist but has since then
> been deleted.
>
> In this case, num_ours and num_theirs had undefined values and a call to
> `git for-each-ref --format="%(upstream:track)"` could print spurious
> values such as
>
>   [behind -111794512]
>   [ahead 38881640, behind 5103867]
>
> even for repositories with one single commit.
>
> We now properly verify stat_tracking_info()'s return value and do not
> print anything if it returns -1. This behavior also matches the
> documentation ("has no effect if the ref does not have tracking
> information associated with it").
>
> Signed-off-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
> ---
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index bda354c..df9c3bd 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
>  '
>
>  cat >expected <<EOF
> +
> +
> +EOF
> +
> +test_expect_success 'Check that :track[short] works when upstream is gone' '
> +       git branch --track to_delete master &&
> +       git branch --track parent_gone to_delete &&
> +       git branch -D to_delete &&
> +       git for-each-ref --format="%(upstream:track)" refs/heads/parent_gone >actual &&
> +       git for-each-ref --format="%(upstream:trackshort)" refs/heads/parent_gone >>actual &&
> +       test_when_finished "git branch -D parent_gone" &&

This still has the same problem. If the commands prior to
test_when_finish() fail, the test_when_finished() will never be
invoked. To fix, move test_when_finished() to just after the command
which creates the "parent_gone" branch.

> +       test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
>  $(git rev-parse --short HEAD)
>  EOF
>
> --
> 2.1.4