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

Configuration support for enabling/disabling shallow cloning #1559

Closed
wants to merge 1 commit into from

Conversation

nwinkler
Copy link
Contributor

@nwinkler nwinkler commented Oct 8, 2014

Added a configuration option in the .bowerrc file to allow enabling/disabling the shallow clone feature per host.

This fixes #1558. Let me know if the configuration is acceptable. Another option would be to provide an array or comma separated list of hosts which allow shallow cloning, but this format provides greater flexibility. Let me know if you would like to see changes.

Since this adds a new option to the .bowerrc file, the documentation needs to be updated as well. Let me know if you want me to do this.

The configuration looks like this:

{
    "hosts": [
        {
            "name": "git.myservice.com",
            "shallowClone": true
        }
    ]
}

The hosts element accepts an array of host entries, each one with a name and a shallowClone attribute. The name of the host is matched against the target host in the clone/install command.

If the target host is not part of the configuration, or no hosts configuration has been provided at all, Bower defaults to false for the shallow clone feature, the current default behavior for all non-GitHub repos.

I hope this addresses the issue described in #1389, #886 and #1393, which took a bit of a shotgun approach by disabling shallow cloning for everyone, even for hosts where it worked perfectly fine. I would like to see this PR (or a similar option) in Bower to allow a flexible setting for everyone. For us, disabling shallow cloning has a huge performance impact on our build process - and I would like to get that fixed as soon as possible.

Let me know if this PR (my first for Bower) is acceptable or whether you would like to see changes. Adapting the configuration to a different format shouldn't be too difficult.

Added a configuration option in the .bowerrc file to allow
enabling/disabling the shallow clone feature per host.

The configuration looks like this:

```
{
    "hosts": [
        {
            "name": "git.myservice.com",
            "shallowClone": true
        }
    ]
}
```

The `hosts` element accepts an array of host entries, each one with a
`name` and a `shallowClone` attribute. The name of the host is matched
against the target host in the clone/install command.

If the target host is not part of the configuration, or no `hosts`
configuration has been provided at all, Bower defaults to _false_ for
the shallow clone feature, the current default behavior for all
non-GitHub repos.
@nwinkler
Copy link
Contributor Author

Any updates on including this in the next minor release?

@sheerun
Copy link
Contributor

sheerun commented Oct 28, 2014

I think it needs more agreement from bower team..

@nwinkler
Copy link
Contributor Author

How can we make that happen? Whom do we need to get involved?

@Maks3w
Copy link

Maks3w commented Nov 2, 2014

👍 Could you update the doc files with this new config key?

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 3, 2014

Where can I find the documentation to update?

@Maks3w
Copy link

Maks3w commented Nov 3, 2014

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 4, 2014

Added documentation here: bower/bower.github.io#84

@sindresorhus
Copy link
Contributor

I'd rather see automatic detection of shallow clone support than more options. Eg: https://github.com/dimitri/el-get/pull/1921/files

@sheerun
Copy link
Contributor

sheerun commented Nov 4, 2014

@sindresorhus We already automatically detected shallow clones. It doesn't work, because host hangs indefinitely if shallow clone is not supported. It caused bower to hang too, so we just disabled shallow clones except github.

@sindresorhus
Copy link
Contributor

@sheerun Well, it seems to work there, so they're obviously doing something better. I don't really care, I'm just tired of the options overload.

@sheerun
Copy link
Contributor

sheerun commented Nov 4, 2014

They are whitelisting 3 hosts, including GitHub and bitbucket, and checking Content-Type: application/x-git-upload-pack-advertisement for http / https protocol as explained in http://stackoverflow.com/questions/9270488/is-it-possible-to-detect-whether-a-http-git-remote-is-smart-or-dumb

@drublic Tries to implement bitbucket whitelisting in #1568 and admittedly bower never checked for "Content-Type: application/x-git-upload-pack-advertisement". Maybe @nwinkler would like to implement those.

@Maks3w
Copy link

Maks3w commented Nov 4, 2014

Bitbucket works well with shallow clones too. So this option allow us to quickly adopt new providers without to develop, send a PR with the new resolver and wait for next release.

Also allow manage the infinite world of (private/internal/local) Git "servers"

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 4, 2014

I've started to take a look at this. According to the el-get PR, it looks like Bitbucket does not send the required smart header, so it will need to be whitelisted.

I'll try to get something implemented for checking the content type. Currently verifying with our internal Git repo to make sure it will work for us.

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 4, 2014

Just checked with our own Git repo (which supports shallow cloning), and it is indeed sending the Content-Type: application/x-git-upload-pack-advertisement header.

@drublic
Copy link
Contributor

drublic commented Nov 4, 2014

I am a little behind solving the remaining problem with my PR which
includes a BitBucket resolver. This might be mostly about Unit tests. I
will try to dig asap.

On Tue, Nov 4, 2014, 17:31 Nils Winkler notifications@github.com wrote:

Just checked with our own Git repo (which supports shallow cloning), and
it is indeed sending the Content-Type:
application/x-git-upload-pack-advertisement header.


Reply to this email directly or view it on GitHub
#1559 (comment).

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 5, 2014

BTW: I just noticed that BitBucket also returns the correct content type when queried. The trick is to set the user agent to something containing git/. Found this info here: https://answers.launchpad.net/launchpad/+question/218551

Example:

curl -si -A git/ https://bitbucket.org/mugqic/maven-repo.git/info/refs?service=git-upload-pack

Just picked a random public repo on BitBucket, and the above includes the Content-Type: application/x-git-upload-pack-advertisement header as well.

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 6, 2014

I've started an implementation of this feature (automatic detection of servers that support shallow cloning), and I think I'm pretty much done with the heavy lifting. It needs some more testing against some public repos, and there's probably some minor issues that I'll find along the way.

You can see the work in progress here: https://github.com/nwinkler/bower/blob/detect-smart-git-http/lib/core/resolvers/GitRemoteResolver.js#L181

I think I've hit a roadblock, though: Since the check for servers that support shallow cloning is done using an HTTP HEAD request, repos that require authentication will return a 401 status code, and not provide the required Content-Type header. How should we handle repos that require authentication?

Git requests to these repos will use things like credential helpers, so for the actual Git work, this does not apply, but since the request will be done using HTTP HEAD, it doesn't use the credential helpers. Any ideas?

@nwinkler
Copy link
Contributor Author

nwinkler commented Nov 7, 2014

Also noticed that BitBucket doesn't like HTTP HEAD requests - it responds with a 405 status code. For BitBucket, one might have to fall back to a GET request.

@nwinkler
Copy link
Contributor Author

I've provided automatic detection of smart Git hosts in #1628 - closing this one. Please take a look at #1628.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Provide configuration for shallow cloning
5 participants