Re: [PATCH v29 2/2] status: add status.compareBranches config for multiple branch comparisons

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2026, 23:18 UTC
Message-ID: <xmqq1pi876p3.fsf@gitster.g>
In-Reply-To: <6a88f41fa52e7b24fdb75dda6cac692014cebbf6.1772056263.git.gitgitgadget@gmail.com>

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

Show 11 quoted lines

> +static char *resolve_compare_branch(struct branch *branch, const char *name)
> +{
> +	const char *resolved = NULL;
> +
> +	if (!branch || !name)
> +		return NULL;
> +
> +	if (!strcasecmp(name, "@{upstream}"))
> +		resolved = branch_get_upstream(branch, NULL);
> +	else if (!strcasecmp(name, "@{push}"))
> +		resolved = branch_get_push(branch, NULL);

As one of the if/else if/ cascade needs {} around a multi-statement block, everybody else needs {}.

If the current branch does not have @{upstream}, we will silently receive NULL in resolved, and return NULL from here, which is exactly what we want. The same stroy for missing @{push}.

Sounds very sensible.

Show 5 quoted lines

> +	else {
> +		warning(_("ignoring value '%s' for status.compareBranches; only @{upstream} and @{push} are supported"),
> +			name);
> +		return NULL;
> +	}

I don't know if which one between the above "warning" and die("unknown/unsupported") is a better design. In any case, the warning() line is overly long and needs to be wrapped (my litmus test to complain about "overly long lines" is after losing three columns to the left for "> +" in e-mail quote like the above the right edge of the line does not fit on my 92-column terminal, which will never happen if you stick to the official "fit 80-column after quoted for a few times in e-mail" guideline).

Show 14 quoted lines

>  static void format_branch_comparison(struct strbuf *sb,
>  				     bool up_to_date,
>  				     int ours, int theirs,
>  				     const char *branch_name,
>  				     enum ahead_behind_flags abf,
> -				     bool show_divergence_advice)
> +				     unsigned flags)
>  {
> +	bool enable_push_advice = (flags & ENABLE_ADVICE_PUSH) &&
> +		advice_enabled(ADVICE_STATUS_HINTS);
> +	bool enable_pull_advice = (flags & ENABLE_ADVICE_PULL) &&
> +		advice_enabled(ADVICE_STATUS_HINTS);
> +	bool enable_divergence_advice = (flags & ENABLE_ADVICE_DIVERGENCE) &&
> +		advice_enabled(ADVICE_STATUS_HINTS);

You do use enable_push_advice twice so that reduces repetition a little bit, but other than that, I am not sure if the above makes it easier to follow the code.

Especially, the roundabout way these three variables are computed, together with the fact that ENABLE_ADVICE_* are not really about "enabling", but the caller decides that it is an applicable advice (e.g., ENABLE_ADVICE_PULL is passed when the iteration over branches.items[] the caller is making is on the upstream branch and "you should pull" is the appropriate situation. It is more like "advice-pull is applicable in this situation", unlike what the advice_enabled() returns, which is "the user wants this kind of advice messages"), I find the resulting code below somehow harder to follow than without these intermediate variables. If they were named "use_*_advice" (instead of "enable_"), perhaps I may find it more palatable.

I find it a lot more disturbing that what the caller specifies in flags and advice_enabled() is pre-combined in these variables. Perhaps splitting them into two separate concerns, like this ...

	bool use_push_advice = (flags & USE_ADVICE_PUSH);
	bool use_pull_advice = (flags & USE_ADVICE_PULL);
	bool use_divergence_advice = (flags & USE_ADVICE_DIVERGENCE);

	...
	if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS))
		... you should pull first ...

... may make it easier to understand and follow?  I dunno.

> -		if (advice_enabled(ADVICE_STATUS_HINTS))
> +		if (enable_push_advice)
>  			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
>  				    "git status --ahead-behind");

Show 47 quoted lines

> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 0b719bbae6..aa9456bb61 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -292,4 +292,339 @@ test_expect_success '--set-upstream-to @{-1}' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'status tracking origin/main shows only main' '
> +	(
> +		cd test &&
> +		git checkout b4 &&
> +		git status >../actual
> +	) &&
> +	cat >expect <<-EOF &&
> +	On branch b4
> +	Your branch is ahead of ${SQ}origin/main${SQ} by 2 commits.
> +	  (use "git push" to publish your local commits)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'status --no-ahead-behind tracking origin/main shows only main' '
> +	(
> +		cd test &&
> +		git checkout b4 &&
> +		git status --no-ahead-behind >../actual
> +	) &&
> +	cat >expect <<-EOF &&
> +	On branch b4
> +	Your branch and ${SQ}origin/main${SQ} refer to different commits.
> +	  (use "git status --ahead-behind" for details)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'setup for compareBranches tests' '
> +	(
> +		cd test &&
> +		git config push.default current &&
> +		git config status.compareBranches "@{upstream} @{push}"
> +	)
> +'

Shoudln't this be part of the next test, with its own teardown? I.e.,

	test_expect_success 'check both @{upstream} and @{push}' '
        	test_config -C test push.default current &&
        	test_config -C test status.compareBranches "..." &&

		git -C test checkout main &&
		git -C status >actual &&
		cat >expect <<-EOF &&
		...
		EOF
		test_cmp expect actual
	'

Show 14 quoted lines

> +test_expect_success 'status.compareBranches from upstream has no duplicates' '
> +	(
> +		cd test &&
> +		git checkout main &&
> +		git status >../actual
> +	) &&
> +	cat >expect <<-EOF &&
> +	On branch main
> +	Your branch is up to date with ${SQ}origin/main${SQ}.
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'

That way, the tests can be kept more independent from each other, and will clean after themselves, which means that we do not have to assume that ...

> ...

Show 6 quoted lines

> +test_expect_success 'clean up after compareBranches tests' '
> +	(
> +		cd test &&
> +		git config --unset status.compareBranches
> +	)
> +'

... this step will never be skipped or fail, which would result in disturbing the tests that come after this step.

In any case, the design of the main part of the patch is looking much nicer than the open-ended one we saw previously.

Thanks.