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

http: add CURLINFO_HTTP_VERSION and %{http_version} #799

Closed
wants to merge 1 commit into from

Conversation

@gevaerts
Copy link
Contributor

@gevaerts gevaerts commented May 11, 2016

Adds access to the effectively used http version to both libcurl and curl.

Initial attempt, almost certainly not final yet.

@jay
Copy link
Member

@jay jay commented May 11, 2016

I like this idea, I have read of it a few times, as recently as last month. What do you think about using the existing CURL_HTTP_VERSION_X_X enumerations for getinfo, and then for the tool something like:

verstring = ((ver == CURL_HTTP_VERSION_2_0) ? "2.0" :
             (ver == CURL_HTTP_VERSION_1_1) ? "1.1" :
             (ver == CURL_HTTP_VERSION_1_0) ? "1.0" : "0");

@gevaerts
Copy link
Contributor Author

@gevaerts gevaerts commented May 11, 2016

I've updated the patch to use CURL_HTTP_VERSION now.

@bagder bagder added the HTTP label May 11, 2016
@bagder
Copy link
Member

@bagder bagder commented May 11, 2016

👍 to merge first thing after the pending release

@bagder
Copy link
Member

@bagder bagder commented May 17, 2016

Note that the travis-ci test failure is because the new symbol is not properly added to docs/libcurl/symbols-in-versions

@gevaerts
Copy link
Contributor Author

@gevaerts gevaerts commented May 17, 2016

I've added it to symbols-in-versions now.

@bagder
Copy link
Member

@bagder bagder commented May 19, 2016

Ganesh Nikam just posted a version of this patch on the mailing list:

https://curl.haxx.se/mail/lib-2016-05/0131.html

He included a little example in his man page that could be merged into this patch to make it perfect I think!

version = "1.1";
break;
case CURL_HTTP_VERSION_2_0:
version = "2";

Choose a reason for hiding this comment

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

I think the string "2.0" would be better here instead of "2".

Copy link
Contributor Author

@gevaerts gevaerts May 19, 2016

Choose a reason for hiding this comment

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

The consensus seems to be that it's http 2, not 2.0. See e.g. https://http2.github.io/faq/#is-it-http20-or-http2

Copy link
Member

@bagder bagder May 19, 2016

Choose a reason for hiding this comment

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

The protocol is named HTTP/2 and there's no ".0" in the name or the version. I think we avoid a lot of problems by sticking to the official name.

Adds access to the effectively used http version to both libcurl and curl.
@bagder bagder closed this in 071c561 May 30, 2016
@gevaerts gevaerts deleted the http_version branch Dec 21, 2016
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants