-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Downloader] Parse tree URLs when cloning repos #2119
[Downloader] Parse tree URLs when cloning repos #2119
Conversation
Signed-off-by: Toby <tobyharradine@gmail.com>
I'll test this in more depth later, it does appear to work, but I think since this is so easily detected and parsed, a more satisfactory solution would cover this behavior for each major online VCS, not just githubs url syntax. Edit note, my phrasing left that unclear, it's been updated to clarify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works properly for GitHub, and GitLab. Does not work for bitbucket. Have not looked at others. While I'm not sure it matters enough to special case each other online VCS, I'd at least like a sanity check that we're not trying to do anything fancy with a URL scheme which isn't expected, so I'd like this to not match if not using a supported site, and require the old format in that case (a clone URL + branch name)
Signed-off-by: Toby Harradine <tobyharradine@gmail.com>
@mikeshardmind See updated, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the quick changes. Have tested, and this now behaves as expected in the github/gitlab case, without modifying other cases without the same defined behavior.
This addition handles a seemingly common misuse of
[p]repo add
, where users include the trailing/tree/branch
part of a repo URL. The intended behaviour of how these are parsed should be explained by the supplied test case.