Skip to content
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

libfetchers/github: allow slashes in refs #4115

Merged
merged 1 commit into from Jan 18, 2021

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 6, 2020

Refs #4061

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Oct 18, 2020

@edolstra in #4061 you stated that this is probably not a good idea due to its ambiguity. As an alternative to this patch, would it make sense to improve the error to explicitly mention the workaround? Just having an invalid URL-error in that case is probably kinda confusing.

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Nov 8, 2020

@edolstra may I ask what's your opinion re my previous comment?

@AluisioASG
Copy link

@AluisioASG AluisioASG commented Nov 20, 2020

If you keep only the regex change, does it work for the ?ref= case? That doesn't seem as ambiguous to me.

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Nov 20, 2020

Seems fine as well. @edolstra wdyt?

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Jan 8, 2021

cc @edolstra how shall we proceed here?

@edolstra edolstra merged commit 1bbc66f into NixOS:master Jan 18, 2021
2 checks passed
@edolstra
Copy link
Member

@edolstra edolstra commented Jan 18, 2021

Thanks. I was worried that it might introduce ambiguities in the other fetchers but that appears not to be the case.

@Ma27 Ma27 deleted the slashes-in-github-branches branch Jan 18, 2021
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 6, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/flake-input-schema-slash-in-branch-name/11396/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants