Re: [PATCH] status: compare against both upstream and push branch by default

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

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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> In a triangular workflow, where a branch is pulled from one place and
> pushed to another, "git status" only compares the branch to its
> upstream. It says nothing about the branch it is pushed to unless
> status.compareBranches is configured.

I would have expected such an explanation to be on a commit that
allowed this default change to be such a small code change (i.e.,
the commit that added support for status.compareBranches).  In other
words, shouldn't the first paragraph be more like this:

    Commit XXXXX added status.compareBranches so that "git status"
    can show comparison of the current branch with its upstream in
    addition to where the branch is pushed to, which is great to
    support the triangular workflow.  It however did not make it the
    default to show both.

It would highlight that the primary task this commit wants to
achieve is to change the default, which is explained next.

> Show both comparisons by default. When the two resolve to the same
> branch, which is the common case, only one comparison is shown, so the
> output for most users does not change. Setting status.compareBranches
> to "@{upstream}" gives the previous output.

Most users not seeing any change is not a good excuse to make a
default change.  Thinking through different use cases and see if
there are cases where negative impact is made relative to the
current one is how we should evaluate.

In this case, especially given that this is "git status" output
that is meant to shown to interactive end-users, I do not think
of a reason why those who use triangular workflows would not want to
see both by default.

> -test_expect_success 'status.compareBranches with only upstream shows only upstream' '
> -	test_config -C test status.compareBranches "@{upstream}" &&
> -	git -C test checkout main &&
> -	git -C test 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
> -'

What's this deletion about?  Is this duplicated or something?

> @@ -361,9 +348,8 @@ test_expect_success 'status.compareBranches with only push shows only push' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'status.compareBranches shows ahead of both upstream and push branch' '
> +test_expect_success 'status shows ahead of both upstream and push branch by default' '
>  	test_config -C test push.default current &&
> -	test_config -C test status.compareBranches "@{upstream} @{push}" &&
>  	git -C test checkout -b feature3 origin/main &&
>  	git -C test push origin HEAD &&
>  	(cd test && advance work) &&

Even if the default changed, shouldn't we make sure that the
configuration also work as expected?

In other words, the changes to the tests I would expect from a
change like this is (1) to update the tests that have been relying
on the old default to gain test_config to explicitly set the old
default, and (2) new test that demonstrates the new behaviour
without configuration.  That assumes that the test coverage before
the patch is adequate, which may not have been the case for this
test and further changes may be warranted.  It is not quite clear
to me, but the addition we see below is such a case our existing
test failed to cover?

One large argument the proposed commit log message hangs its
decision to change the default on is that we won't see extra output
when @{upstream} and @{push} happen to point at the same place, so
naturally I'd expect to have two tests that show the two cases
(i.e., one with triangular, the other just pulling from where you
push to).  With this test update, do we have adequate test coverage
for these cases?  If so, that is something we should explain in the
proposed commit log message, too.

Thanks.

> +test_expect_success 'status.compareBranches with only upstream shows only upstream' '
> +	test_config -C test push.default current &&
> +	test_config -C test status.compareBranches "@{upstream}" &&
> +	git -C test checkout feature3 &&
> +	git -C test status >actual &&
> +	cat >expect <<-EOF &&
> +	On branch feature3
> +	Your branch is ahead of ${SQ}origin/main${SQ} by 1 commit.
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'