On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:
> > 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@xxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html