Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Create remote with the same name as upstream owner when viewing a PR #1752

Merged
grokys merged 13 commits into from
Jun 21, 2018
Merged

Create remote with the same name as upstream owner when viewing a PR #1752

grokys merged 13 commits into from
Jun 21, 2018

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale jcansdale commented Jun 19, 2018 •
edited by meaghanlewis
Loading

The problem

When a user clones a forked repository and views a PR from upstream, we need to do a fetch on upstream in order to view files in the PR. Previously we were creating a temporary remote for upstream with a unique name, fetching from the remote and removing the remote afterwards.

Unfortunately git doesn't completely remove config file entries and users were ending up with lots of empty [remote ...] elements in their .git/config (one for every previewed PR)!

For example:

[remote "jaredpar-8141ae63-26ec-4082-a75a-6cbe223bc357"]
[remote "jaredpar-3c0cb23a-068d-4386-9904-1a4ed48d2f96"]
[remote "jaredpar-9b39068d-6740-4bd3-8eff-2d6a1b01877f"]

What this PR does

  • If a remote with the required URL already exists, use that (i.e. use existing upstream)
  • If no appropriate remote exists, create a new remote with the same name as the remote's owner
    • This is consistent with how we create a remote when checking out a PR
  • If a remote with the name of the remote's owner already exists but with a different URL, fall back to using a temporary remote like we did before (e.g. if the existing remote uses ssh://)

How to test

  1. git clone https://github.com/jcansdale/PullRequestSandbox
  2. type .git/config
[core]
        repositoryformatversion = 0
        filemode = false
        bare = false
        logallrefupdates = true
        symlinks = false
        ignorecase = true
[remote "origin"]
        url = https://github.com/jcansdale/PullRequestSandbox
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
  1. Open PR list from upstream in Visual Studio (this is the default list)
  2. View a PR and open a file diff
  3. type .git/config
[core]
        repositoryformatversion = 0
        filemode = false
        bare = false
        logallrefupdates = true
        symlinks = false
        ignorecase = true
[remote "origin"]
        url = https://github.com/jcansdale/PullRequestSandbox
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
[remote "grokys"]
        url = https://github.com/grokys/PullRequestSandbox
        fetch = +refs/heads/*:refs/remotes/grokys/*

Should contain [remote "grokys"] with url = https://github.com/grokys/PullRequestSandbox.
6. Checkout the PR
7. type .git/config

[core]
        repositoryformatversion = 0
        filemode = false
        bare = false
        logallrefupdates = true
        symlinks = false
        ignorecase = true
[remote "origin"]
        url = https://github.com/jcansdale/PullRequestSandbox
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
[remote "grokys"]
        url = https://github.com/grokys/PullRequestSandbox
        fetch = +refs/heads/*:refs/remotes/grokys/*
[branch "pr/54-testing-marking-the-branch-as-a-pr-4"]
        remote = grokys
        merge = refs/heads/testing-marking-4
        ghfvs-pr-owner-number = "grokys#54"

Should contain [branch ...] with remote = grokys.

Related

Fixes #1749
Fixes #1753

jcansdale added 2 commits June 19, 2018 10:26
@jcansdale
Add failing test for feting from existing remote
cc30981 
We should fetch from the existing remote, not simply avoid adding it!
@jcansdale
Fetch from the matched remote name
8650e71
@jcansdale jcansdale changed the title Use correct remote name when fetching from existing remote [wip] Use correct remote name when fetching from existing remote Jun 19, 2018
jcansdale added 7 commits June 19, 2018 12:03
@jcansdale
Check we create remote with name of upstream owner
42cfd7c 
Unless this remote already exists with different URL.
@jcansdale
Create remote with same name as repository owner
0ff13a9 
When fetching from a URL, create a remote with the same name as the
owner of the target repository. If a remote with the same name already
exists with a different URL, fall back to using a temporary remote with
a unique name.
@jcansdale
Compare Urls ignoring any trailing .git
bc1b52e 
When deciding whether to create a new remove, compare the Urls of
existing remotes ignoring any trailing ".git".
@jcansdale
Ignore case when comparing remote Urls
9e4209f 
Don't create a new remote if there is an existing one with a Url that
that only differs in case.
@jcansdale
Add RepositoryUrlsAreEqual method to UriString
3cdd1f6 
Compare to see if repository URLs are equivalent.
@jcansdale
Update to use UriString.RepositoryUrlsAreEqual
d1ca52c
@jcansdale
Fix failing FetchUsingHttps test
fdd2b48
@jcansdale jcansdale force-pushed the fixes/1749-fetching-from-existing-remote branch from a5eba76 to fdd2b48 Compare June 19, 2018 17:18
@jcansdale jcansdale changed the title [wip] Use correct remote name when fetching from existing remote Create remote with the same name as upstream owner when viewing a PR Jun 19, 2018
@jcansdale jcansdale requested review from grokys and meaghanlewis June 19, 2018 18:11
jcansdale
jcansdale commented Jun 20, 2018
View reviewed changes
Copy link
Collaborator Author

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug here. This should have used remote.Name not defaultOriginName.

src/GitHub.App/Services/GitClient.cs
if (remoteHttpsString.Equals(httpsString, StringComparison.OrdinalIgnoreCase))
if (UriString.RepositoryUrlsAreEqual(new UriString(remote.Url), cloneUrl))
{
return Fetch(repo, defaultOriginName, refspecs);
Copy link
Collaborator Author

@jcansdale jcansdale Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug here. This should have used remote.Name not defaultOriginName.

@meaghanlewis
Copy link
Contributor

meaghanlewis commented Jun 20, 2018

This is working as expected for me @jcansdale and I don't end up with empty remotes anymore

before:
before

after:
after

@jcansdale
Copy link
Collaborator Author

jcansdale commented Jun 21, 2018

@meaghanlewis excellent, thanks for the before and after confirmation! ✨

@jcansdale
Remove extra new lines
2d0cb4c 
I'm not sure where they came from.
grokys
grokys approved these changes Jun 21, 2018
View reviewed changes
Copy link
Contributor

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@grokys grokys merged commit 45bccfd into master Jun 21, 2018
@grokys grokys deleted the fixes/1749-fetching-from-existing-remote branch June 21, 2018 17:36
@grokys grokys mentioned this pull request Jun 21, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@meaghanlewis meaghanlewis Awaiting requested review from meaghanlewis

1 more reviewer

@grokys grokys grokys approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Ready for Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jcansdale @meaghanlewis @grokys