Re: [PATCH v14 1/2] refactor: format_branch_comparison in preparation

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

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Refactor format_branch_comparison function in preparation for showing
> comparison with push remote tracking branch.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>  remote.c | 99 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 40 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 59b3715120..58093f64b0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2237,66 +2237,50 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
>  	return stat_branch_pair(branch->refname, base, num_ours, num_theirs, abf);
>  }
>  
> -/*
> - * Return true when there is anything to report, otherwise false.
> - */
> -int format_tracking_info(struct branch *branch, struct strbuf *sb,
> -			 enum ahead_behind_flags abf,
> -			 int show_divergence_advice)
> +static void format_branch_comparison(struct strbuf *sb,
> +				     int ahead, int behind,
> +				     const char *branch_name,
> +				     int upstream_is_gone,
> +				     enum ahead_behind_flags abf,
> +				     int sti)
>  {
> -	int ours, theirs, sti;
> -	const char *full_base;
> -	char *base;
> -	int upstream_is_gone = 0;
> -
> -	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);
> -	if (sti < 0) {
> -		if (!full_base)
> -			return 0;
> -		upstream_is_gone = 1;
> -	}
> -
> -	base = refs_shorten_unambiguous_ref(get_main_ref_store(the_repository),
> -					    full_base, 0);
>  	if (upstream_is_gone) {
>  		strbuf_addf(sb,
>  			_("Your branch is based on '%s', but the upstream is gone.\n"),
> -			base);
> +			branch_name);
>  		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addstr(sb,
>  				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
>  	} else if (!sti) {
>  		strbuf_addf(sb,
>  			_("Your branch is up to date with '%s'.\n"),
> -			base);
> +			branch_name);

So, we used to say this when stat-tracking-info gave 0, i.e., ours
and theirs have not diverged.  But now you decided to make the
caller responsible for making the call to stat_tracking_info(), it
seems to me that this if/else-if arm is somewhat redundant given
that the caller can and will signal the same thing with both ahead
and behind being zero.

IOW, it smells to me that leaving "sti" as a parameter to this
function is an incomplete refactoring---I say "smell" because I
haven't seen the other, new, caller that will be added in the future
step of this patch series.

And if the new calling convention is to let the caller be
responsible for calling stat_tracking_info() and figuring out the
base branch name, it probably is also better to have the caller
handle upstream_is_gone case.  This is especially true if your plan
is to reuse this "branch comparison" helper to compare a branch with
another branch that is *NOT* its upstream (e.g., where the result is
pushed to).

>  	} else if (abf == AHEAD_BEHIND_QUICK) {
>  		strbuf_addf(sb,
>  			    _("Your branch and '%s' refer to different commits.\n"),
> -			    base);
> +			    branch_name);
>  		if (advice_enabled(ADVICE_STATUS_HINTS))
>  			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
>  				    "git status --ahead-behind");
> -	} else if (!theirs) {

As the code already handled !sti, which is equivalent to (!theirs &&
!ours), in the original, !theirs here meant (!theirs && ours), which
is (ahead && !behind) in the new world order, which we see a few lines
below.

> +	} else if (ahead == 0 && behind == 0) {

And this is what the above (!sti) should have checked for, after the
helper function lost the sti parameter.

Do not compare with 0 for equality.  Instead write it like so:

	} else if (!ahead && !behind) {

I won't repeat for other style violations of the same kind.

> +		strbuf_addf(sb,
> +			_("Your branch is up to date with '%s'.\n"),
> +			branch_name);

> +	} else if (ahead > 0 && behind == 0) {
>  		strbuf_addf(sb,
>  			Q_("Your branch is ahead of '%s' by %d commit.\n",
>  			   "Your branch is ahead of '%s' by %d commits.\n",
> -			   ours),
> -			base, ours);
> -		if (advice_enabled(ADVICE_STATUS_HINTS))
> -			strbuf_addstr(sb,
> -				_("  (use \"git push\" to publish your local commits)\n"));
> -	} else if (!ours) {
> +			   ahead),
> +			branch_name, ahead);
> +	} else if (behind > 0 && ahead == 0) {
>  		strbuf_addf(sb,
>  			Q_("Your branch is behind '%s' by %d commit, "
>  			       "and can be fast-forwarded.\n",
>  			   "Your branch is behind '%s' by %d commits, "
>  			       "and can be fast-forwarded.\n",
> -			   theirs),
> -			base, theirs);
> -		if (advice_enabled(ADVICE_STATUS_HINTS))
> -			strbuf_addstr(sb,
> -				_("  (use \"git pull\" to update your local branch)\n"));
> -	} else {
> +			   behind),
> +			branch_name, behind);
> +	} else if (ahead > 0 && behind > 0) {
>  		strbuf_addf(sb,
>  			Q_("Your branch and '%s' have diverged,\n"
>  			       "and have %d and %d different commit each, "
> @@ -2304,13 +2288,48 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  			   "Your branch and '%s' have diverged,\n"
>  			       "and have %d and %d different commits each, "
>  			       "respectively.\n",
> -			   ours + theirs),
> -			base, ours, theirs);
> -		if (show_divergence_advice &&
> -		    advice_enabled(ADVICE_STATUS_HINTS))
> +			   ahead + behind),
> +			branch_name, ahead, behind);
> +	}
> +}
> +
> +/*
> + * Return true when there is anything to report, otherwise false.
> + */
> +int format_tracking_info(struct branch *branch, struct strbuf *sb,
> +			 enum ahead_behind_flags abf,
> +			 int show_divergence_advice)
> +{
> +	int ours, theirs, sti;
> +	const char *full_base;
> +	char *base;
> +	int upstream_is_gone = 0;
> +
> +	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, 0, abf);
> +	if (sti < 0) {
> +		if (!full_base)
> +			return 0;
> +		upstream_is_gone = 1;
> +	}
> +
> +	base = refs_shorten_unambiguous_ref(get_main_ref_store(the_repository),
> +					    full_base, 0);
> +
> +	format_branch_comparison(sb, ours, theirs, base, upstream_is_gone, abf, sti);
> +	if (sti > 0 && abf != AHEAD_BEHIND_QUICK) {
> +		if (!theirs && advice_enabled(ADVICE_STATUS_HINTS)) {
> +			strbuf_addstr(sb,
> +				_("  (use \"git push\" to publish your local commits)\n"));
> +		} else if (!ours && advice_enabled(ADVICE_STATUS_HINTS)) {
> +			strbuf_addstr(sb,
> +				_("  (use \"git pull\" to update your local branch)\n"));
> +		} else if (ours && theirs && show_divergence_advice &&
> +			   advice_enabled(ADVICE_STATUS_HINTS)) {
>  			strbuf_addstr(sb,
>  				_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));
> +		}
>  	}
> +
>  	free(base);
>  	return 1;
>  }

Overall I think this is a reasonable direction to go, even though
passing the "sti" thing is iffy, and the implementation in the
function may have small rooms for improvements.

I didn't look at minute details to ensure that the output for all
cases are identical to that of the original, though.