Omit ports in constructed URL

[alexanderchiu]

Hi!

Firstly want to say thanks to all the maintainers for this project. I was very excited when I came across it.

I ran into the same issue as described in #193 and I saw the stagnant related PR #259.

I thought I would add some tests ( as requested), and added a fix for the omitting ports in constructed URLs with fewer regex changes.

I don't write much JS so any style based feedback would be much appreciated!

[salemhilal]

Hey, thank you for this PR (especially for the master -> main rename in the tests). We are prepping a minor version release at the moment, but once that's settled we can give this some more attention.

[alexanderchiu]

Sounds good - thanks @salemhilal!

[salemhilal]

Should this repo URL contain the port? i.e. would we expect this to be "//bitbucket.org:7999/YourOrganization/RepoOne/src/main/test.txt" in all cases?

[alexanderchiu]

Thanks for the reviewing this.

My actual goal is to get Hound working with BitBucket Server. From https://confluence.atlassian.com/bitbucketserver074/enabling-ssh-access-to-git-repositories-in-bitbucket-server-1019386434.html#EnablingSSHaccesstoGitrepositoriesinBitbucketServer-SSHbaseURL

Resulting SSH URL for a repo: ssh://[email protected]:7999/<projectname>/<reponame>.git
Bitbucket Server base URL: https://bitbucket.atlassian.com

In this case the port is required in the config.json when specifying the ssh url, but the port needs to be dropped when resolving the url when clicking through to a file after search.

I guess the best approach would be to allow specifying a {port} in base-url to allow the user to pick whether they want the port or not.

As an aside, I thought this was the only change needed to get it working with BitBucket Server, but after finally getting around to building and running my branch I've noticed that the required URL is in the form:

https://bitbucket.atlassian.com/projects/<projectname>/repos/<reponame>/browse 

which would require further additions to the UrlToRepo method. This diversion may not be what you want, so won't open a PR unless you think it's worthwhile.

[salemhilal]

Sorry for the late response here! I think that if the port is dropped to support bitbucket URLs, but bitbucket URLs are themselves also kinda weirdly formatted, it might be better to leave the port in by default, and then to make a special exemption for bitbucket / atlassian-hosted repositories. Does that work for you? I'd also be open to a PR to handle Bitbucket URLs, either as part of this PR or in a separate one.

[alexanderchiu]

No stress. Thanks for getting back to me.

leave the port in by default, and then to make a special exemption for bitbucket / atlassian-hosted repositories.

Sounds good to me.

I'd also be open to a PR to handle Bitbucket URLs, either as part of this PR or in a separate one.

Ok cool thanks - I'm not really sure where the best place to add that logic would be. I'll update this PR so that the ports are not ommited, and then you can add the exemption for on prem bitbucket repositories (or give me some guidance on what you have in mind and I can do it).

[salemhilal]

For sure. I think in that function itself, there's already some edge cases being handled (like this one for Github wikis. I wouldn't be mad at exceptional handling of some sort for Bitbucket in there as well. If that method gets too unruly, it shouldn't be too difficult to refactor later, too.

[alexanderchiu]

Added full support for the Bitbucket server style repos by adding expansion vars. Tested it against our deployed bitbucket and click through to source is working now. Everything else should have remained backwards compatible.

[salemhilal]

I have one implementation-specific question (I'm not totally familiar with how we would expect ports to be used here), but otherwise this PR looks good to me! Thank you for contributing this fix.

[Crocmagnon]

Hi there 👋🏻
I'm seeing mentions of bitbucket, will this fix work for gitlab for example ?

Our clone urls look something like this:

ssh://[email protected]:4222/group/project.git

The urls generated by hound on the search results look like this:

https://git.example.com/4222/group/project/blob/master/folder/file.extension

We expect the /4222 to be removed.

[salemhilal]

Hey @Crocmagnon, this PR is exactly about that problem (see this issue for more). I haven't had a chance to put fresh eyes on this PR, but I'd like to do that shortly (life's crazy, etc.)

[alexanderchiu]

I tested locally with the proposed changes and the following passes:

  test("gitlab", () => {
      const repo = {
          url: "ssh://[email protected]:4222/group/project.git",
          "url-pattern":
          {
              "base-url" : "{hostname}/{project}/{repo}/blob/{rev}/{path}{anchor}",
              "anchor" : "#{line}",
          }
      };
      const path = "folder/file.extension"
      const line = null
      const rev = "master"
      expect(UrlToRepo(repo, path, line, rev)).toBe(
          "//git.example.com/group/project/blob/master/folder/file.extension"
      );
  });

[salemhilal]

@alexanderchiu if you want to toss that test into this PR that works for me. Otherwise, this looks good to merge!

[alexanderchiu]

That functionality is already covered by an existing test so it's probably not necessary. I think you can merge when convenient. Thanks for the reviews @salemhilal!

[salemhilal]

Thank you for this PR (and your patience) @alexanderchiu!