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

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

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

> +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.

> +	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).

>  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");


> 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
	'

> +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 ...

> ...

> +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.