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

Remove VCS urls pertaining to git protocol from docs #7938

Merged
merged 4 commits into from
Apr 1, 2020

Conversation

deveshks
Copy link
Contributor

Fixes #1983 by removing vcs urls pertaining to git protocol from pip install doc

@uranusjr
Copy link
Member

Maybe it’s a good idea to change git:// (which is the same as git+git://) to git+https:// in the next block as well.

news/1983.doc Outdated Show resolved Hide resolved
@deveshks
Copy link
Contributor Author

Maybe it’s a good idea to change git:// (which is the same as git+git://) to git+https:// in the next block as well.

So you mean changing for e.g.
git://git.example.com/MyProject.git@master#egg=MyProject to
git+https://git.example.com/MyProject.git@master#egg=MyProject
for all 4 links in the next block?

@sbidoul
Copy link
Member

sbidoul commented Mar 30, 2020

There are also a couple of git and git+git mentions in the paragraph above (pip currently supports cloning over...).

Perhaps we should add paragraph saying that these forms are supported too but their use discouraged? My concern is that a future maintainer would drop the feature because it is undocumented.

@deveshks
Copy link
Contributor Author

deveshks commented Mar 30, 2020

Hi @sbidoul

I assume you mean the line

pip currently supports cloning over git, git+http, git+https, git+ssh, git+git and git+file:

I think we can either remove the mention of git and git+git from this line, or rephrase it as

pip currently supports cloning over git, git+http, git+https, git+ssh, git+git and git+file, but note that the git and git+git are based on the Git protocol which is obsolete and currently insecure, so their use is discouraged

@sbidoul
Copy link
Member

sbidoul commented Mar 30, 2020

@deveshks the rephrasing you propose sounds good. I don't know myself how obsolete the git protocol is, but it is certainly insecure according to the git documentation.

@deveshks
Copy link
Contributor Author

Hi @sbidoul

So we propose that git and git+git should avoid being used by rephrasing, and do not give example on how to use them as well to further our point, is that what we are looking for with this PR?

@uranusjr
Copy link
Member

So you mean changing for e.g. git://git.example.com/MyProject.git@master#egg=MyProject to git+https://git.example.com/MyProject.git@master#egg=MyProject for all 4 links in the next block?

Yup.


The Git protocol is not obsolete or deprecated, only not recommended due to its lack of security. HTTP without SSL is in the same boat here.

Personally I would prefer to simply say

pip currently supports cloning over git, git+http, git+https, git+ssh, git+git and git+file.

without elaboration, and remove the git:// examples. If the user can infer how to use git+git, they probably know what they’re doing.

@deveshks
Copy link
Contributor Author

Hi @uranusjr

I think adding some elaboration won't harm, but yes I would reword it in a better way.

pip currently supports cloning over git, git+http, git+https, git+ssh, git+git and git+file, but note that the git and git+git are based on the Git protocol which are not recommended due to it's lack of security.

Do you also want to remove the git+http as well?

Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Do you also want to remove the git+http as well?

git+http can also be noted as insecure yes.

@deveshks
Copy link
Contributor Author

deveshks commented Mar 30, 2020

Do you also want to remove the git+http as well?

git+http can also be noted as insecure yes.

Hi @xavfernandez ,

Do you want me to remove git+http from the PR as well before you merge this?

@deveshks
Copy link
Contributor Author

Hi @uranusjr

@xavfernandez approved this PR, but also suggested me git+http as insecure, which I will do, but do we also want to remove it from the list of supported URLs?

@uranusjr
Copy link
Member

It does not hurt to also mention HTTP is insecure, but I think it is fine either way, since HTTP is more widely known to be insecure than git://.

@deveshks
Copy link
Contributor Author

It does not hurt to also mention HTTP is insecure, but I think it is fine either way, since HTTP is more widely known to be insecure than git://.

Thanks @uranusjr , I have reworded it to

pip currently supports cloning over git, git+http, git+https, git+ssh, git+git and git+file, 
but note that the git and git+git which are based on the Git protocol, and git+http are not recommended due to their lack of security.:

@deveshks deveshks requested a review from uranusjr March 31, 2020 07:39
@uranusjr
Copy link
Member

Hmm, the wording reads like only the HTTP one is not recommended. I would say something like

but note that the git, git+git, and git+http are not recommended due to their lack of security. (The former two uses the Git Protocol.)

With a link to Git documentation on “the Git Protocol.”

Also there’s a trailing colon at the end of the paragraph. Looks like a typo?

@deveshks
Copy link
Contributor Author

Hi @uranusjr

Thanks for the suggestions. I made the requested changes. Please take a look :)

@deveshks deveshks closed this Mar 31, 2020
@deveshks deveshks reopened this Mar 31, 2020
@pradyunsg pradyunsg closed this Mar 31, 2020
@pradyunsg pradyunsg reopened this Mar 31, 2020
@pradyunsg
Copy link
Member

I hate wasting compute time to re-run failed CI but all these services seems to be flaky. :(

@uranusjr
Copy link
Member

uranusjr commented Mar 31, 2020

### ERRORED 08:43:45Z

- An unexpected error occurred when executing this Action. Please submit a new support request using our Support website:

https://support.github.com/contact

Please include this unique ID in your request: CEA5:4BD3:447EF4:621576:5E8302BA

LOL come on GitHub. (Edit: Sumitted a support request as the message suggested.)

@deveshks
Copy link
Contributor Author

deveshks commented Mar 31, 2020

Hi @pradyunsg , @uranusjr

I think this failure might be from an older run, since I see a total of 52 checks being run. In any case, what is the resolution for this, we cannot keep re-running tests. Do you want me to create another PR taking the code from here, and link to this?

@pradyunsg
Copy link
Member

LOL come on GitHub. (Edit: Sumitted a support request as the message suggested.)

Ah well, I did it too. Not sure who did it first among us but whatever. :)

docs/html/reference/pip_install.rst Outdated Show resolved Hide resolved
news/1983.doc Outdated Show resolved Hide resolved
docs/html/reference/pip_install.rst Outdated Show resolved Hide resolved
docs/html/reference/pip_install.rst Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member

Do you want me to create another PR taking the code from here, and link to this?

Eh; we'll just ignore that failing check for now. I think this PR needs a bit more iteration, so maybe that failing check will go away when you update this anyway?

@deveshks
Copy link
Contributor Author

Hi @pradyunsg

I have updated the PR with the requested changes, and the checks have passed as well.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

One final minor comment; LGTM otherwise! ^>^

@deveshks
Copy link
Contributor Author

deveshks commented Apr 1, 2020

One final minor comment; LGTM otherwise! ^>^

Hi @pradyunsg , I have fixed the text according to your suggestion. Please take a look :)

@deveshks deveshks closed this Apr 1, 2020
@deveshks deveshks reopened this Apr 1, 2020
@deveshks deveshks requested a review from pradyunsg April 1, 2020 15:22
@pradyunsg pradyunsg changed the title Remove vcs urls pertaining to git protocol from docs Remove VCS urls pertaining to git protocol from docs Apr 1, 2020
@pradyunsg pradyunsg merged commit 43f82c2 into pypa:master Apr 1, 2020
@deveshks deveshks deleted the remove-obsolete-git-urls branch April 1, 2020 15:37
@pradyunsg
Copy link
Member

Thanks for the PR @deveshks! And for the reviews @xavfernandez and @uranusjr! ^>^

@deveshks
Copy link
Contributor Author

deveshks commented Apr 1, 2020

Thanks for the quick turnaround @pradyunsg :)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip should warn on git:// protocol
5 participants