Get default push remote from configuration

[cruessler]

This PR is in response to #1093. It changes how GitUI gets the remote to push to. It does not change how it gets the remote to pull from, so it doesn’t directly address #1093, but it is a first step towards making the code paths for getting push and pull configuration independent.

It changes the following:

• It adds get_default_remote_for_push_in_repo which reads git configuration related to remote repositories. It first reads branch.<name>.pushRemote, then remote.pushDefault, then branch.<name>.remote. This is according to the docs.

I followed the checklist:

• I added unittests
• I ran make check without errors
• I tested the overall application
• I added an appropriate item to the changelog

Test push behavior in status tab:

• no upstream branch configured
• upstream branch configured, branch.<>.pushRemote is set
• upstream branch configured, remote.pushDefault is set
• upstream branch configured, branch.<>.remote is set

[cruessler]

In addition to branch.<name>.pushRemote, we should also try to read branch.<name>.remote. I’ll update the PR soon.

https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtremote

[extrawurst]

looks good! yeah we should really try to get a unittest on this. i think local remotes to push to should work with libgit. so we can probably just simulate this that way.

[cruessler]

In addition to branch.<name>.pushRemote, we should also try to read branch.<name>.remote. I’ll update the PR soon.

https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtremote

I’ve updated the PR.

[extrawurst]

@cruessler what's the status here? can this be merged? is it ready in your opinion? if not what TODOs do you still see?

[cruessler]

@cruessler what's the status here? can this be merged? is it ready in your opinion? if not what TODOs do you still see?

I found a way to start testing this PR. I’ll add a testing-related TODO list to the initial post soon-ish. The function I added is pretty solid I think, what’s left to do is wire it up in the right places and make sure all combinations of config values work as expected.

I didn‘t leave a more high-level comment related to the overall progress, thinking that my last comment was sufficient. Now I see that was not the case. :-)

[cruessler]

@extrawurst I updated the PR. I think it’s now ready for testing! I tested different configurations for pushing and did not encounter any issues. I don’t have a good feeling for what I might have accidentally broken, so my testing was focused on pushing.

[cruessler]

Also, there is now a bit of duplication, but I thought we could address it in the future, once we now this is the way to go.

[martihomssoler]

I would add back the test_default_remote_inconclusive() test. Since the one that you added checks for push defaults and not for the inconclusive case for fetch repos. Apart from that, it look good. 👍

[cruessler]

I would add back the test_default_remote_inconclusive() test. Since the one that you added checks for push defaults and not for the inconclusive case for fetch repos. Apart from that, it look good. 👍

Thanks for the review, I restored the original test.

[cruessler]

I updated this PR’s description to reflect the fact that this only tackles getting push configuration yet. Given what I’ve learned about the subject, adding something similar for pull configuration should be rather straight-forward. I think it makes sense to create a follow-up PR, but I’ll be equally happy to add more functionality in this PR.

[extrawurst]

No let’s wrap this one up and make a follow up for pull. Can you create that @cruessler ?

[extrawurst]

@cruessler please add a changelog entry and then we are good to go

[cruessler]

No let’s wrap this one up and make a follow up for pull. Can you create that @cruessler ?

Will do, I’m aiming for sometime this weekend.

[cruessler]

@cruessler please add a changelog entry and then we are good to go

Done. (I didn’t find a specific issue this PR fixes, there’s one for pulling which I’ll link to in the follow-up PR.)