Re: [PATCH v3 1/5] stat_tracking_info: return +1 when branches not equal

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2018, 21:41 UTC
Message-ID: <xmqqr2r5e1yh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180103214733.797-2-git@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 9 quoted lines

> -		if (stat_tracking_info(branch, &num_ours,
> -				       &num_theirs, NULL)) {
> +		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> +				       NULL, AHEAD_BEHIND_FULL) < 0) {
> ...
> -		if (stat_tracking_info(branch, &num_ours,
> -				       &num_theirs, NULL))
> +		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> +				       NULL, AHEAD_BEHIND_FULL) < 0)

Mental note: any code that reacted to stat_tracking_info() returning non-zero was reacting to "no useful info in num_{ours,theirs}". They now have to compare the returned value with "< 0" for the same purpose.

Show 23 quoted lines

> ...
>   * 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.
> + * upstream defined, or ref does not exist).  Returns 0 if the commits are
> + * identical.  Returns 1 if commits are different.
>   */
>  int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
> -		       const char **upstream_name)
> +		       const char **upstream_name, enum ahead_behind_flags abf)
>  {
>  	struct object_id oid;
>  	struct commit *ours, *theirs;
> @@ -2019,6 +2026,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
>  		*num_theirs = *num_ours = 0;
>  		return 0;
>  	}
> +	if (abf == AHEAD_BEHIND_QUICK)
> +		return 1;
> ...
>  	argv_array_clear(&argv);
> -	return 0;
> +	return 1;
>  }

When a caller gets +1 from the function, it is not safe to peek into *num_{ours,theirs} if it passed _QUICK.

> @@ -2064,7 +2073,8 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
> -	if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
> +	if (stat_tracking_info(branch, &ours, &theirs, &full_base,
> +			       AHEAD_BEHIND_FULL) < 0) {

Sane conversion to the new return value convention.

Show 11 quoted lines

> diff --git a/wt-status.c b/wt-status.c
> index 94e5eba..8f7fdc6 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1791,7 +1791,8 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>  
>  	color_fprintf(s->fp, branch_color_local, "%s", branch_name);
>  
> -	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {
> +	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base,
> +			       AHEAD_BEHIND_FULL) < 0) {

Ditto.

Show 7 quoted lines

> @@ -1928,7 +1929,8 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
>  		/* Lookup stats on the upstream tracking branch, if set. */
>  		branch = branch_get(branch_name);
>  		base = NULL;
> -		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) == 0);
> +		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind,
> +					      &base, AHEAD_BEHIND_FULL) >= 0);

If a later step plans to (conditionally) allow _QUICK to be passed here, this conversion is questionable, because ab_info being true no longer is a sign that nr_{ahead,behind} are valid.

I suspect that moving the "s/ab_info/sti/" bits around here from step [2/5] to this commit may make the result after this patch more consistent, but it is not a big deal either way.

>  		if (base) {
>  			base = shorten_unambiguous_ref(base, 0);
>  			fprintf(s->fp, "# branch.upstream %s%c", base, eol);