-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add --proto-default
option
#351
Conversation
This might be short sighted of me but I can't see how anything would break doing this. I modified your patch slightly, draft here. It doesn't try as hard. The reason being the current behavior is to continue to parse and fill regardless of if the http protocol is allowed and I don't know the implications of changing that behavior. Also some style changes to conform. |
Thanks, @jay . I had added the final guessing loop in because it seemed confusing that Anyway, I think your modification is good, and I too believe that this would be a benign patch. |
I think there's a problem with using --proto for this (new) functionality: What if you just want the default guess to be HTTPS if no proto is given, but still allow HTTP if the protocol is explicitly given? Seems perfectly reasonable to me... |
@bagder what are you suggesting, a separate option?
|
Another implementation I'd thought about was to use the Piggy-backing on an existing flag is attractive because it's less code and fewer semi-overlapping options for a user to decipher, but the new option @jay's describing sounds reasonable. |
Using I think I prefer a new option simply because it has the least risk of destroying any existing apps and it offers the biggest flexibility. |
--proto-default
option
I've completely overhauled this PR (I hope it's OK that I'm continuing in this thread). Now it adds a The patch is much bigger. If there's anything I can do to make it easier to review, please let me know. Some examples:
|
The test failure is because symbols-in-versions isn't in sync. I don't know what the correct change to make is there. |
Thanks for your work on this. Don't worry about the symbols thing I'll get it once we figure out if it's going in and what version. I'll do a review soon, but I think we're past the feature window for 7.44 (@bagder ??). I just glanced but I can tell you this line below will have to go, i'm guessing it was for debug:
|
When `--proto-default <protocol>` is passed, any URLs without a protocol specification will attempt to use the supplied default. When the option is not used, curl will follow its usual plan of guessing from the hostname and falling back to 'http'.
Yes, feature window is closed until the coming release, due to ship on Aug 12 |
I took a look at Nathaniel's patch today and I'm wondering if it makes more sense for me to implement something similar using the same syntax as I wrote up a doc to see how it would look, but it's complicated in comparison. I'm not sure if the benefits of having the same syntax and being able to still allow the best guess based on subdomain are worth it. As far as the "popular order" thing goes I couldn't think of a better way to handle that.
If there's not a lot of interest in this I'll continue my review of his patch. |
I don't like how the "popular order" mechanic works, because it has confusing effects. For example, there's no way to handle Badger's original scenario of defaulting to HTTPS but allowing explicitly-specified HTTP URLs. If you allowed changes to the popular order, that'd be cool, but then it wouldn't match |
Actually it allows for that. You could default to https by doing |
Yes, I see you're right. So setting With your proposed change, would |
Yes |
No, I actually meant
That would produce a similar effect to my original PR, but with more flexibility. |
I did a review of your patch this evening, you can see it at jay@a45724e I still have to add a test and do some testing. You'll notice I refer in the docs to the protocol as a scheme as well because it's more correct. Although in the codebase protocol seems to be popular. Also, I have a reservation about using the protocol as a string. I wonder if it would be better to use the CURLPROTO flags instead of a string so the user can do something like |
My personal preference would be to leave it as a string to make it more obvious to users that it really is a single scheme you'd specify. |
All of your changes look good to me @jay . |
The final review changeset can be found at jay@03e5a5a. I added two tests and changed some more documentation. I squashed both changesets into yours and it landed in 9756d1d. Thanks! |
This modifies the protocol-guessing algorithm to respect
the available protocols. For example:
curl --proto -ftp ftp.gnu.org
does not consider FTP as the protocol (instead it will try HTTP)curl --proto '=https' www.gnu.org
will try HTTPSThe impetus here is that I want to be able to get an HTTPS connection when I have a scheme-less URL.
I'm not sure if --proto was the right option to go after here, but like that this stops curl from selecting a disallowed protocol and then complaining about it.