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

[Phillip Wood <phillip.wood123@gmail.com>]

On 09/01/2026 16:00, Harald Nordgren wrote:
>> Using an enum for a set of flags is a bit confusing.
> 
>> On reflection I wonder if we should be calling branch_get_push() instead
>> of remote_ref_for_branch() and tracking_for_push_dest() as it respects
>> 'push.default' and so the branch it returns is the one that "git push"
>> without any arguments would push to.
> 
> I'm not getting that to work without tests breaking.

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

The advantage would be that it simplifies the code and it means we 
respect `push.default=upstream`.

The current implementation returns NULL if there is a push refspec 
exists but it does not match the current branch - in that case we should 
return a copy of the branch name instead.

>> I think it would be simpler to just return the full refname and let the
>> caller shorten it.
> 
> We could to that but what's the benefit? Shouldn't a helper function reduce
> the work for the caller, not the other way around? 🤗

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.

>> Why are we checking for BRANCH_MODE_PUSH here? Don't we want to show
>> this advice regardless of the mode?
> 
> Yeah, that's a good point. However, this will often lead to the advice
> being shown twice, see this test:
> 
> ```
> status --no-ahead-behind shows diverged from origin/main and ahead of feature branch
> ```

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.

>> If we don't want to show this can't we set show_divergance_adivce to
>> false when we call this function - why is it guarded by BRANCH_MODE_PULL
>> as well?
> 
> This will already have been shown the the pull branch (the upstream
> branch).
> 
> And in the cases when it's NOT shown for the pull branch but only the push
> branch has diverged, then pulling won't solve the problem anyway. (Unless
> you would try to pull from your push branch.)

But we set show_divergance_advice to false for the push branch so there 
is no need to check the flag.

Thanks

Phillip