From: Harald Nordgren <haraldnordgren@gmail.com>
Date: Tue, 20 Oct 2026 09:16:58 GMT
Subject: Re: [PATCH] status: compare against both upstream and push branch by default
Message-ID: <CAHwyqnXab+JEG6i=31xEV9Xmm9fo=h0HsjunMsCBLZ0A4oOQzw@mail.gmail.com>
In-Reply-To: <xmqq33u11etp.fsf@gitster.g>

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

Fair enough, thanks!

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

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

Make sense, yes.


Harald