Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream

From: Jeff King <peff@peff.net>
Date: May 22, 2015, 00:46 UTC
Message-ID: <20150522004643.GA11739@peff.net>
In-Reply-To: <xmqqpp5tivga.fsf@gitster.dls.corp.google.com>

On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> > Note also that the original may dereference branch->merge[0] even if it
> > is NULL. I think that can't actually happen in practice (we only
> > allocate branch->merge if we have at least one item to put in it, and
> > all of the checks of branch->merge[0] are actually being over-careful).
> > But the code I just wrote above does not have that problem.
> 
> Perhaps update the patch with this explanation in the log message,
> as a separate preparatory step?

I decided on a separate patch on top which improves the logic and explains the issues. Here it is (it goes on top of the existing patch 8, "report specific errors from branch_get_upstream").

-- >8 --
Subject: remote.c: untangle error logic in branch_get_upstream

The error-diagnosis logic in branch_get_upstream was copied straight from sha1_name.c in the previous commit. However, because we check all error cases and upfront and then later diagnose them, the logic is a bit tangled. In particular:

  - if branch->merge[0] is NULL, we may end up dereferencing
    it for an error message (in practice, it should never be
    NULL, so this is probably not a triggerable bug).

  - We may enter the code path because branch->merge[0]->dst
    is NULL, but we then start our error diagnosis by
    checking whether our local branch exists. But that is
    only relevant to diagnosing missing merge config, not a
    missing tracking ref; our diagnosis may hide the real
    problem.

Instead, let's just use a sequence of "if" blocks to check for each error type, diagnose it, and return NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 1b7051a..d2519c2 100644
--- a/remote.c
+++ b/remote.c
@@ -1721,18 +1721,25 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 {
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
-	if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {
+
+	if (!branch->merge || !branch->merge[0]) {
+		/*
+		 * no merge config; is it because the user didn't define any,
+		 * or because it is not a real branch, and get_branch
+		 * auto-vivified it?
+		 */
 		if (!ref_exists(branch->refname))
 			return error_buf(err, _("no such branch: '%s'"),
 					 branch->name);
-		if (!branch->merge)
-			return error_buf(err,
-					 _("no upstream configured for branch '%s'"),
-					 branch->name);
+		return error_buf(err,
+				 _("no upstream configured for branch '%s'"),
+				 branch->name);
+	}
+
+	if (!branch->merge[0]->dst)
 		return error_buf(err,
 				 _("upstream branch '%s' not stored as a remote-tracking branch"),
 				 branch->merge[0]->src);
-	}
 
 	return branch->merge[0]->dst;
 }

-- 
2.4.1.528.g00591e3