On Thu, May 21, 2015 at 11:07:33AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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@xxxxxxxx>
>
> 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
--
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