Re: [PATCH v17 1/2] refactor format_branch_comparison in preparation

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 13, 2026, 10:41 UTC
Message-ID: <d8ecb19d-8a17-4979-b663-a6d928709e4b@gmail.com>
In-Reply-To: <20260112194733.23192-1-haraldnordgren@gmail.com>

On 12/01/2026 19:47, Harald Nordgren wrote:

Show 16 quoted lines

>> It is hard to discuss this without knowing what actually breaks. Are you
>> talking about the tests added in this series? If so that means we're
>> expecting a different behavior to what "git push" actually does. As Ben
>> has pointed out elsewhere in this thread, if you're pushing back to a
>> different branch on the same remote as the upstream branch you need to
>> set `push.default=current`.
> 
> Yes, it's my new tests that are breaking. Maybe it's easiest if you check
> out the `seen` branch which now has this logic, play with the code and run
> the tests to see when it breaks.
> 
> I designed the feature around 'push.default=current' which I use.
> 
> If we would design the feature around 'push.default=upstream' then what is
> the point? 🤗 Why do we need to show status for both an upstream and a push
> branch if we are already pushing to our upstream branch?

I'm not suggesting we design the feature around 'push.default=upstream', I'm suggesting that we design it to respect 'push.default' so it gives sensible output (i.e. something that resembles what "git push" would do) whatever the setting.

Show 7 quoted lines

>> The benefit is that you get a sane interface rather that returning two
>> different versions of the same string in two different ways (one from
>> the function's return value and the other from a function parameter). It
>> also matches what we do for the upstream branch.
> 
> That's a good point about matching what we do for upstream branch, I'll
> take a look.

Thanks, I think that would be cleaner

Show 5 quoted lines

>> I can't seem to see that test. If we're printing the advice once for the
>> upstream branch and once for the default push remote I think that would
>> be ok.
> 
> This test is also part of my patch 🤗

Oh, I was looking in junio's "seen" branch from the 8th and it wasn't there. I've updated and I can see it now. I can see tests for

  - upstream differs from the local branch, no push branch shown
  - upstream and push branches differ from the local branch
  - upstream and push branches match the local branch

I can't see a test for the local branch differing from the upstream branch when the push branch matches the local branch. As far as I can see in that case we don't show the advice when we should do (and we currently do show it)

> I disagree about showing the same advice twice.

I think it is less bad than not showing it when the upstream branch does not match the local branch.

>> But we set show_divergance_advice to false for the push branch so there
>> is no need to check the flag.
> 
> Good point! I'll update it!

That's great

Thanks

Phillip

> 
> Harald