Skip to content

Commit

Permalink
Special parsing for git's not-really-urls containing scp host specifi…
Browse files Browse the repository at this point in the history 
…ers (#2990)

* Non-nullable git supportsArchiveCache

* Git URLs are not real URLs, they also support scp host specifiers

Fixes #1796

* Remove workaround replacing ':' in git URLs

(#573, #1796)

* Expect already-parsed URL in Git constructor
  • Loading branch information
@kornelski @arcanis
kornelski authored and arcanis committed Apr 3, 2017
1 parent bc128d1 commit 3b8a1ac
Show file tree
Hide file tree
Showing 9 changed files with 128 additions and 122 deletions.
2 changes: 1 addition & 1 deletion __tests__/resolvers/exotics/bitbucket-resolver.js
View file
Expand Up @@ -120,5 +120,5 @@ test('getGitSSHUrl should return URL containing protocol', () => {
});

expect(url.parse(gitSSHUrl).protocol).toEqual('git+ssh:');
expect(url.parse(Git.cleanUrl(gitSSHUrl)).protocol).toEqual('ssh:');
expect(Git.npmUrlToGitUrl(gitSSHUrl).protocol).toEqual('ssh:');
});
45 changes: 0 additions & 45 deletions __tests__/resolvers/exotics/git-resolver.js
View file

This file was deleted.

2 changes: 1 addition & 1 deletion __tests__/resolvers/exotics/github-resolver.js
View file
Expand Up @@ -46,5 +46,5 @@ test('getGitSSHUrl should return URL containing protocol', () => {
});

expect(url.parse(gitSSHUrl).protocol).toEqual('git+ssh:');
expect(url.parse(Git.cleanUrl(gitSSHUrl)).protocol).toEqual('ssh:');
expect(Git.npmUrlToGitUrl(gitSSHUrl).protocol).toEqual('ssh:');
});
2 changes: 1 addition & 1 deletion __tests__/resolvers/exotics/gitlab-resolver.js
View file
Expand Up @@ -46,5 +46,5 @@ test('getGitSSHUrl should return URL containing protocol', () => {
});

expect(url.parse(gitSSHUrl).protocol).toEqual('git+ssh:');
expect(url.parse(Git.cleanUrl(gitSSHUrl)).protocol).toEqual('ssh:');
expect(Git.npmUrlToGitUrl(gitSSHUrl).protocol).toEqual('ssh:');
});
60 changes: 45 additions & 15 deletions __tests__/util/git.js
View file
Expand Up @@ -5,13 +5,43 @@ import {NoopReporter} from '../../src/reporters/index.js';

jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;

test('cleanUrl', () => {
expect(Git.cleanUrl('git+https://github.com/npm-opam/ocamlfind.git'))
.toEqual('https://github.com/npm-opam/ocamlfind.git');
expect(Git.cleanUrl('https://github.com/npm-opam/ocamlfind.git'))
.toEqual('https://github.com/npm-opam/ocamlfind.git');
expect(Git.cleanUrl('git://github.com/npm-opam/ocamlfind.git'))
.toEqual('git://github.com/npm-opam/ocamlfind.git');
test('npmUrlToGitUrl', () => {
expect(Git.npmUrlToGitUrl('git+https://github.com/npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'https:',
hostname: 'github.com',
repository: 'https://github.com/npm-opam/ocamlfind.git',
});
expect(Git.npmUrlToGitUrl('https://github.com/npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'https:',
hostname: 'github.com',
repository: 'https://github.com/npm-opam/ocamlfind.git',
});
expect(Git.npmUrlToGitUrl('git://github.com/npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'git:',
hostname: 'github.com',
repository: 'git://github.com/npm-opam/ocamlfind.git',
});
expect(Git.npmUrlToGitUrl('git+ssh://git@github.com/npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'ssh:',
hostname: 'github.com',
repository: 'ssh://git@github.com/npm-opam/ocamlfind.git',
});
expect(Git.npmUrlToGitUrl('git+ssh://scp-host-nickname:npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'ssh:',
hostname: 'scp-host-nickname',
repository: 'scp-host-nickname:npm-opam/ocamlfind.git',
});
expect(Git.npmUrlToGitUrl('git+ssh://user@scp-host-nickname:npm-opam/ocamlfind.git'))
.toEqual({
protocol: 'ssh:',
hostname: 'scp-host-nickname',
repository: 'user@scp-host-nickname:npm-opam/ocamlfind.git',
});
});

test('isCommitHash', () => {
Expand All @@ -30,25 +60,25 @@ test('isCommitHash', () => {
});


test('secureUrl', async function (): Promise<void> {
test('secureGitUrl', async function (): Promise<void> {
const reporter = new NoopReporter();

let hasException = false;
try {
await Git.secureUrl('http://fake-fake-fake-fake.com/123.git', '', reporter);
await Git.secureGitUrl(Git.npmUrlToGitUrl('http://fake-fake-fake-fake.com/123.git'), '', reporter);
} catch (e) {
hasException = true;
}
expect(hasException).toEqual(true);

let url = await Git.secureUrl('http://github.com/yarnpkg/yarn.git', '', reporter);
expect(url).toEqual('https://github.com/yarnpkg/yarn.git');
let gitURL = await Git.secureGitUrl(Git.npmUrlToGitUrl('http://github.com/yarnpkg/yarn.git'), '', reporter);
expect(gitURL.repository).toEqual('https://github.com/yarnpkg/yarn.git');

url = await Git.secureUrl('https://github.com/yarnpkg/yarn.git', '', reporter);
expect(url).toEqual('https://github.com/yarnpkg/yarn.git');
gitURL = await Git.secureGitUrl(Git.npmUrlToGitUrl('https://github.com/yarnpkg/yarn.git'), '', reporter);
expect(gitURL.repository).toEqual('https://github.com/yarnpkg/yarn.git');

url = await Git.secureUrl('git://github.com/yarnpkg/yarn.git', '', reporter);
expect(url).toEqual('https://github.com/yarnpkg/yarn.git');
gitURL = await Git.secureGitUrl(Git.npmUrlToGitUrl('git://github.com/yarnpkg/yarn.git'), '', reporter);
expect(gitURL.repository).toEqual('https://github.com/yarnpkg/yarn.git');

},
);
3 changes: 2 additions & 1 deletion src/fetchers/git-fetcher.js
View file
Expand Up @@ -110,7 +110,8 @@ export default class GitFetcher extends BaseFetcher {
const hash = this.hash;
invariant(hash, 'Commit hash required');

const git = new Git(this.config, this.reference, hash);
const gitUrl = Git.npmUrlToGitUrl(this.reference);
const git = new Git(this.config, gitUrl, hash);
await git.init();
await git.clone(this.dest);

Expand Down
22 changes: 6 additions & 16 deletions src/resolvers/exotics/git-resolver.js
View file
Expand Up @@ -11,7 +11,6 @@ import ExoticResolver from './exotic-resolver.js';
import Git from '../../util/git.js';

const urlParse = require('url').parse;
const urlFormat = require('url').format;

// we purposefully omit https and http as those are only valid if they end in the .git extension
const GIT_PROTOCOLS = ['git:', 'git+ssh:', 'git+https:', 'ssh:'];
Expand Down Expand Up @@ -61,14 +60,6 @@ export default class GitResolver extends ExoticResolver {
return false;
}

// This transformUrl util is here to replace colon separators in the pathname
// from private urls. It takes the url parts retrieved using urlFormat and
// returns the associated url. Related to #573, introduced in #2519.
static transformUrl(parts) : string {
const pathname = parts.pathname ? parts.pathname.replace(/^\/:/, '/') : '';
return urlFormat({...parts, pathname});
}

async resolve(forked?: true): Promise<Manifest> {
const {url} = this;

Expand Down Expand Up @@ -102,9 +93,8 @@ export default class GitResolver extends ExoticResolver {

const {config} = this;

const transformedUrl = GitResolver.transformUrl(parts);

const client = new Git(config, transformedUrl, this.hash);
const gitUrl = Git.npmUrlToGitUrl(url);
const client = new Git(config, gitUrl, this.hash);
const commit = await client.init();

async function tryRegistry(registry): Promise<?Manifest> {
Expand All @@ -115,12 +105,12 @@ export default class GitResolver extends ExoticResolver {
return null;
}

const json = await config.readJson(`${transformedUrl}/${filename}`, () => JSON.parse(file));
const json = await config.readJson(`${url}/${filename}`, () => JSON.parse(file));
json._uid = commit;
json._remote = {
resolved: `${transformedUrl}#${commit}`,
resolved: `${url}#${commit}`,
type: 'git',
reference: transformedUrl,
reference: url,
hash: commit,
registry,
};
Expand All @@ -143,6 +133,6 @@ export default class GitResolver extends ExoticResolver {
}
}

throw new MessageError(this.reporter.lang('couldntFindManifestIn', transformedUrl));
throw new MessageError(this.reporter.lang('couldntFindManifestIn', url));
}
}
8 changes: 5 additions & 3 deletions src/resolvers/exotics/hosted-git-resolver.js
View file
Expand Up @@ -104,7 +104,8 @@ export default class HostedGitResolver extends ExoticResolver {
}

async getRefOverHTTP(url: string): Promise<string> {
const client = new Git(this.config, url, this.hash);
const gitUrl = Git.npmUrlToGitUrl(url);
const client = new Git(this.config, gitUrl, this.hash);

let out = await this.config.requestManager.request({
url: `${url}/info/refs?service=git-upload-pack`,
Expand Down Expand Up @@ -211,8 +212,9 @@ export default class HostedGitResolver extends ExoticResolver {
// NOTE: Here we use a different url than when we delegate to the git resolver later on.
// This is because `git archive` requires access over ssh and github only allows that
// if you have write permissions
if (await Git.hasArchiveCapability(sshUrl)) {
const archiveClient = new Git(this.config, sshUrl, this.hash);
const sshGitUrl = Git.npmUrlToGitUrl(sshUrl);
if (await Git.hasArchiveCapability(sshGitUrl)) {
const archiveClient = new Git(this.config, sshGitUrl, this.hash);
const commit = await archiveClient.init();
return await this.fork(GitResolver, true, `${sshUrl}#${commit}`);
}
Expand Down

2 comments on commit 3b8a1ac

@balthazar
Copy link

@balthazar balthazar commented on 3b8a1ac Apr 13, 2017 •
edited

Choose a reason for hiding this comment

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

@pornel Any particular reason the workaround resolving the semicolons in urls that was introduced by #2519 has been removed? Can you explain how is it related to #1796?

@kornelski
Copy link
Contributor Author

@kornelski kornelski commented on 3b8a1ac Apr 14, 2017 •
edited

Choose a reason for hiding this comment

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

@apercu Previously "git+ssh://host:path" was interpreted as git clone "ssh://host/path", which from git perspective changes the protocol from SCP URL to a regular SSH URL, and prevented SCP features from working.

npm (and Yarn after that fix) interprets "git+ssh://host:path" as git clone "host:path", which allows using SCP features such as host aliases and per-host private keys.

Please sign in to comment.