Fix: support valid remote and branch names #147
Conversation
felixfbecker
left a comment
felixfbecker
left a comment
There was a problem hiding this comment.
Awesome that you refactored and added tests! I didn't look at the logic in depth, I'll defer to @sourcegraph/extensibility
src/git.ts
Outdated
| branch = stdout.slice(remoteName.length + 1) // remove '/' from start of branch | ||
| } | ||
| } catch { | ||
| // noop. upstream may not be set |
There was a problem hiding this comment.
Can you narrow down this error a bit so we don't blanket-swallow all errors, including e.g. the git executable not being found? Is there a certain exit code or stderr output?
There was a problem hiding this comment.
Git doesnt seem to return specific error codes.
Some examples:
If stderr starts with fatal: no upstream configured for branch we could swallow, else rethrow.
However, errors like git executable not being found or git repo not existing would be thrown when we execute git.remotes or git.branches (both are executed before git.upstreamAndBranch)
| // Used to determine which part of upstreamAndBranch is the remote name, or as fallback if no upstream is set | ||
| const remotes = await git.remotes(repoDirectory) | ||
| const branch = await git.branch(repoDirectory) | ||
|
|
||
| try { | ||
| const upstreamAndBranch = await git.upstreamAndBranch(repoDirectory) | ||
| // Subtract $BRANCH_NAME from $UPSTREAM_REMOTE/$BRANCH_NAME. | ||
| // We can't just split on the delineating `/`, since refnames can include `/`: | ||
| // https://sourcegraph.com/github.com/git/git@454cb6bd52a4de614a3633e4f547af03d5c3b640/-/blob/refs.c#L52-67 | ||
|
|
||
| // Example: | ||
| // stdout: remote/two/tj/feature | ||
| // remoteName: remote/two, branch: tj/feature | ||
|
|
||
| const branchPosition = upstreamAndBranch.lastIndexOf(branch) | ||
| const maybeRemote = upstreamAndBranch.slice(0, branchPosition - 1) | ||
| if (maybeRemote) { | ||
| remoteName = maybeRemote |
There was a problem hiding this comment.
Core logic to be reviewed
|
Merging as we reviewed in hack time |
|
🎉 This PR is included in version 1.2.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
tjkandala
changed the title
Closes #145
Background
Existing implementation split upstream and branch by
/, but refname components can contain/. Instead of splitting on/, subtract the upstream and branch by branch name (two separate git command invocations).Progress