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

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

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.

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.

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

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.