Feature/50 fix gitlab pathmatch 3 0 0

[mikemimik]

Closes #50

Related to #51

[isaacs]

Could this be tested with a url where the host matches, but the path isn't a match? Like https://github.com/isaacs?

Possibly I'm misreading this bit, and it is actually just a safety guard that never hits. But in that case, it'd be preferable to make that explicit.

/*istanbul ignore if */
if (matched[1] === null || matched[1] === undefined) {
  throw new Error('this should be impossible')
}

[isaacs]

Generally looks good to me. I'm usually not a huge fan of programmatically generated tests, but in this case, I can see how these would get hella repetitive (and already had some generated tests anyway).

[lpinca]

I think this will also incorrectly match URLs like https://gitlab.com/user/foo/bar/baz/repository/archive.tar.gz

[isaacs]

It looks like these two suffixes also need to be excluded:

\/repository\/archive.tar.gz(\?ref=...)?$
\/repository\/[^\/]\/archive.tar.gz$

[isaacs]

Seems like this pathmatch change should do the right thing:

diff --git a/git-host-info.js b/git-host-info.js
index 4dec801..04d5f63 100644
--- a/git-host-info.js
+++ b/git-host-info.js
@@ -25,7 +25,7 @@ var gitHosts = module.exports = {
     'bugstemplate': 'https://{domain}/{user}/{project}/issues',
     'httpstemplate': 'git+https://{auth@}{domain}/{user}/{projectPath}.git{#committish}',
     'tarballtemplate': 'https://{domain}/{user}/{project}/repository/archive.tar.gz?ref={committish}',
-    'pathmatch': /^[/]([^/]+)[/](?!.*\/-\/)(.+?)(?:[.]git|[/])?$/
+    'pathmatch': /^[/]([^/]+)[/](?!.*(\/-\/|\/repository\/archive\.tar\.gz|\/repository\/[^\/]+\/archive.tar.gz))(.+)(?:[.]git|[/])?$/
   },
   gist: {
     'protocols': [ 'git', 'git+ssh', 'git+https', 'ssh', 'https' ],

[lpinca]

Not sure if allowed by GitLab but that regex will incorrectly match something like https://gitlab.com/user/repository/archive.tar.gz/projname. Anyway it is probably an edge case not worth fixing.

[isaacs]

Can a repo be just a single path part on GitLab? I thought that they were always at least 2.

[lpinca]

I think so if you mean something like https://gitlab.com/user/group/subgroup/repo.

[isaacs]

Right, so you could have a user named user, then a group named repository, then a subgroup named archive.tar.gz and a project named foo. I think that's possibly valid, unless GitLab doesn't allow repository or archive.tar.gz as group names (which would be a good idea!)

But the requirement here isn't to match every and only valid gitlab URLs. Just to match all the ones that are valid and likely to be used. If we treat an invalid gitlab url as valid, and then it blows up, that's fine, because it was never going to work anyway. As long as we're not trying to download a valid tarball url but using git, making it fail.

[lpinca]

As long as we're not trying to download a valid tarball url but using git, making it fail.

Yes that's my point. https://gitlab.com/user/repository/archive.tar.gz/foo would be treated as tarball url instead of a git one no?

But the requirement here isn't to match every and only valid gitlab URLs. Just to match all the ones that are valid and likely to be used.

Agreed.

[isaacs]

Yes that's my point. https://gitlab.com/user/repository/archive.tar.gz/foo would be treated as tarball url instead of a git one no?

Oh, I see. Hm. This seems to do the trick:

/^[/]([^/]+)[/](?!.*(\/-\/|\/repository\/archive\.tar\.gz\?=.*|\/repository\/[^\/]+\/archive.tar.gz$))(.+)(?:[.]git|[/])?$/

Wonder if it might be too limiting to have it anchored at the end, though.