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

Check if GitHub repo exists before every hub clone #1000

Merged
merged 4 commits into from Jan 21, 2016
Merged

Conversation

mislav
Copy link
Owner

@mislav mislav commented Sep 27, 2015

If it doesn't exist, print a more helpful error message about it so the user knows which "owner/repo" pair was tested. This aims to reduce confusion when cloning own repos such as hub clone dotfiles.

The downside is now there is an extra HTTP request on every hub clone because the repository existence is checked every time.

Fixes #868

If it doesn't exist, print a more helpful error message about it so the
user knows which "owner/repo" pair was tested.
The `transformCloneArgs` function now performs API requests and is best
testable through cukes rather than these tests.
When I run `hub clone dotfiles`
Then the exit status should be 1
And the stdout should contain exactly ""
And the stderr should contain exactly "Error: repository mislav/dotfiles doesn't exist\n"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm maybe this message should also say "on github.com" so it's clear that the repo was searched for online and on which host?

Copy link
Contributor

Choose a reason for hiding this comment

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

Error: repository git@github.com:mislav/dotfiles doesn't exist? Do you think the full path is too confusing? If this were used on an enterprise instance, for example, it would be great to specify.

@parkr
Copy link
Contributor

parkr commented Sep 28, 2015

The extra network request is a bummer. I work on a lot of high-latency networks and this would likely reduce efficiency and effectiveness of this tool for me. Perhaps, if the 'git clone' operation fails, hub can print a message containing the relevant information?

"fatal: failed to clone mislav/dotfiles from GitHub.com"

@mislav
Copy link
Owner Author

mislav commented Sep 28, 2015

Then we would have to execute git clone internally, inspect its exit code and message, and replace the message with our own. Doable, but we would have to change more significantly how clone works. It's not a bad idea.

It's just that the API request will likely take the negligible amount of time compared to the clone operation.

@parkr
Copy link
Contributor

parkr commented Sep 28, 2015

Optimizing for the successful case would seem preferable to me. Assume it's going to clone, clone it, and if it fails, print a message. I'm not suggesting you remove the git clone output, just add a line at the bottom of the output with the details about your clone attempt.

How expensive is a failed git clone operation?

What if GitHub's API is down but clone operations are fine? Do I have to wait 30 seconds for it to time out for every clone? If the API is up, this might only add a second to the total real time for the user in the mean case. Is that second, multiplied over thousands of users who don't get 404's, worth it?

Just trying to save some network time here, because latency is horrifying for me. The intent is to print owner and host name information, right? I'm interested in providing that without any added overhead so it doesn't degrade the successful case (where the repo exists).

@parkr
Copy link
Contributor

parkr commented Jan 20, 2016

I don't go to Asia much anymore so I'm feeling less defensive about the extra network request, haha.

This would certainly be a great addition for the next release of hub!

@mislav mislav merged commit 94cb5c9 into master Jan 21, 2016
@mislav mislav deleted the clone-usage branch January 21, 2016 14:41
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.

None yet

2 participants