[Git] Check for smart git remote before using depth option #36

segiddins · 2014-09-29T03:37:58Z

@fabiopelosin this PR is incomplete (aka tests fail and the URI.regexp isn't great), but this approach should address the issues people are having where their remotes aren't supporting the depth option.

alloy · 2014-09-29T09:22:46Z

@segiddins Please never refer to intimate knowledge, but explain the full issue instead so everyone can look at it and understand it right the first time around. Now I don’t know what this is about exactly, is it that non-smart remotes would be slow with --depth, do they not work at all?

orta · 2014-09-29T09:26:49Z

This relates to: CocoaPods/CocoaPods#2537

alloy · 2014-09-29T09:31:43Z

@orta Thanks 👍

alloy · 2014-09-29T09:32:46Z

@segiddins This looks good to me. Assuming it actually works, what type of feedback are you looking for?

fabiopelosin · 2014-09-29T14:57:52Z

Looks good to me as well! 👍

segiddins · 2014-09-29T15:19:40Z

@alloy non-smart remotes don't support the depth option at all. I'm looking for feedback in 3 places.

the tests fail because the downloader url isn't actually a URL
URI.regexp doesn't currently match git@... URIs. I think I might write a custom URI matcher for them.
Is it OK to add a dependency on nap in here?

alloy · 2014-09-29T15:56:34Z

URI.regexp doesn't currently match git@... URIs. I think I might write a custom URI matcher for them.

Is it OK to add a dependency on nap in here?

Those both sound perfectly fine by me 👍

segiddins · 2014-09-29T17:48:36Z

Looks like https://github.com/martinemde/gitable matches SCP-style URIs. I can either use it or take inspiration from its parsing to create a gem that doesn't depend upon addressable

segiddins · 2014-09-29T18:15:37Z

@alloy I don't really like my solution, as it relies on another gem for git URI parsing. On the other hand, the gem could be used to improve the generation of names from source URIs

segiddins · 2014-09-29T23:01:56Z

But your code actually returns false if the url is invalid :P

Fixed

segiddins · 2014-10-03T18:10:32Z

After playing around with this, I realize it's totally useless if the endpoint requires authentications 😞

…lone

…Error so the Downloader can catch it Helpful for CocoaPods/cocoapods-downloader#36

…ruction to a helper method

segiddins · 2014-10-03T21:26:56Z

@kylef @alloy this is working in my testing, please take a quick look before I merge.

kylef · 2014-10-03T21:56:00Z

lib/cocoapods-downloader/git.rb

@@ -55,22 +55,50 @@ def download_head!
      #         If any specific option should be ignored and the HEAD of the
      #         repo should be cloned.
      #
-      def clone(force_head = false)
+      # @param  [Bool] shallow_clone


Should probably document the default value?

We didn't do that elsewhere in here, and I think YARD will actually handle that.

that YARD handles that.

[Git] Check for smart git remote before using depth option

segiddins · 2014-10-04T00:11:13Z

:D

alloy · 2014-10-06T09:20:06Z

@segiddins Great work!! 👍

fabiopelosin · 2014-10-06T22:53:24Z

I think this should've released bumping the patch version.

segiddins · 2014-10-06T22:54:23Z

@fabiopelosin I think we were waiting until we release CocoaPods 0.34.2

kylef · 2014-10-06T22:54:52Z

@fabiopelosin Yes, we're doing 0.34.2 and patching all the things soon. (Discussions in Slack).

fabiopelosin · 2014-10-06T23:15:48Z

(For clarity: My point was that there is no need to wait for CP, this could have been released straight away)

segiddins · 2014-10-06T23:17:16Z

@fabiopelosin I don't have gem release powers, but feel free for you or Kyle to do it if you want.

kylef · 2014-10-06T23:24:37Z

I did think about this the other day, but then though something else might make it into this patch release. There is also the issue with GHE not supporting --depth (which is a GHE issue, but we could make a workaround).

segiddins · 2014-10-06T23:25:30Z

@kylef the GHE issue should've been fixed by this PR.

kylef · 2014-10-06T23:30:39Z

I'll see to releasing this in the morning 👍

fabiopelosin · 2014-10-07T09:24:41Z

I did think about this the other day, but then though something else might make it into this patch release. There is also the issue with GHE not supporting --depth (which is a GHE issue, but we could make a workaround).

I commented for this reason. There would have been no problem in doing two patch releases (especially as they are almost automated for a single dependency like cocoapods-downloader). I learned my lesson, about releasing often 😉

alloy · 2014-10-07T09:29:02Z

Agreed :)

nutritious · 2014-12-04T00:05:25Z

Hi guys,

I am reading through the comments here and I mostly likely missing something, but it looks like the problem is NOT fixed. When I try to use GHE with https option, it still hangs on git clone and there is no option to manually remove --depth 1. I have the latest cocoapods downloader 0.8 and GHE 11.10.348. I temp. switched to using git@, but it won't let us go behind firewall in this case. It also generates unpleasant warnings doing pod repo push ...

Any advise will be much appreciated,
Andrey

[Git] Check for smart git remote before using depth option

995f2f3

Use gitable to parse git URIs

d9c0904

segiddins added 3 commits September 29, 2014 11:17

[RuboCop] Useless assignment to variable

7830cb3

Presume that any non-http-based URI is a smart remote

f0a03af

[Git] Presume a remote is smart if it is an ssh-style URI

c06a68b

segiddins mentioned this pull request Oct 3, 2014

Adding optional support of --depth for github only. #31

Closed

segiddins added 2 commits October 3, 2014 13:36

[Documentation] Fix spelling error

565f8ff

[Git] Properly handle 'dumb' remotes by retrying with a non-shallow c…

136c8ae

…lone

segiddins added a commit to CocoaPods/CocoaPods that referenced this pull request Oct 3, 2014

[Downloader] ensure that a downloader failure results in a Downloader…

13bd791

…Error so the Downloader can catch it Helpful for CocoaPods/cocoapods-downloader#36

segiddins added 3 commits October 3, 2014 13:58

[RuboCop] Split expected Exception message over multiple lines

d1d76f0

[RuboCop] Reduce complexity in Git#clone by extracting argument const…

f6fc701

…ruction to a helper method

[Gemspec] Remove un-needed nap dependency

be4282c

kylef reviewed Oct 3, 2014
View reviewed changes

kylef added a commit that referenced this pull request Oct 4, 2014

Merge pull request #36 from CocoaPods/smart-git

f949cee

[Git] Check for smart git remote before using depth option

kylef merged commit f949cee into master Oct 4, 2014

kylef deleted the smart-git branch October 4, 2014 00:08

segiddins mentioned this pull request Oct 5, 2014

Add option to avoid shallow git clone #35

Closed

segiddins mentioned this pull request May 31, 2015

creating a shallow clone crash git repo server CocoaPods/CocoaPods#2960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Git] Check for smart git remote before using depth option #36

[Git] Check for smart git remote before using depth option #36

segiddins commented Sep 29, 2014

alloy commented Sep 29, 2014

orta commented Sep 29, 2014

alloy commented Sep 29, 2014

alloy commented Sep 29, 2014

fabiopelosin commented Sep 29, 2014

segiddins commented Sep 29, 2014

alloy commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Oct 3, 2014

segiddins commented Oct 3, 2014

kylef Oct 3, 2014

segiddins Oct 3, 2014

kylef Oct 3, 2014

segiddins commented Oct 4, 2014

alloy commented Oct 6, 2014

fabiopelosin commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

fabiopelosin commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

fabiopelosin commented Oct 7, 2014

alloy commented Oct 7, 2014

nutritious commented Dec 4, 2014

[Git] Check for smart git remote before using depth option #36

[Git] Check for smart git remote before using depth option #36

Conversation

segiddins commented Sep 29, 2014

alloy commented Sep 29, 2014

orta commented Sep 29, 2014

alloy commented Sep 29, 2014

alloy commented Sep 29, 2014

fabiopelosin commented Sep 29, 2014

segiddins commented Sep 29, 2014

alloy commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Sep 29, 2014

segiddins commented Oct 3, 2014

segiddins commented Oct 3, 2014

kylef Oct 3, 2014

Choose a reason for hiding this comment

segiddins Oct 3, 2014

Choose a reason for hiding this comment

kylef Oct 3, 2014

Choose a reason for hiding this comment

segiddins commented Oct 4, 2014

alloy commented Oct 6, 2014

fabiopelosin commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

fabiopelosin commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

segiddins commented Oct 6, 2014

kylef commented Oct 6, 2014

fabiopelosin commented Oct 7, 2014

alloy commented Oct 7, 2014

nutritious commented Dec 4, 2014