Allow user to specify default remote when there's no remote named "origin" #1730

jcansdale · 2018-06-13T15:52:29Z

What this PR does

Some users like to rename their "origin" remote so they're forced to be explicit about which repository they're pushing to. When this happend the extension wasn't able to detect their repository originated from GitHub.

This PR takes advantage of the fact the order of remotes doesn't change when a remote is renamed, added or deleted. When a user renames their "origin" remote, it will remain the first defined remote. We can fall back to using the first remote as the originating remote/clone URL.

Use the first remote as the originating remote when no "origin" is defined

TODO

The default remote "origin" is hard-coded in a bunch of other places:

  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(290):                        var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(374):                    var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(440):                        var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(500):                    var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(613):                var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\PullRequestService.cs(666):                var remote = await gitClient.GetHttpRemote(repo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryForkService.cs(79):                            var remote = await gitClient.GetHttpRemote(activeRepo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryForkService.cs(106):            await gitClient.SetRemote(repository, "origin", originUri);
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryPublishService.cs(50):                                     await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl));
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryPublishService.cs(51):                                     await gitClient.Push(repo.LocalRepo, "master", "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryPublishService.cs(52):                                     await gitClient.Fetch(repo.LocalRepo, "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.App\Services\RepositoryPublishService.cs(53):                                     await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin");
  C:\Source\github.com\github\VisualStudio\src\GitHub.InlineReviews\Services\PullRequestSessionService.cs(237):                    await gitClient.Fetch(repo, "origin", sha, pullHeadRef);

How to test

git remote rename origin renamed_origin
Confirm that GitHub section doesn't appear on Team Explorer - Home
Install the build from this PR
Confirm that GitHub section now appears on Team Explorer - Home

Fixes #1729

When a repository is cloned, a remote named "origin" is automatically added with the clone URL. The remote list order doesn't change when a remote is added, deleted or renamed. This means that if a user renames "origin", the first remote in the list will still contain the original clone URL.

Check first remote in list is returned when there's no "origin".

This should be used instead of assuming the default remote "origin".

Find original origin using IGitService.FindOriginalRemoteName.

The original remote name will be calculated in the method.

Throw InvalidOperationException if repository contains no remotes. Default to using GetOriginRemoteName in GetHttpRemote.

If remote isn't specified it will use a default origin remote name.

Don't hard-code origin remote as "origin".

Don't hard-code the origin name as "origin".

Should throw InvalidOperationException when no remotes.

GetOriginRemoteName should return the configured remote name.

If there is no "origin" remote, use remote from HEAD or "master" branch.

Rename method from GetOriginRemoteName to GetDefaultRemoteName.

Make sure GitService.GetUri is robust.

We need the default remote to remain consistent.

grokys · 2018-06-18T07:20:46Z

src/GitHub.Exports.Reactive/Services/IGitClient.cs


-        Task<Remote> GetHttpRemote(IRepository repo, string remote);
+        /// <summary>
+        /// Get a <see cref="Remote"/> that used the http protocol.


Should this be uses?

grokys · 2018-06-18T07:21:37Z

src/GitHub.Exports.Reactive/Services/IGitClient.cs

+        /// Get a <see cref="Remote"/> that used the http protocol.
+        /// </summary>
+        /// <param name="repository">The repository</param>
+        /// <param name="remote">The name of the remote or null to use original remote.</param>


What does "original remote" mean here? origin? I've never heard of that referred to as the "original remote". Would "default remote" maybe be better?

It was supposed to find the original remote that was created when the repository was cloned. Alas libgit2 returns remotes in alphabetical order, so it's impossible to find the first created remote short of manually parsing the .git\config file. This would be too much of a hack even for my taste. 😉

grokys · 2018-06-18T07:25:02Z

src/GitHub.Exports/Services/GitService.cs

+            if (remoteName != null)
+            {
+                // Use remote from the "master" branch
+                return remoteName;


This doesn't seem to do what the PR says in the description. The description says "We can fall back to using the first remote as the originating remote/clone URL.", instead this uses master's remote as the default remote.

I'm not sure this is a good idea as I sometimes make master track upstream/master rather than origin/master on forks.

Yes, I agree completely. Making master track upstream/master is almost certainly what people should be doing. I imagine it's pretty rare to want your own master that's independant of upstream/master.

So do you agree the logic here is wrong? Why does it not do what you say it does in the description, i.e. "fall back to using the first remote as the originating remote/clone URL"?

So do you agree the logic here is wrong?

Yup. Fixed in latest push.

Some users like to rename origin and delete master, so this wouldn't have worked for them anyway.

Why does it not do what you say it does in the description, i.e. "fall back to using the first remote as the originating remote/clone URL"?

It turns out that libgit2 sorts the list of remotes in alphabetical order, which completely messes up this logic. 😭

Chances are user would prefer to have master track upstream/master not origin/master!

Some users like to work without an "origin" remote defined. Allow them to set a "ghfvs.origin" config property so GHfVS knows which remote to treat as the origin.

grokys · 2018-06-21T17:37:58Z

@jcansdale is this PR still needed after #1752?

jcansdale · 2018-11-22T11:56:08Z

We're planning to warn the user with a call-out notification (see #2026) rather than allowing them to explicitly name the default remote.

jcansdale added 16 commits June 13, 2018 18:29

Clean up GitServiceTests

ccc49e9

Test different combinations of remotes

2a04e80

Check first remote in list is returned when there's no "origin".

Make "default" a constant defaultOriginName

7b47f79

Add FindOriginalRemoteName to IGitService

636b11e

This should be used instead of assuming the default remote "origin".

Default to use original origin when fetching

b6e4a63

Find original origin using IGitService.FindOriginalRemoteName.

Make IGitService remote names default to null

f725bbd

The original remote name will be calculated in the method.

Rename to GetOriginRemoteName

cdac730

Throw InvalidOperationException if repository contains no remotes. Default to using GetOriginRemoteName in GetHttpRemote.

Let GetHttpRemote accept a default remote name

8acf74b

If remote isn't specified it will use a default origin remote name.

Make PullRequestService use default origin remote

e4f223f

Don't hard-code origin remote as "origin".

Make RepositoryForkService use default origin name

3480932

Don't hard-code the origin name as "origin".

Add test for GetOriginRemoteName when no remotes

651c5f1

Should throw InvalidOperationException when no remotes.

Make SetupLocalRepoMock configure original remote

afb7067

GetOriginRemoteName should return the configured remote name.

Add space after if

d9c0cfa

Fall back to using remotes from HEAD/master

c49765c

If there is no "origin" remote, use remote from HEAD or "master" branch.

Rename to GetDefaultRemoteName and update xmldocs

ba14b3f

Rename method from GetOriginRemoteName to GetDefaultRemoteName.

meaghanlewis added this to the 2.5.4 milestone Jun 14, 2018

jcansdale added 2 commits June 14, 2018 19:03

Add tests for GitService.GetUri returning null

a090996

Make sure GitService.GetUri is robust.

Don't change default remote based on HEAD

8dbeee0

We need the default remote to remain consistent.

grokys suggested changes Jun 18, 2018

View reviewed changes

jcansdale added 3 commits June 18, 2018 19:52

Merge branch 'master' into fixes/1729-find-original-remote-name

fbf7c74

Don't fall back to using remote from master branch

3944b4f

Chances are user would prefer to have master track upstream/master not origin/master!

Allow user override origin using ghfvs.origin

5330df2

Some users like to work without an "origin" remote defined. Allow them to set a "ghfvs.origin" config property so GHfVS knows which remote to treat as the origin.

jcansdale changed the title ~~Find original remote when there's no remote named "origin"~~ Allow user to specify origin when there's no remote named "origin" Jun 19, 2018

jcansdale changed the title ~~Allow user to specify origin when there's no remote named "origin"~~ Allow user to specify default remote when there's no remote named "origin" Jun 19, 2018

jcansdale added 3 commits June 20, 2018 16:26

Improve xmldoc comments for GetHttpRemote

3cfb8d2

Fix build after merge

93d3382

Merge branch 'master' into fixes/1729-find-original-remote-name

d9a19ce

Merge branch 'master' into fixes/1729-find-original-remote-name

1629695

meaghanlewis removed this from the 2.5.4 milestone Jul 13, 2018

jcansdale closed this Nov 22, 2018

jcansdale deleted the fixes/1729-find-original-remote-name branch November 22, 2018 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow user to specify default remote when there's no remote named "origin" #1730

Allow user to specify default remote when there's no remote named "origin" #1730

Uh oh!

jcansdale commented Jun 13, 2018 •

edited

Loading

Uh oh!

grokys Jun 18, 2018

Uh oh!

grokys Jun 18, 2018

Uh oh!

jcansdale Jun 19, 2018

Uh oh!

grokys Jun 18, 2018

Uh oh!

jcansdale Jun 18, 2018

Uh oh!

grokys Jun 18, 2018

Uh oh!

jcansdale Jun 18, 2018

Uh oh!

grokys commented Jun 21, 2018

Uh oh!

jcansdale commented Nov 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Allow user to specify default remote when there's no remote named "origin" #1730

Allow user to specify default remote when there's no remote named "origin" #1730

Uh oh!

Conversation

jcansdale commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

TODO

How to test

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

jcansdale Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

grokys commented Jun 21, 2018

Uh oh!

jcansdale commented Nov 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcansdale commented Jun 13, 2018 •

edited

Loading