Re: [PATCH v14 1/2] refactor: format_branch_comparison in preparation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2026, 04:40 UTC
- Message-ID
- <xmqq8qeeng7c.fsf@gitster.g>
- In-Reply-To
- <a2c160c53ee0159a88234c64409f2a216d584cc4.1767445236.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 58 quoted lines
> 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).
Show 9 quoted lines
> } 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);Show 83 quoted lines
> + } 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.