diff --git a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte index 0f056e28b..e9c6aa482 100644 --- a/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte +++ b/desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte @@ -651,18 +651,18 @@ function selectTemplate(t: { name: string; source: string }) { - {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull Request"} + {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull / Merge Request"} Branch Commit - Pull Request + Pull / Merge Request
- {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull request"} + {refType === "branch" ? "Branch" : refType === "commit" ? "Commit" : "Pull / Merge request"} {refValue}
diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts index 8111a8560..8c635e41f 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.test.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.test.ts @@ -59,13 +59,34 @@ describe("buildWorkspaceSource", () => { expect(out.source).toBe("github.com/org/repo@sha256:abc123") }) - it("git: PR ref appends @pull/N/head", () => { + it("git: PR ref appends @pull/N/head for GitHub", () => { const out = buildWorkspaceSource( gitForm({ repoUrl: "github.com/org/repo", refType: "pr", refValue: "42" }), ) expect(out.source).toBe("github.com/org/repo@pull/42/head") }) + it("git: MR ref appends @merge-requests/N/head for GitLab", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "gitlab.com/org/repo", refType: "pr", refValue: "7125" }), + ) + expect(out.source).toBe("gitlab.com/org/repo@merge-requests/7125/head") + }) + + it("git: MR ref detects self-hosted GitLab by hostname", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "git@gitlab.example.com:org/repo.git", refType: "pr", refValue: "7" }), + ) + expect(out.source).toBe("git@gitlab.example.com:org/repo.git@merge-requests/7/head") + }) + + it("git: PR ref uses pull/N/head when only the owner/path says gitlab", () => { + const out = buildWorkspaceSource( + gitForm({ repoUrl: "git@github.com:gitlab-org/repo.git", refType: "pr", refValue: "7" }), + ) + expect(out.source).toBe("git@github.com:gitlab-org/repo.git@pull/7/head") + }) + it("git: subpath appends @subpath: after ref", () => { const out = buildWorkspaceSource( gitForm({ diff --git a/desktop/src/renderer/src/lib/utils/workspace-source.ts b/desktop/src/renderer/src/lib/utils/workspace-source.ts index 11845fcf3..859ce3da0 100644 --- a/desktop/src/renderer/src/lib/utils/workspace-source.ts +++ b/desktop/src/renderer/src/lib/utils/workspace-source.ts @@ -34,7 +34,31 @@ export interface WorkspaceSourceResult { prebuildRepository?: string } -function refSuffix(refType: GitRefType, refValue: string): string { +// hostFromUrl extracts the hostname from SSH (git@host:path) and URL +// (scheme://[user@]host/path) forms. +function hostFromUrl(repoUrl: string): string { + let s = repoUrl + const scheme = s.indexOf("://") + if (scheme !== -1) s = s.slice(scheme + 3) + const at = s.indexOf("@") + if (at !== -1) s = s.slice(at + 1) + const sep = s.search(/[/:]/) + if (sep !== -1) s = s.slice(0, sep) + return s +} + +// GitLab exposes merge requests at merge-requests/N/head; every other host +// uses pull/N/head. +function prRefspec(repoUrl: string, number: string): string { + const segment = /gitlab/i.test(hostFromUrl(repoUrl)) ? "merge-requests" : "pull" + return `@${segment}/${number}/head` +} + +function refSuffix( + refType: GitRefType, + refValue: string, + repoUrl: string, +): string { const value = refValue.trim() if (!value) return "" switch (refType) { @@ -43,7 +67,7 @@ function refSuffix(refType: GitRefType, refValue: string): string { case "commit": return `@sha256:${value}` case "pr": - return `@pull/${value}/head` + return prRefspec(repoUrl, value) } } @@ -72,7 +96,8 @@ export function buildWorkspaceSource( } } - const source = `${form.repoUrl.trim()}${refSuffix(form.refType, form.refValue)}${subPathSuffix(form.subPath)}` + const repoUrl = form.repoUrl.trim() + const source = `${repoUrl}${refSuffix(form.refType, form.refValue, repoUrl)}${subPathSuffix(form.subPath)}` return { source, diff --git a/pkg/git/git.go b/pkg/git/git.go index 1193dbd98..2693a684f 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -11,13 +11,13 @@ import ( const ( CommitDelimiter string = "@sha256:" - PullRequestReference string = "pull/([0-9]+)/head" + PullRequestReference string = "(?:pull|merge-requests)/([0-9]+)/head" SubPathDelimiter string = "@subpath:" repoBaseRegEx string = `((?:(?:https?|git|ssh|file):\/\/)?\/?(?:[^@\/\n]+@)?` + `(?:[^:\/\n]+)(?:[:\/][^\/\n]+)+(?:\.git)?)` ) -// WARN: Make sure this matches the regex in /desktop/src/views/Workspaces/CreateWorkspace/CreateWorkspaceInput.tsx! +// WARN: Keep these in sync with the ref parsing in UI. var ( branchRegEx = regexp.MustCompile(`^` + repoBaseRegEx + `@([a-zA-Z0-9\./\-\_]+)$`) commitRegEx = regexp.MustCompile( @@ -36,7 +36,8 @@ var recognizedSchemes = []string{"ssh://", "git@", "http://", "https://", "file: // NormalizeRepository parses a repository reference into its structured parts. // Accepts plain URLs, the "git:" workspace-source scheme, and references -// suffixed with @branch, @subpath:, @sha256:, or @pull/N/head. +// suffixed with @branch, @subpath:, @sha256:, or a pull/merge +// request ref (@pull/N/head or @merge-requests/N/head). // Bare host[/path] inputs are upgraded to https://. func NormalizeRepository(str string) *GitInfo { str = canonicalizeURL(str) @@ -87,14 +88,23 @@ func PingRepository(str string, extraEnv []string) bool { return At("", WithEnv(extraEnv)).LsRemote(timeoutCtx, str) == nil } +// GetBranchNameForPR returns the local branch name for a request ref, using the +// provider convention encoded in the ref (PR for GitHub, MR for GitLab). func GetBranchNameForPR(ref string) string { - regex := regexp.MustCompile(PullRequestReference) - return regex.ReplaceAllString(ref, "PR${1}") + number := prNumber(ref) + if number == "" { + return ref + } + return hostForRef(ref).BranchName(number) } +// GetIDForPR returns the lowercased request identifier used in workspace IDs. func GetIDForPR(ref string) string { - regex := regexp.MustCompile(PullRequestReference) - return regex.ReplaceAllString(ref, "pr${1}") + number := prNumber(ref) + if number == "" { + return ref + } + return strings.ToLower(hostForRef(ref).BranchName(number)) } // GitInfo is the parsed form of a repository reference. Branch, Commit, PR, diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index db73297ff..bd8dac642 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -151,6 +151,14 @@ var normalizeRepositoryCases = []testCaseNormalizeRepository{ expectedCommit: "", expectedSubpath: "", }, + { + in: "git@gitlab.com:h3upperbounds/data/data-team.git@merge-requests/7125/head", + expectedRepo: "git@gitlab.com:h3upperbounds/data/data-team.git", + expectedPRReference: "merge-requests/7125/head", + expectedBranch: "", + expectedCommit: "", + expectedSubpath: "", + }, { in: "github.com/devsy-org/devsy-without-protocol-with-slash.git@subpath:/test/path", expectedRepo: repoDevsyNoProtoSlash, @@ -260,6 +268,10 @@ func TestGetBranchNameForPRReference(t *testing.T) { in: testPRRef, expectedBranch: "PR996", }, + { + in: "merge-requests/7125/head", + expectedBranch: "MR7125", + }, { in: "pull/abc/head", expectedBranch: "pull/abc/head", diff --git a/pkg/git/host.go b/pkg/git/host.go new file mode 100644 index 000000000..4f1704924 --- /dev/null +++ b/pkg/git/host.go @@ -0,0 +1,101 @@ +package git + +import ( + "regexp" + "strings" +) + +var prNumberRegEx = regexp.MustCompile(`(?:pull|merge-requests)/([0-9]+)/head`) + +const ( + hostNameGitHub = "github" + hostNameGitLab = "gitlab" +) + +// Host is a git provider's convention for referencing pull/merge requests: +// GitHub exposes them at pull/N/head, GitLab at merge-requests/N/head. +type Host struct { + Name string + refPrefix string + branchAbbr string + hostHint string // substring identifying the provider in a repository URL +} + +var ( + HostGitHub = Host{ + Name: hostNameGitHub, + refPrefix: "pull", + branchAbbr: "PR", + hostHint: hostNameGitHub, + } + HostGitLab = Host{ + Name: hostNameGitLab, + refPrefix: "merge-requests", + branchAbbr: "MR", + hostHint: hostNameGitLab, + } + + knownHosts = []Host{HostGitHub, HostGitLab} +) + +func (h Host) Refspec(number string) string { + return h.refPrefix + "/" + number + "/head" +} + +func (h Host) BranchName(number string) string { + return h.branchAbbr + number +} + +// DetectHost picks the provider from a repository URL, defaulting to GitHub. +func DetectHost(repoURL string) Host { + host := strings.ToLower(hostFromURL(repoURL)) + for _, h := range knownHosts { + if strings.Contains(host, h.hostHint) { + return h + } + } + return HostGitHub +} + +// hostFromURL extracts the hostname from SSH (git@host:path) and URL +// (scheme://[user@]host/path) forms, returning "" when none is present. +func hostFromURL(repoURL string) string { + s := repoURL + if i := strings.Index(s, "://"); i != -1 { + s = s[i+3:] + } + if i := strings.Index(s, "@"); i != -1 { + s = s[i+1:] + } + if i := strings.IndexAny(s, "/:"); i != -1 { + s = s[:i] + } + return s +} + +// hostForRef infers the provider from the ref itself. +func hostForRef(ref string) Host { + if strings.Contains(ref, HostGitLab.refPrefix+"/") { + return HostGitLab + } + return HostGitHub +} + +func prNumber(ref string) string { + if m := prNumberRegEx.FindStringSubmatch(ref); m != nil { + return m[1] + } + return "" +} + +// prCandidates orders the hosts to try for a checkout. +func prCandidates(repoURL string) []Host { + primary := DetectHost(repoURL) + candidates := []Host{primary} + for _, h := range knownHosts { + if h.Name != primary.Name { + candidates = append(candidates, h) + } + } + return candidates +} diff --git a/pkg/git/host_test.go b/pkg/git/host_test.go new file mode 100644 index 000000000..3e9774adb --- /dev/null +++ b/pkg/git/host_test.go @@ -0,0 +1,50 @@ +package git + +import ( + "testing" + + "gotest.tools/assert" + "gotest.tools/assert/cmp" +) + +func TestDetectHost(t *testing.T) { + cases := []struct { + url string + want string + }{ + {"git@github.com:org/repo.git", hostNameGitHub}, + {"https://github.com/org/repo.git", hostNameGitHub}, + {"git@gitlab.com:org/repo.git", hostNameGitLab}, + {"https://gitlab.example.com/org/repo.git", hostNameGitLab}, + // Only the hostname decides: a "gitlab" owner/path on github.com is GitHub. + {"git@github.com:gitlab-org/repo.git", hostNameGitHub}, + {"https://github.com/org/gitlab-mirror.git", hostNameGitHub}, + { + "https://git.internal.example/org/repo.git", + hostNameGitHub, + }, // unknown host defaults to GitHub + } + for _, c := range cases { + assert.Check(t, cmp.Equal(c.want, DetectHost(c.url).Name), "url=%s", c.url) + } +} + +func TestHostRefspecAndBranch(t *testing.T) { + assert.Equal(t, "pull/42/head", HostGitHub.Refspec("42")) + assert.Equal(t, "PR42", HostGitHub.BranchName("42")) + assert.Equal(t, "merge-requests/42/head", HostGitLab.Refspec("42")) + assert.Equal(t, "MR42", HostGitLab.BranchName("42")) +} + +func TestPRNumber(t *testing.T) { + assert.Equal(t, "996", prNumber("pull/996/head")) + assert.Equal(t, "7125", prNumber("merge-requests/7125/head")) + assert.Equal(t, "", prNumber("refs/heads/main")) +} + +func TestPRCandidatesOrder(t *testing.T) { + got := prCandidates("git@gitlab.com:org/repo.git") + assert.Equal(t, 2, len(got)) + assert.Equal(t, hostNameGitLab, got[0].Name) // detected host first + assert.Equal(t, hostNameGitHub, got[1].Name) // fallback +} diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 133c25dff..20d18838a 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "fmt" "strings" @@ -79,15 +80,46 @@ func (r *Repo) Switch(ctx context.Context, branch string) error { return nil } -// CheckoutPR fetches the given pull request refspec and switches to a local branch for it. -func (r *Repo) CheckoutPR(ctx context.Context, prRef string) error { - log.Debugf("fetching pull request: %s", prRef) +// CheckoutPR fetches a pull/merge request into a local branch and switches to +// it. The request number is taken from prRef; the remote refspec is resolved +// against repoURL's hosting provider, falling back to the other known +// conventions when the detected one has no such ref (e.g. self-hosted GitLab on +// a custom domain that URL detection can't recognize). +func (r *Repo) CheckoutPR(ctx context.Context, repoURL, prRef string) error { + number := prNumber(prRef) + if number == "" { + return fmt.Errorf("not a pull/merge request reference: %q", prRef) + } - prBranch := GetBranchNameForPR(prRef) - if err := r.Fetch(ctx, prRef+":"+prBranch); err != nil { - return err + var lastErr error + for _, host := range prCandidates(repoURL) { + refspec := host.Refspec(number) + prBranch := host.BranchName(number) + log.Debugf("fetching %s request: %s", host.Name, refspec) + + err := r.Fetch(ctx, refspec+":"+prBranch) + if err == nil { + return r.Switch(ctx, prBranch) + } + if !isMissingRefError(err) { + return err + } + lastErr = err + } + return lastErr +} + +// isMissingRefError reports whether err is git failing to find the requested +// remote ref, as opposed to an auth, network, or cancellation failure. +func isMissingRefError(err error) bool { + var cmdErr *CommandError + if !errors.As(err, &cmdErr) { + return false } - return r.Switch(ctx, prBranch) + msg := strings.ToLower(cmdErr.Stderr) + return strings.Contains(msg, "couldn't find remote ref") || + strings.Contains(msg, "no such ref") || + strings.Contains(msg, "not found in upstream") } // Reset moves HEAD to commit using the given mode. @@ -140,7 +172,7 @@ func (r *Repo) CloneFromInfo( switch { case gitInfo.PR != "": - if err := r.CheckoutPR(ctx, gitInfo.PR); err != nil { + if err := r.CheckoutPR(ctx, gitInfo.Repository, gitInfo.PR); err != nil { return err } case gitInfo.Commit != "": diff --git a/pkg/git/repo_test.go b/pkg/git/repo_test.go index 8abad6c15..39a6d5adb 100644 --- a/pkg/git/repo_test.go +++ b/pkg/git/repo_test.go @@ -2,6 +2,7 @@ package git import ( "context" + "errors" "testing" "gotest.tools/assert" @@ -11,11 +12,16 @@ import ( // Shared literals used across Repo tests. const ( testRepoURL = "git@host:org/repo.git" + testGitHubURL = "https://github.com/org/repo.git" + testGitLabURL = "git@gitlab.com:org/repo.git" testPRRef = "pull/996/head" testPRLocal = "PR996" testPRRefSpec = testPRRef + ":" + testPRLocal + testMRLocal = "MR7" + testMRRefSpec = "merge-requests/7/head:" + testMRLocal testCommit = "abc123" subFetch = "fetch" + subSwitch = "switch" originRemote = "origin" testTarget = "/tmp/target" ) @@ -23,13 +29,22 @@ const ( // fakeRunner records the invocations it receives and returns canned output, // letting git operations be tested without a real repository. type fakeRunner struct { - calls []RunOptions - stdout []byte - err error + calls []RunOptions + stdout []byte + err error + errUntil int } func (f *fakeRunner) Run(_ context.Context, opts RunOptions) (RunResult, error) { f.calls = append(f.calls, opts) + if len(f.calls) <= f.errUntil { + return RunResult{}, &CommandError{ + Args: opts.Args, + ExitCode: 128, + Stderr: "fatal: couldn't find remote ref " + opts.Args[len(opts.Args)-1], + Err: errors.New("exit status 128"), + } + } return RunResult{Stdout: f.stdout}, f.err } @@ -61,10 +76,60 @@ func TestRepoCheckoutPR(t *testing.T) { fake := &fakeRunner{} repo := At("/tmp/repo", WithRunner(fake)) - assert.NilError(t, repo.CheckoutPR(context.Background(), testPRRef)) - // Expect a fetch of the PR ref into a local branch, then a switch to it. + err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) + assert.NilError(t, err) assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[0].Args) - assert.DeepEqual(t, []string{"switch", testPRLocal}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subSwitch, testPRLocal}, fake.calls[1].Args) +} + +func TestRepoCheckoutPRGitLab(t *testing.T) { + fake := &fakeRunner{} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), testGitLabURL, "merge-requests/7/head") + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) +} + +func TestRepoCheckoutPRGitLabFromGitHubStyleRef(t *testing.T) { + fake := &fakeRunner{} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), testGitLabURL, "pull/7/head") + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[1].Args) +} + +func TestRepoCheckoutPRFallback(t *testing.T) { + fake := &fakeRunner{errUntil: 1} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR( + context.Background(), + "https://git.internal.example/org/repo.git", + "pull/7/head", + ) + assert.NilError(t, err) + assert.DeepEqual(t, []string{subFetch, originRemote, "pull/7/head:PR7"}, fake.calls[0].Args) + assert.DeepEqual(t, []string{subFetch, originRemote, testMRRefSpec}, fake.calls[1].Args) + assert.DeepEqual(t, []string{subSwitch, testMRLocal}, fake.calls[2].Args) +} + +func TestRepoCheckoutPRNonRefErrorNoFallback(t *testing.T) { + authErr := &CommandError{ + Args: []string{subFetch}, + ExitCode: 128, + Stderr: "fatal: Authentication failed for 'https://host/org/repo.git'", + Err: errors.New("exit status 128"), + } + fake := &fakeRunner{err: authErr} + repo := At("/tmp/repo", WithRunner(fake)) + + err := repo.CheckoutPR(context.Background(), testGitHubURL, testPRRef) + assert.ErrorContains(t, err, "Authentication failed") + assert.Equal(t, 1, len(fake.calls)) } func TestRepoLsRemote(t *testing.T) { @@ -160,7 +225,7 @@ func TestRepoCloneFromInfoPR(t *testing.T) { assert.NilError(t, repo.CloneFromInfo(context.Background(), info, "")) assert.Equal(t, subClone, fake.calls[0].Args[0]) assert.DeepEqual(t, []string{subFetch, originRemote, testPRRefSpec}, fake.calls[1].Args) - assert.DeepEqual(t, []string{"switch", testPRLocal}, fake.calls[2].Args) + assert.DeepEqual(t, []string{subSwitch, testPRLocal}, fake.calls[2].Args) } func TestRepoCloneFromInfoHelper(t *testing.T) {