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

[Harald Nordgren <haraldnordgren@gmail.com>]

> Using an enum for a set of flags is a bit confusing.

The point of the flag and the bitmasking is to selectively turn off the push
and pull advice advice from the relevant branch when the push branch
comparison is active.

In an earlier implementation the advice logic was moved to the caller
instead 'format_branch_comparison', but it's more faithful to the original
to have the advice logic inside 'format_branch_comparison'. Maybe I
misunderstood your comments around this?

Would happily take a suggestion on a nicer way to handle it.

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

> 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? 🤗

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

> Having to test the flags each time is a bit cumbersome. We could define
> a couple of local variables to simplify this
> 
> 	bool want_push_advice = (advice_flags & BRANCH_MODE_PUSH) &&
> 				advice_enabled(ADVICE_STATUS_HINTS);
> 	bool want_pull_advice = advice_flags & BRANCH_MODE_PULL &&
> 				advice_enabled(ADVICE_STATUS_HINTS);
> 
> Then we can simplify the above to
> 
> 	if (want_push_advice)

Done, will be in the next batch!

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

> Here we set an enum to a value that is not a member of the enum.

I use bitmasking to enable both modes by default. But see my comments above
maybe there is a nicer way to handle all of this.

> This combined with checking "push_branch_modes & BRANCH_MODE_PUSH" below 
> ensures we skip the push branch if push_sti < 0. That's good but it is a 
> bit hard to follow.

Possibly this can be done in a nicer way too. But at least I try to
encapsulate the complexity to only here, to allow the rest of the code to
be more straightforward. A good trade-off I think.


Harald