Ignore links to other branches

[jablko]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• If applicable, I’ve added docs and tests

Description of changes

Append GitHost.committish -> urlConfig.prefix, thereby only matching links to the configured branch and ignoring others.

Fixes DefinitelyTyped/DefinitelyTyped#58598

In this instance ☝️, the falsely-failing link was https://github.com/DefinitelyTyped/DefinitelyTyped/blob/1253faabf5e0d2c5470db6ea87795d7f96fef7e2/types/history/v2/tsconfig.json (it's valid).

The linked file exists in that commit but not in the main branch.

Before this PR, urlConfig.prefix is /DefinitelyTyped/DefinitelyTyped/blob/, which matches every branch/commit. After, it's /DefinitelyTyped/DefinitelyTyped/blob/master/, which only matches the configured branch.

[wooorm]

Hey!

This looks rather verbose, and it’s unclear to me what’s going on.

I’m not sure about the original request on DT. It’s not reasonable to expect this project to look across branches, but given that branches are supposed to be merged, I think it’s fine to expect that files also exist on different branches?
I may be open to this change, but not sure.

Before this PR, urlConfig.prefix is /DefinitelyTyped/DefinitelyTyped/blob/, which matches every branch/commit.

Yeah. That behavior is kinda intentional:

remark-validate-links/lib/find/find-references.js

Line 254 in f259b30

// Currently, we’re ignoring this and just not supporting branches.

.

[wooorm]

this looks complex, compared to the previous code, what’s browse returning, what happens new URL().pathname?

[jablko]

hostedGitInfo.fromUrl('https://github.com/DefinitelyTyped/DefinitelyTyped.git').browse({treepath: 'blob'})
// -> 'https://github.com/DefinitelyTyped/DefinitelyTyped'

hostedGitInfo.fromUrl('https://github.com/DefinitelyTyped/DefinitelyTyped.git#master').browse({treepath: 'blob'})
// -> 'https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master'

https://github.com/npm/hosted-git-info/blob/702b7df467f2fd4bf46395134c62534bb966ca7b/git-host-info.js#L8

so hosted-git-info omits the "blob" segment from branch-less URLs.

[wooorm]

again? viewPaths[info.type] was just used?

[wooorm]

This is way too magic? It doesn’t seem “educated”? Who says branches are 5+ characters?

[jablko]

What I was wanting to get at wasn't the characters:

'/DefinitelyTyped/DefinitelyTyped/blob/'.split('/')`
// -> [ '', 'DefinitelyTyped', 'DefinitelyTyped', 'blob', '' ]

so .length <= 5. Currently this is true of every hosted-git-info browsetemplate/pathtemplate: https://github.com/npm/hosted-git-info/blob/702b7df467f2fd4bf46395134c62534bb966ca7b/git-host-info.js#L8

[wooorm]

this is a rather verbose test. What’s okay, and should (not) be okay? What’s broken and should (not) be broken?

[wooorm]

I’m still not 100% on whether this is a good idea.

But, particularly, I find it unclear what the implications are, because:
a) to enable this feature, a “commitish” has to explicitly be passed, and there’s only one test for that.
b) there are no docs

What happens if a SHA is passed instead of a branch?
What if the branch was automatically detected (like the repo)?

What about common workflows:

• I’m on the main branch and everything’s tested
• I check out my-feature-branch to do some work
• I refactor a bunch of things, but none of the local links are checked, because none of them go to the branch my-feature-branch?
• The branch is merged, tons of errors occur — seems quite unfortunate?

[jablko]

I see what you mean: I've now added docs covering what happens when the repository option does/doesn't have a branch, and what happens when the repo is automatically detected.

In that workflow ☝️:

• If the repository option has a branch (e.g. https://github.com/remarkjs/remark-validate-links.git#main) the plugin checks links to the main branch, regardless of the current branch (my-feature-branch or main).

  Without this PR it doesn't check anything, because currently urlConfig.prefix is /remarkjs/remark-validate-links#main/blob/ which doesn't match any links.

• If it doesn't have a branch the plugin checks links to all branches as if they pointed to the current branch (same behavior as today).

• The same happens when the repo is automatically detected (same as today).

So in every case, local links are checked.

[jablko]

In the latest commit I split up the test into individual test cases.