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

Use proper git URL for GitHub repos #11475

Merged
merged 1 commit into from Dec 13, 2022
Merged

Use proper git URL for GitHub repos #11475

merged 1 commit into from Dec 13, 2022

Conversation

Chocobo1
Copy link
Contributor

It can be seen in the following link that a git repo URL from GitHub should end with ".git": https://docs.github.com/en/get-started/getting-started-with-git/about-remote-repositories#about-remote-repositories

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thank you for the patch.

Could you provide a reference saying that URL without .git suffix is inappropriate? I know this is somewhat a convention, but I couldn't find any official doc from GitHub telling their preference.

It also needs a bit more work to interact with URL containing .git. See

// Trim off the `.git` from the repository, if present, since that's
// optional for GitHub and won't work when we try to use the API as well.
let repository = repository.strip_suffix(".git").unwrap_or(repository);

Personally, I lean towards leaving it as-is.

@Chocobo1
Copy link
Contributor Author

Could you provide a reference saying that URL without .git suffix is inappropriate? I know this is somewhat a convention, but I couldn't find any official doc from GitHub telling their preference.

I can't find one either.
However after more digging, I found something interesting (at least for me):

I would prefer to stick to what the repo hosting service provides.

It also needs a bit more work to interact with URL containing .git. See:

IMO the comment in code is misleading, or at least it misunderstood it.
Here is how Github (https://docs.github.com/en/rest/commits/commits#list-branches-for-head-commit) puts it:

repo string Required
The name of the repository. The name is not case sensitive.

My understanding is that "repo URL" != "repo name" as the former could be a link to a git bare repo (which would contains ".git" suffix).

@weihanglo
Copy link
Member

weihanglo commented Dec 12, 2022

Thank you for the response! I also like the explicitness of .git and happy to merge this. However, I got some blind spots and need to make them clear :)

This strongly suggest that Github wants it to be seen as bare repo.

I doubt it. Bare repositories are less useful for average GitHub users. And average GitHub users usually expect a repo to have a working directory. We could have a contact to people from GitHub and see if cloning one or the other can reduce the stress of their services, though I also guess not, as cloning without .git is already widely used.

This seems to explains why Github uses a bare repo.
I would prefer to stick to what the repo hosting service provides.

GitHub as a service can choose any kind of infra to help themselves. This could be nothing related to us cloning bare repos without .git extension. For example, git clone --bare https://github.com/rust-lang/cargo clone a bare repo whereas git clone https://github.com/rust-lang/cargo.git contains a working directory. We could observe that GitHub handles these variants interchangeably for clients.

My understanding is that "repo URL" != "repo name" as the former could be a link to a git bare repo (which would contains ".git" suffix).

I believe "repo URL" != "repo name" is why Cargo chops .git extension off to make the URL reflect the {owner}/{repo} format, but not for the reason you mentioned. GitHub handles that for clients. You cannot even create a newrepo.git on GitHub. But you're right, services outside GitHub may need .git extension to work with bare repos.

image

@Chocobo1
Copy link
Contributor Author

Chocobo1 commented Dec 12, 2022

This strongly suggest that Github wants it to be seen as bare repo.

Just to clarify myself. I realized I chosen my word badly... What I actually meant is: when creating a new git repo on github. Github probably creates a bare repo (or something alike internally) and therefore they expose the url with the .git suffix (to follow the convention?). Anyway, this is just guesswork and I still couldn't find github statement on it.

This strongly suggest that Github wants it to be seen as bare repo.

I doubt it. Bare repositories are less useful for average GitHub users. And average GitHub users usually expect a repo to have a working directory.

I think there are misunderstanding here (poor choice of words from me).
First, whether github is using a bare repo or not doesn't really affect the user. The following is just speculation of why github repos has .git suffix:
I meant Github might internally host a repo as a bare repo (or whatever alike). And github actually do want users to know that they are working with a bare repo remotely, not one with a working directory/copy as to lessen the fear of getting the "git push" error/warning mentioned in the aforementioned stackoverflow link. Note that I'm not saying pushing to a github url without .git suffix will result in error/warning, it is just how github want users to perceive.

@weihanglo
Copy link
Member

Indeed. I believe GitHub uses something similar internally, but I am still not convinced that GitHub wants users to perceive the existence of bare repositories, given that it probably wouldn't bring any value to GitHub. Who else needs to know how a git server works when 99.9% of one's time on git is being a dumb client?

Anyway, it's really not useful for us debating here without any information from GitHub people. I do want to merge this pull request as myself loves .git much more 😆. However, sorry I didn't mean to turn you down. It just that we need to clarify the purpose of a change even it is only a simple doc update. If we could get official advice for favouring .git, or more other benefits with .git, we could then merge this.
(such as internally Cargo uses bare repo in a few places, but I don't think Cargo wants someone to depend on implementation details)

@Chocobo1
Copy link
Contributor Author

Chocobo1 commented Dec 13, 2022

If we could get official advice for favouring .git, or more other benefits with .git, we could then merge this.

I'm not sure what kind of statement you need from github when their official document (link in the opening post) already include .git in all the examples. And their web interface also includes it (this indication itself is strong enough IMO): screenshot
Note that the top (?) button links to the documentation.

I might be wrong, yet I can't understand why one would like to outsmart github and omit the ".git" part when none of the official document says it is supported (at least I didn't found it). The fact that it will work is probably just a convenience feature and IMO one should not rely on it comfortably.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Fair enough. I guess what GitHub tries to replicate is somewhat a DWIM like the following: https://github.com/git/git/blob/c48035d29b4e524aed3a32f0403676f0d9128863/path.c#L772-L791

That works fine with file: protocol with official git CLI. That said, you're completely correct that a tool probably shouldn't rely on the undocumented behaviour. Although the actual convenience path is determined by either Cargo or git services, Cargo's doc could have a more consistent view of what a GIT_URL looks like even it doesn't affect any actual behaviour.

Sorry for being like a dumbass, and thank you for your patience!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 13, 2022

📌 Commit ce174d4 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2022
@bors
Copy link
Collaborator

bors commented Dec 13, 2022

⌛ Testing commit ce174d4 with merge 7779606...

@bors
Copy link
Collaborator

bors commented Dec 13, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 7779606 to master...

@bors bors merged commit 7779606 into rust-lang:master Dec 13, 2022
@Chocobo1 Chocobo1 deleted the gitUrl branch December 13, 2022 11:22
@Chocobo1
Copy link
Contributor Author

@weihanglo
Thanks for the review!

weihanglo added a commit to weihanglo/rust that referenced this pull request Dec 14, 2022
8 commits in 70898e522116f6c23971e2a554b2dc85fd4c84cd..cc0a320879c17207bbfb96b5d778e28a2c62030d
2022-12-05 19:43:44 +0000 to 2022-12-14 14:46:57 +0000
- artifact deps should works when target field specified coexists with `optional = true` (rust-lang/cargo#11434)
- Add `home` crate to cargo crates (rust-lang/cargo#11359)
- Use proper git URL for GitHub repos (rust-lang/cargo#11475)
- Fixes flock(fd, LOCK_UN) emulation on Solaris. (rust-lang/cargo#11474)
- Allow Check targets needed for optional doc-scraping to fail without killing the build (rust-lang/cargo#11450)
- fix: gleaning rustdocflags from `target.cfg(…)` is not supported (rust-lang/cargo#11323)
- Fix typo (rust-lang/cargo#11470)
- resolver.md: Fix naming in example (rust-lang/cargo#11469)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2022
Update cargo

8 commits in 70898e522116f6c23971e2a554b2dc85fd4c84cd..cc0a320879c17207bbfb96b5d778e28a2c62030d 2022-12-05 19:43:44 +0000 to 2022-12-14 14:46:57 +0000
- artifact deps should works when target field specified coexists with `optional = true` (rust-lang/cargo#11434)
- Add `home` crate to cargo crates (rust-lang/cargo#11359)
- Use proper git URL for GitHub repos (rust-lang/cargo#11475)
- Fixes flock(fd, LOCK_UN) emulation on Solaris. (rust-lang/cargo#11474)
- Allow Check targets needed for optional doc-scraping to fail without killing the build (rust-lang/cargo#11450)
- fix: gleaning rustdocflags from `target.cfg(…)` is not supported (rust-lang/cargo#11323)
- Fix typo (rust-lang/cargo#11470)
- resolver.md: Fix naming in example (rust-lang/cargo#11469)

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants