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

First version/steps of unraveling fetch code and wrapping libcurl #2376

Merged
merged 6 commits into from
Mar 23, 2023

Conversation

Hind-M
Copy link
Member

@Hind-M Hind-M commented Mar 14, 2023

Part of #2356 fix
Based on #2366

@JohanMabille
Copy link
Member

This one can be rebased now that #2366 has been merged

@Hind-M Hind-M marked this pull request as ready for review March 22, 2023 10:13
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Outdated Show resolved Hide resolved
libmamba/src/core/curl.cpp Show resolved Hide resolved
libmamba/src/core/curl.cpp Show resolved Hide resolved
libmamba/src/core/curl.hpp Show resolved Hide resolved
}
}

const std::pair<std::string_view, CurlLogLevel> CURLHandle::init_curl_ssl_session()
Copy link
Member

Choose a reason for hiding this comment

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

I think the name is misleading, this function does not initialize anything but simply get info about the ssl backend. Maybe get_ssl_backend_info would be a better name.

Regarding the implementation, I think it should return info->backend instead of a string_view with a log, and have a log function performing the switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the implementation, I think it should return info->backend instead of a string_view with a log, and have a log function performing the switch.

Yes exactly, that's why I did it this way until we decide what we do with logging (I left curl specific code inside on purpose, but that's a way among others of doing it). I added an explicit comment though for later...

Add comments, rewording and factorization
@JohanMabille JohanMabille merged commit 6df0f26 into mamba-org:main Mar 23, 2023
@Hind-M Hind-M deleted the curl_handle branch March 24, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants