# [PATCH] for-each-ref: Always check stat_tracking_info()'s return value. 3 messages from 2015-01-02 to 2015-01-02. Participants: Raphael Kubo da Costa, Eric Sunshine. Thread: https://gitlist.dev/t/38273 ## Raphael Kubo da Costa, 2015-01-02 20:28 Subject: [PATCH] for-each-ref: Always check stat_tracking_info()'s return value. Message-ID: <1420230521-8365-1-git-send-email-raphael.kubo.da.costa@intel.com> URL: https://gitlist.dev/e/1420230521-8365-1-git-send-email-raphael.kubo.da.costa%40intel.com ``` 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 --- 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 <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 < URL: https://gitlist.dev/e/87d26x3oyn.fsf%40rkubodac-mobl1.ger.corp.intel.com In-Reply-To: <1420230521-8365-1-git-send-email-raphael.kubo.da.costa@intel.com> ``` Raphael Kubo da Costa 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(-) ``` ## Eric Sunshine, 2015-01-02 20:51 Subject: Re: [PATCH] for-each-ref: Always check stat_tracking_info()'s return value. Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR4KgM6iH8Y6w6Hscq_U-usbukwU259chsN6L8vxmc74A%40mail.gmail.com In-Reply-To: <1420230521-8365-1-git-send-email-raphael.kubo.da.costa@intel.com> ``` On Fri, Jan 2, 2015 at 3:28 PM, Raphael Kubo da Costa 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 > --- > 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 > + > +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 < $(git rev-parse --short HEAD) > EOF > > -- > 2.1.4 ```