{"thread":{"id":"38271","subject":"[PATCH] for-each-ref: Always check stat_tracking_info()'s return value.","startedAt":"2015-01-02T15:44:02Z","lastAt":"2015-01-02T18:31:26Z","messageCount":2,"participants":["Raphael Kubo da Costa","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254248","messageId":"1420213442-5310-1-git-send-email-raphael.kubo.da.costa@intel.com","threadId":"38271","inReplyTo":null,"subject":"[PATCH] for-each-ref: Always check stat_tracking_info()'s return value.","fromName":"Raphael Kubo da Costa","fromEmail":"raphael.kubo.da.costa@intel.com","sentAt":"2015-01-02T15:44:02Z","receivedAt":"2015-01-02T15:44:02Z","isPatch":true,"sender":{"key":"raphael.kubo.da.costa@intel.com","avatar":null},"body":"The code handling %(upstream:track) and %(upstream:trackshort) assumed\nit always had a valid branch that had been sanitized earlier in\npopulate_value(), and thus did not check the return value of the call to\nstat_tracking_info().\n\nWhile there is indeed some sanitization code that basically corresponds\nto stat_tracking_info() returning 0 (no base branch set), the function\ncan also return -1 when the base branch did exist but has since then\nbeen deleted.\n\nIn this case, num_ours and num_theirs had undefined values and a call to\n`git for-each-ref --format=\"%(upstream:track)\"` could print spurious\nvalues such as\n\n [behind -111794512]\n [ahead 38881640, behind 5103867]\n\neven for repositories with one single commit.\n\nWe now properly verify stat_tracking_info()'s return value and do not\nprint anything if it returns -1. This behavior also matches the\ndocumentation (\"has no effect if the ref does not have tracking\ninformation associated with it\").\n\nSigned-off-by: Raphael Kubo da Costa \n---\n builtin/for-each-ref.c | 11 +++++++++--\n t/t6300-for-each-ref.sh | 15 +++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 603a90e..52e6323 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -717,7 +717,10 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\t starts_with(name, \"upstream\")) {\n \t\t\t\tchar buf[40];\n \n-\t\t\t\tstat_tracking_info(branch, &num_ours, &num_theirs);\n+\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t\t\t &num_theirs) != 1)\n+\t\t\t\t\tcontinue;\n+\n \t\t\t\tif (!num_ours && !num_theirs)\n \t\t\t\t\tv->s = \"\";\n \t\t\t\telse if (!num_ours) {\n@@ -735,7 +738,11 @@ static void populate_value(struct refinfo *ref)\n \t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n \t\t\t\t starts_with(name, \"upstream\")) {\n \t\t\t\tassert(branch);\n-\t\t\t\tstat_tracking_info(branch, &num_ours, &num_theirs);\n+\n+\t\t\t\tif (stat_tracking_info(branch, &num_ours,\n+\t\t\t\t\t\t\t&num_theirs) != 1)\n+\t\t\t\t\tcontinue;\n+\n \t\t\t\tif (!num_ours && !num_theirs)\n \t\t\t\t\tv->s = \"=\";\n \t\t\t\telse if (!num_ours)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex bda354c..775c255 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '\n '\n \n cat >expected <actual &&\n+\tgit for-each-ref --format=\"%(upstream:trackshort)\" refs/heads/parent_gone >>actual &&\n+\tgit branch -D parent_gone &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected < wrote:\n> The code handling %(upstream:track) and %(upstream:trackshort) assumed\n> it always had a valid branch that had been sanitized earlier in\n> populate_value(), and thus did not check the return value of the call to\n> stat_tracking_info().\n>\n> While there is indeed some sanitization code that basically corresponds\n> to stat_tracking_info() returning 0 (no base branch set), the function\n> can also return -1 when the base branch did exist but has since then\n> been deleted.\n>\n> In this case, num_ours and num_theirs had undefined values and a call to\n> `git for-each-ref --format=\"%(upstream:track)\"` could print spurious\n> values such as\n>\n> [behind -111794512]\n> [ahead 38881640, behind 5103867]\n>\n> even for repositories with one single commit.\n>\n> We now properly verify stat_tracking_info()'s return value and do not\n> print anything if it returns -1. This behavior also matches the\n> documentation (\"has no effect if the ref does not have tracking\n> information associated with it\").\n>\n> Signed-off-by: Raphael Kubo da Costa \n> ---\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index bda354c..775c255 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -335,6 +335,21 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '\n> '\n>\n> cat >expected < +\n> +\n> +EOF\n> +\n> +test_expect_success 'Check that :track[short] works when upstream is gone' '\n> + git branch --track to_delete master &&\n> + git branch --track parent_gone to_delete &&\n> + git branch -D to_delete &&\n> + git for-each-ref --format=\"%(upstream:track)\" refs/heads/parent_gone >actual &&\n> + git for-each-ref --format=\"%(upstream:trackshort)\" refs/heads/parent_gone >>actual &&\n\nMinor comment: If any of the above code fails...\n\n> + git branch -D parent_gone &&\n\nthen this cleanup will not be performed. To ensure cleanup even if the\ntest fails, employ test_when_finished().\n\n> + test_cmp expected actual\n> +'\n> +\n> +cat >expected < $(git rev-parse --short HEAD)\n> EOF\n>\n> --\n> 2.1.4\n"}]}