Re: [PATCH] status: compare against both upstream and push branch by default
Show 18 quoted lines
> > 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 14 quoted lines
> > -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?I adapted it into another test further down.
But I'll bring things back closer to the old coverage and add more instead.
Show 33 quoted lines
> > @@ -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.