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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Thu, May 21, 2015 at 02:14:29PM -0400, Jeff King wrote:
>
>> 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:
>
> So just for fun, here is what a whole patch, with the refactoring of the
> return value, would look like.

It looks much nicer and reads better, at least to me.

Nice.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index cc55ff2..8ecabd1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -425,25 +425,19 @@ 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)) {
> -	case 0:
> -		/* no base */
> -		return;
> -	case -1:
> -		/* with "gone" base */
> +	if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> +		if (!upstream)
> +			return;
>  		upstream_is_gone = 1;
> -		break;
> -	default:
> -		/* with base */
> -		break;
>  	}
>  
>  	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),
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 6847400..05dd23d 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -730,7 +730,7 @@ static void populate_value(struct refinfo *ref)
>  				char buf[40];
>  
>  				if (stat_tracking_info(branch, &num_ours,
> -						       &num_theirs) != 1)
> +						       &num_theirs, NULL))
>  					continue;
>  
>  				if (!num_ours && !num_theirs)
> @@ -753,7 +753,7 @@ static void populate_value(struct refinfo *ref)
>  				assert(branch);
>  
>  				if (stat_tracking_info(branch, &num_ours,
> -							&num_theirs) != 1)
> +							&num_theirs, NULL))
>  					continue;
>  
>  				if (!num_ours && !num_theirs)
> diff --git a/remote.c b/remote.c
> index be45a39..6765051 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2016,12 +2016,15 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
>  
>  /*
>   * Compare a branch with its upstream, and save their differences (number
> - * of commits) in *num_ours and *num_theirs.
> + * of commits) in *num_ours and *num_theirs. The name of the upstream branch
> + * (or NULL if no upstream is defined) is returned via *upstream_name, if it
> + * is not itself NULL.
>   *
> - * Return 0 if branch has no upstream (no base), -1 if upstream is missing
> - * (with "gone" base), otherwise 1 (with base).
> + * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
> + * upstream defined, or ref does not exist), 0 otherwise.
>   */
> -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> +		       const char **upstream_name)
>  {
>  	unsigned char sha1[20];
>  	struct commit *ours, *theirs;
> @@ -2032,8 +2035,10 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
>  
>  	/* Cannot stat unless we are marked to build on top of somebody else. */
>  	base = branch_get_upstream(branch, NULL);
> +	if (upstream_name)
> +		*upstream_name = base;
>  	if (!base)
> -		return 0;
> +		return -1;
>  
>  	/* Cannot stat if what we used to build on no longer exists */
>  	if (read_ref(base, sha1))
> @@ -2051,7 +2056,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
>  	/* are we the same? */
>  	if (theirs == ours) {
>  		*num_theirs = *num_ours = 0;
> -		return 1;
> +		return 0;
>  	}
>  
>  	/* Run "rev-list --left-right ours...theirs" internally... */
> @@ -2087,7 +2092,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
>  	/* clear object flags smudged by the above traversal */
>  	clear_commit_marks(ours, ALL_REV_FLAGS);
>  	clear_commit_marks(theirs, ALL_REV_FLAGS);
> -	return 1;
> +	return 0;
>  }
>  
>  /*
> @@ -2096,23 +2101,17 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
>  int format_tracking_info(struct branch *branch, struct strbuf *sb)
>  {
>  	int ours, theirs;
> +	const char *full_base;
>  	char *base;
>  	int upstream_is_gone = 0;
>  
> -	switch (stat_tracking_info(branch, &ours, &theirs)) {
> -	case 0:
> -		/* no base */
> -		return 0;
> -	case -1:
> -		/* with "gone" base */
> +	if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
> +		if (!full_base)
> +			return 0;
>  		upstream_is_gone = 1;
> -		break;
> -	default:
> -		/* with base */
> -		break;
>  	}
>  
> -	base = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> +	base = shorten_unambiguous_ref(full_base, 0);
>  	if (upstream_is_gone) {
>  		strbuf_addf(sb,
>  			_("Your branch is based on '%s', but the upstream is gone.\n"),
> diff --git a/remote.h b/remote.h
> index 3326f2b..312b7ca 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -249,7 +249,8 @@ enum match_refs_flags {
>  };
>  
>  /* Reporting of tracking info */
> -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);
> +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> +		       const char **upstream_name);
>  int format_tracking_info(struct branch *branch, struct strbuf *sb);
>  
>  struct ref *get_local_heads(void);
> diff --git a/wt-status.c b/wt-status.c
> index 38cb165..7c8ae57 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>  
>  	color_fprintf(s->fp, branch_color_local, "%s", branch_name);
>  
> -	switch (stat_tracking_info(branch, &num_ours, &num_theirs)) {
> -	case 0:
> -		/* no base */
> -		fputc(s->null_termination ? '\0' : '\n', s->fp);
> -		return;
> -	case -1:
> -		/* with "gone" base */
> +	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {
> +		if (!base) {
> +			fputc(s->null_termination ? '\0' : '\n', s->fp);
> +			return;
> +		}
> +
>  		upstream_is_gone = 1;
> -		break;
> -	default:
> -		/* with base */
> -		break;
>  	}
>  
> -	base = branch->merge[0]->dst;
>  	base = shorten_unambiguous_ref(base, 0);
>  	color_fprintf(s->fp, header_color, "...");
>  	color_fprintf(s->fp, branch_color_remote, "%s", base);