Opening a gist repository gives a fetch error#2933
Opening a gist repository gives a fetch error#2933alexr00 merged 3 commits intomicrosoft:mainfrom burkeholland:burkeholland-issue2324
Conversation
|
I think the fix would be either to add {
"authority": "gist.github.com",
"fragment": "",
"path": "/20dec7376005dec0a1...",
"query": "",
"scheme": "https"
} |
|
This works for me for both diff --git a/src/authentication/githubServer.ts b/src/authentication/githubServer.ts
index a3859d4..dc87dc0 100644
--- a/src/authentication/githubServer.ts
+++ b/src/authentication/githubServer.ts
@@ -17,6 +17,11 @@ export class GitHubManager {
return false;
}
+ // gist repos are not supported
+ if (host.authority === 'gist.github.com' || /^\/[a-z0-9]+$/i.test(host.path)) {
+ return false;
+ }
+
if (this._servers.has(host.authority)) {
return !!this._servers.get(host.authority);
}EDIT: summary of what it works on:
I'm unaware if |
src/authentication/githubServer.ts
Outdated
| // .wiki repos are not supported | ||
| if (host.path.endsWith('.wiki')) { | ||
| // .wiki/.git repos are not supported | ||
| if (host.path.endsWith('.wiki') || host.path.match(/gist[.]github[.]com/g)) { |
There was a problem hiding this comment.
Does this need to be a global regular expression match (/g)?
There was a problem hiding this comment.
Doesn't. What do you think of the proposed solution from @mcornella? It appears to handle the github.com case as well. I have no idea what that regex does, but his solution for gist.github.com looks more elegant than mine.
There was a problem hiding this comment.
I usually go here to visualize regular expressions: https://regexper.com/#%5E%5C%2F%5Ba-z0-9%5D%2B%24
I don't think we want to exclude all github hosts with non-alphanumeric characters in them (I have no idea how often those will occur with enterprise for example). I'm happy with the simple test that this PR uses.
There was a problem hiding this comment.
I'm ok with ignoring that part and just check the domain name. For that though we need to check host.authority instead of host.path.
There was a problem hiding this comment.
Removed the g for now
There was a problem hiding this comment.
@burkeholland, @mcornella is correct, it is host.authority then needs to be checked for gist.github.com.
There was a problem hiding this comment.
Implemented hostpath check as per @mcornella
alexr00
left a comment
There was a problem hiding this comment.
Just #2933 (comment) then we're good to merge.
alexr00
left a comment
There was a problem hiding this comment.
Doesn't host.authority need to be checked instead of host.path?
Fixes #2324