Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper

[Jeff King <peff@peff.net>]

On Thu, May 21, 2015 at 11:07:33AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > All of the information needed to find the @{upstream} of a
> > branch is included in the branch struct, but callers have to
> > navigate a series of possible-NULL values to get there.
> > Let's wrap that logic up in an easy-to-read helper.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> This step in the whole series is a gem.  I cannot believe that we
> were content having to repeat that "branch->merge[0]->dst if we can
> dereference down to that level" this many times.  Nice clean-up.

There is a related cleanup I resisted, which is that several call-sites
will call stat_tracking_info, then later look directly at
branch->merge[0]->dst without a check for NULL (fill_tracking_info is
such a site).

This works because stat_tracking_info's return value tells us that we
did indeed find an upstream to compare with. But it feels a little leaky
to me. One solution is for stat_tracking_info to pass out the name of
thte upstream, making the caller side something like:

diff --git a/builtin/branch.c b/builtin/branch.c
index cc55ff2..edc4deb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -425,11 +425,12 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	int ours, theirs;
 	char *ref = NULL;
 	struct branch *branch = branch_get(branch_name);
+	const char *upstream;
 	struct strbuf fancy = STRBUF_INIT;
 	int upstream_is_gone = 0;
 	int added_decoration = 1;
 
-	switch (stat_tracking_info(branch, &ours, &theirs)) {
+	switch (stat_tracking_info(branch, &ours, &theirs, &upstream)) {
 	case 0:
 		/* no base */
 		return;
@@ -443,7 +444,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	}
 
 	if (show_upstream_ref) {
-		ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+		ref = shorten_unambiguous_ref(upstream, 0);
 		if (want_color(branch_use_color))
 			strbuf_addf(&fancy, "%s%s%s",
 					branch_get_color(BRANCH_COLOR_UPSTREAM),


This is still a little error-prone, though. We assume "upstream" was
filled in depending on the return value of stat_tracking_info. I wonder
if we could get rid of the weird tri-state return value from
stat_tracking_info, and just have callers detect the "there is no base"
case by checking "upstream != NULL".

I dunno. It is not buggy in any of the current callers, so it might not
be worth spending too much time on.

-Peff