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

Mentioning libssh2 in remote's pydoc #463

Merged
merged 1 commit into from
Dec 30, 2014
Merged

Conversation

KINFOO
Copy link
Contributor

@KINFOO KINFOO commented Dec 30, 2014

Relates to #456

@jdavid jdavid merged commit e807ad4 into libgit2:master Dec 30, 2014
@carlosmn
Copy link
Member

This seems like a really odd place to mention this. It doesn't say why or how it may need libssh2. The referenced issue is about not noticing that the headers for libssh2 weren't installed and as such this text wouldn't have changed the outcome. Adding querying for git_libgit2_features() and mentioning that should provide a more direct method by which a user can see if the installation is as they expect.

@KINFOO
Copy link
Contributor Author

KINFOO commented Jan 13, 2015

It aims to remind users about libssh2 when they read generated pydoc.

That is what I expected when calls to ssh related methods failed.

@carlosmn You think this has to be removed?

@jdavid
Copy link
Member

jdavid commented Jan 14, 2015

Ideally, when a method requiring libssh2 fails, it should give an informative error to the user. Then this comment in the docstring should be removed.

Looking at issues #379 and #456 today the error is GitError: This transport isn't implemented. Sorry.
So we should catch this error, and give a better message to the user in this case, something like ssh transport not supported, please compile libgit2 with libssh2, or something like that.

Now, looking at the changes in the recently released lbgit2 v0.22, the message This transport isn't implemented. Sorry is gone. So maybe now libgit2 raises a more informative message by itself, @carlosmn ?

@carlosmn
Copy link
Member

The error was changed, since the text we had up to 0.21 was the first error message, which was expected to trigger when we had in fact not implemented support for https or ssh. It's been changed to say the url is unuspported.

The message however doesn't mention a particular dependency because that part of libgit2 does not know about the particular dependency. All it knows is whether there is a supported url schema or not, and libssh2 is just the default way to get ssh.

The problem with the mention of libssh2 is that doing the obvious thing (installing libssh2) won't change anything, and it's just one possible dependency. You also need OpenSSL to use https but that's not mentioned. Soon (hopefully) you might also want GnuTLS or SecureTransport to use https. Are we going to mention everything in the hopes the user guesses they need to install the dev package and rebuild libgit2 from that?

Getting the dependencies in the right place is something that needs to happen during the installation of libgit2, which the package manager might handle but if not, these dependencies are something you would want

@jdavid
Copy link
Member

jdavid commented Jan 14, 2015

It could be a generic message, it just needs to give enough clues so the user will find his way. The message up to 0.21 was misleading, it implied an upstream problem (not implemented).

Ok, so lets revert this commit and move to 0.22, we will see if this problem pops again.

@jdavid
Copy link
Member

jdavid commented Jan 16, 2015

commit reverted

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

Successfully merging this pull request may close these issues.

3 participants