Use proper git URL for GitHub repos by Chocobo1 · Pull Request #11475 · rust-lang/cargo

Chocobo1 · 2022-12-12T09:58:10Z

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 · 2022-12-12T09:58:14Z

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.

weihanglo

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

cargo/src/cargo/sources/git/utils.rs

Lines 1163 to 1165 in 4a8d17e

    
           // 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 · 2022-12-12T11:17:55Z

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):

https://stackoverflow.com/questions/11068576/why-do-some-repository-urls-end-in-git-while-others-dont
Almost everywhere on Github web interface is providing *.git links. This strongly suggest that Github wants it to be seen as bare repo.
https://stackoverflow.com/questions/7861184/what-is-the-difference-between-git-init-and-git-init-bare/7861243#7861243
This seems to explains why Github uses a bare repo.

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 · 2022-12-12T12:37:28Z

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.

Chocobo1 · 2022-12-12T21:22:37Z

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 · 2022-12-13T00:28:57Z

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 · 2022-12-13T04:49:26Z

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):
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.

weihanglo

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 · 2022-12-13T08:59:28Z

@bors r+

bors · 2022-12-13T08:59:30Z

📌 Commit ce174d4 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2022-12-13T08:59:37Z

⌛ Testing commit ce174d4 with merge 7779606...

bors · 2022-12-13T09:47:51Z

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

Chocobo1 · 2022-12-13T11:23:13Z

@weihanglo
Thanks for the review!

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)

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`

Use proper git URL for GitHub repos

ce174d4

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 assigned ehuss Dec 12, 2022

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

weihanglo reviewed Dec 12, 2022

View reviewed changes

weihanglo approved these changes Dec 13, 2022

View reviewed changes

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 merged commit 7779606 into rust-lang:master Dec 13, 2022

Chocobo1 deleted the gitUrl branch December 13, 2022 11:22

weihanglo mentioned this pull request Dec 14, 2022

Update cargo rust-lang/rust#105704

Merged

ehuss added this to the 1.68.0 milestone Dec 30, 2022

weihanglo mentioned this pull request Feb 29, 2024

doc: Edits for git/path dependency sections #13341

Merged

	// 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);

Conversation

Chocobo1 commented Dec 12, 2022

Uh oh!

rustbot commented Dec 12, 2022

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Chocobo1 commented Dec 12, 2022

Uh oh!

weihanglo commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chocobo1 commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo commented Dec 13, 2022

Uh oh!

Chocobo1 commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weihanglo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo commented Dec 13, 2022

Uh oh!

bors commented Dec 13, 2022

Uh oh!

bors commented Dec 13, 2022

Uh oh!

bors commented Dec 13, 2022

Uh oh!

Chocobo1 commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

weihanglo commented Dec 12, 2022 •

edited

Loading

Chocobo1 commented Dec 12, 2022 •

edited

Loading

Chocobo1 commented Dec 13, 2022 •

edited

Loading

weihanglo left a comment •

edited

Loading