From f9b7ca86286e0525afcc292ed10bb6ae30b88821 Mon Sep 17 00:00:00 2001 From: David Faure Date: Tue, 7 Jul 2026 13:11:04 +0200 Subject: [PATCH 1/2] fix(trace): keep stderr for successful commands Servers print MR/review links to stderr on a successful push, but the trace only recorded stderr for failed commands, so those links were dropped from both the summary and the log file. --- src/trace.rs | 6 +++--- src/trace_test.rs | 24 +++++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/trace.rs b/src/trace.rs index d3e68d1..d3e9488 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -241,7 +241,7 @@ fn format_log_suffix(logger: &LoomLogger) -> String { out.push('\n'); } } - if !entry.success && !entry.stderr.is_empty() { + if !entry.stderr.is_empty() { out.push_str(" [stderr]\n"); for line in entry.stderr.lines() { out.push_str(line); @@ -284,8 +284,8 @@ fn format_log(logger: &LoomLogger) -> String { } } - // Stderr on failure - if !entry.success && !entry.stderr.is_empty() { + // Stderr (on success too — servers print MR/review links there) + if !entry.stderr.is_empty() { out.push_str(" [stderr]\n"); for line in entry.stderr.lines() { out.push_str(line); diff --git a/src/trace_test.rs b/src/trace_test.rs index cf5daad..3673d45 100644 --- a/src/trace_test.rs +++ b/src/trace_test.rs @@ -72,7 +72,7 @@ fn failed_command_includes_stderr() { } #[test] -fn successful_command_excludes_stderr_section() { +fn successful_command_without_stderr_has_no_section() { let (_dir, git_dir) = setup_git_dir(); super::init(&git_dir, "git loom commit"); @@ -84,6 +84,28 @@ fn successful_command_excludes_stderr_section() { assert!(!content.contains("[stderr]")); } +#[test] +fn successful_command_includes_stderr() { + // Servers print MR/review links to stderr on a successful push, so the + // trace must keep stderr even when the command succeeds. + let (_dir, git_dir) = setup_git_dir(); + + super::init(&git_dir, "git loom push feature-a"); + super::log_command( + "git", + "push -u origin feature-a", + 1200, + true, + "remote: To create a merge request, visit:\nremote: https://gitlab.com/g/p/-/merge_requests/new", + ); + let path = super::finalize().unwrap(); + + let content = fs::read_to_string(&path).unwrap(); + assert!(!content.contains("FAILED")); + assert!(content.contains("[stderr]")); + assert!(content.contains("merge_requests/new")); +} + #[test] fn annotations_appear_in_log() { let (_dir, git_dir) = setup_git_dir(); From 78f5ceaf9202c4b868ab326afb962d24b682d87b Mon Sep 17 00:00:00 2001 From: David Faure Date: Tue, 7 Jul 2026 13:11:11 +0200 Subject: [PATCH 2/2] feat(push): support GitLab and surface server MR/PR links Plain pushes now display any remote: URL lines from the push output, so GitLab's merge-request creation link is shown even when the remote type is not detected (e.g. self-hosted invent.kde.org). Add a GitLab remote type (config gitlab or a gitlab URL) that pushes with merge_request.create push options so the server creates or links the MR directly. --- docs/src/commands/push.md | 23 +++++-- specs/011-push.md | 37 ++++++++-- src/push.rs | 139 ++++++++++++++++++++++++++++---------- src/push_test.rs | 64 ++++++++++++++++++ 4 files changed, 219 insertions(+), 44 deletions(-) diff --git a/docs/src/commands/push.md b/docs/src/commands/push.md index bad8850..ac43e29 100644 --- a/docs/src/commands/push.md +++ b/docs/src/commands/push.md @@ -24,11 +24,14 @@ git loom push [branch] [--no-pr] Detection priority (first match wins): -1. **Explicit config** — `git config loom.remote-type` set to `github`, `azure`, or `gerrit` +1. **Explicit config** — `git config loom.remote-type` set to `github`, `gitlab`, `azure`, or `gerrit` 2. **URL heuristics** — remote URL contains `github.com` → GitHub -3. **URL heuristics** — remote URL contains `dev.azure.com` → Azure DevOps -4. **Hook inspection** — `.git/hooks/commit-msg` contains "gerrit" → Gerrit -5. **Fallback** — Plain Git +3. **URL heuristics** — remote URL contains `gitlab` → GitLab +4. **URL heuristics** — remote URL contains `dev.azure.com` → Azure DevOps +5. **Hook inspection** — `.git/hooks/commit-msg` contains "gerrit" → Gerrit +6. **Fallback** — Plain Git + +Self-hosted GitLab whose hostname does not contain `gitlab` (e.g. `invent.kde.org`) is not auto-detected — set `git config loom.remote-type gitlab`. Even without detection, a plain push still surfaces the MR link the server prints. ## Push Remote Selection @@ -54,6 +57,8 @@ git push --force-with-lease --force-if-includes -u Uses `--force-with-lease` because woven branches are frequently rebased. `--force-if-includes` adds extra safety. +Any `remote:` lines containing an `http(s)` URL are shown below the success message, so the MR/PR creation link that servers like GitLab print on push is visible even when the remote type was not detected. + ### GitHub Pushes the branch with `--force-with-lease`, then checks whether a PR already exists for the branch: @@ -67,6 +72,15 @@ In a **fork workflow** (tracking `upstream/main`), pushes go to `origin` (your f If the branch being pushed is the upstream target branch itself, PR creation is skipped. +### GitLab + +```bash +git push --force-with-lease --force-if-includes \ + -o merge_request.create -o merge_request.target= -u +``` + +Uses GitLab [push options](https://docs.gitlab.com/ee/user/project/push_options.html) so the server creates a merge request (or points to the existing one) during the push. The MR URL GitLab prints is shown below the success message. No extra CLI tool is required. If the branch being pushed is the upstream target branch itself, the MR push options are skipped. + ### Azure DevOps Pushes the branch with `--force-with-lease`, then checks whether a PR already exists for the branch: @@ -100,6 +114,7 @@ Use `--no-pr` when you want to push a branch to the remote without triggering PR |-------------|-------------------| | Plain | Same as normal (force-with-lease push) | | GitHub | Skips `gh pr create` | +| GitLab | Plain push without `merge_request.create` push options | | Azure DevOps | Skips `az repos pr create` | | Gerrit | Plain push to branch ref instead of `refs/for/` (see below) | diff --git a/specs/011-push.md b/specs/011-push.md index 8574128..7b97347 100644 --- a/specs/011-push.md +++ b/specs/011-push.md @@ -45,11 +45,17 @@ git-loom push [branch] [--no-pr] Detection priority (first match wins): -1. **Explicit config**: `git config loom.remote-type` — values: `github`, `azure`, `gerrit` +1. **Explicit config**: `git config loom.remote-type` — values: `github`, `gitlab`, `azure`, `gerrit` 2. **URL heuristics**: Remote URL contains `github.com` → GitHub -3. **URL heuristics**: Remote URL contains `dev.azure.com` → Azure DevOps -4. **Hook inspection**: `.git/hooks/commit-msg` contains "gerrit" (case-insensitive) → Gerrit -5. **Fallback**: Plain +3. **URL heuristics**: Remote URL contains `gitlab` → GitLab +4. **URL heuristics**: Remote URL contains `dev.azure.com` → Azure DevOps +5. **Hook inspection**: `.git/hooks/commit-msg` contains "gerrit" (case-insensitive) → Gerrit +6. **Fallback**: Plain + +Self-hosted GitLab instances whose hostname does not contain `gitlab` (e.g. +`invent.kde.org`) are not auto-detected — set `git config loom.remote-type +gitlab` for those. Even without detection, a plain push still surfaces the MR +link the server prints (see [Plain](#plain-default)). ## Push Remote Selection @@ -73,6 +79,11 @@ Uses `--force-with-lease` because woven branches are frequently rebased and need force pushing. `--force-if-includes` adds an extra safety check that the local ref includes the remote ref. +After pushing, any `remote:` lines containing an `http(s)` URL are surfaced as +indented continuation lines below the success message. This shows the MR/PR +creation link that servers like GitLab print on push, even when the remote type +was not detected as GitLab. + ### GitHub ```bash @@ -102,6 +113,24 @@ repo. branch itself (e.g. pushing `main` when tracking `origin/main`), PR creation is skipped and the push falls back to the plain force-with-lease strategy. +### GitLab + +```bash +git push --force-with-lease --force-if-includes \ + -o merge_request.create -o merge_request.target= -u +# Pushed 'feature-a' to origin +# https://gitlab.com/group/repo/-/merge_requests/42 +``` + +Uses GitLab [push options](https://docs.gitlab.com/ee/user/project/push_options.html) +so the server creates a merge request (or points to the existing one) as part of +the push. The MR URL GitLab prints in the push output is surfaced below the +success message, reusing the same `remote:` URL extraction as the plain and +Gerrit strategies. No extra CLI tool is required. + +If the branch being pushed is the upstream target branch itself, the MR push +options are skipped and it falls back to a plain push. + ### Azure DevOps ```bash diff --git a/src/push.rs b/src/push.rs index 43545d2..b5c42c9 100644 --- a/src/push.rs +++ b/src/push.rs @@ -16,6 +16,7 @@ use crate::trace as loom_trace; enum RemoteType { Plain, GitHub, + GitLab, AzureDevOps, Gerrit { target_branch: String }, } @@ -67,6 +68,7 @@ pub fn run(branch: Option, no_pr: bool) -> Result<()> { base_oid, &info.upstream.label, ), + RemoteType::GitLab => push_gitlab(&workdir, &remote_name, &branch_name, &target_branch), RemoteType::AzureDevOps => push_azure( &repo, &workdir, @@ -110,6 +112,9 @@ fn detect_remote_type( if value == "github" { return Ok(RemoteType::GitHub); } + if value == "gitlab" { + return Ok(RemoteType::GitLab); + } if value == "azure" { return Ok(RemoteType::AzureDevOps); } @@ -119,7 +124,7 @@ fn detect_remote_type( } msg::warn(&format!( "Unknown loom.remote-type '{}' — falling back to auto-detection.\n\ - Valid values: github, azure, gerrit", + Valid values: github, gitlab, azure, gerrit", config_value.trim() )); } @@ -131,6 +136,9 @@ fn detect_remote_type( if url.contains("github.com") { return Ok(RemoteType::GitHub); } + if url.contains("gitlab") { + return Ok(RemoteType::GitLab); + } if url.contains("dev.azure.com") { return Ok(RemoteType::AzureDevOps); } @@ -240,8 +248,63 @@ fn resolve_push_remote( } } +/// Run a `git push …`, trace-log it, bail on failure, and return its stderr. +/// +/// stderr carries the server's `remote:` messages — GitLab MR links, Gerrit +/// review URLs, GitHub "create a pull request" hints — so callers surface them +/// to the user via [`append_remote_urls`]. +fn run_push_capture(workdir: &Path, args: &[&str]) -> Result { + let start = Instant::now(); + let output = Command::new("git") + .current_dir(workdir) + .args(args) + .output()?; + + let duration_ms = start.elapsed().as_millis(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + loom_trace::log_command( + "git", + &args.join(" "), + duration_ms, + output.status.success(), + &stderr, + ); + + if !output.status.success() { + bail!("git push failed"); + } + + Ok(stderr) +} + +/// Append `remote:` URLs found in git push stderr to `message`. +/// +/// Servers print MR/review links as `remote: https://…` lines. Each such URL +/// is added as an indented continuation line. A trailing `[tag]` (Gerrit) is +/// wrapped in backticks. +fn append_remote_urls(message: &mut String, stderr: &str) { + for line in stderr.lines() { + if let Some(rest) = line.strip_prefix("remote:") { + let trimmed = rest.trim(); + if trimmed.starts_with("http://") || trimmed.starts_with("https://") { + message.push('\n'); + if trimmed.ends_with(']') { + if let Some(pos) = trimmed.rfind('[') { + let (before, tag) = trimmed.split_at(pos); + message.push_str(&format!("{}`{}`", before, tag)); + } else { + message.push_str(trimmed); + } + } else { + message.push_str(trimmed); + } + } + } + } +} + fn git_push(workdir: &Path, remote: &str, branch: &str) -> Result<()> { - git::run_git( + let stderr = run_push_capture( workdir, &[ "push", @@ -252,7 +315,9 @@ fn git_push(workdir: &Path, remote: &str, branch: &str) -> Result<()> { branch, ], )?; - msg::success(&format!("Pushed `{}` to `{}`", branch, remote)); + let mut message = format!("Pushed `{}` to `{}`", branch, remote); + append_remote_urls(&mut message, &stderr); + msg::success(&message); Ok(()) } @@ -475,6 +540,39 @@ fn find_existing_github_pr(workdir: &Path, gh_repo: &str, head_arg: &str) -> Opt prs.get(0)?.get("url")?.as_str().map(str::to_string) } +/// Push to GitLab: push the branch with `merge_request.create` push options so +/// GitLab creates (or points to) a merge request, then surface the MR URL it +/// prints in the push output. +/// +/// If the branch being pushed is the upstream target branch itself, skip the +/// MR push options and fall back to a plain push. +fn push_gitlab(workdir: &Path, remote: &str, branch: &str, target_branch: &str) -> Result<()> { + if branch == target_branch { + return git_push(workdir, remote, branch); + } + + let target_opt = format!("merge_request.target={}", target_branch); + let stderr = run_push_capture( + workdir, + &[ + "push", + "--force-with-lease", + "--force-if-includes", + "-o", + "merge_request.create", + "-o", + &target_opt, + "-u", + remote, + branch, + ], + )?; + let mut message = format!("Pushed `{}` to `{}`", branch, remote); + append_remote_urls(&mut message, &stderr); + msg::success(&message); + Ok(()) +} + /// Azure DevOps coordinates parsed from a git remote URL. struct AzureRemote { /// Organization URL, e.g. `https://dev.azure.com/`. @@ -801,44 +899,13 @@ fn push_gerrit(workdir: &Path, remote: &str, branch: &str, target_branch: &str) let refspec = format!("{}:refs/for/{}", branch, target_branch); let topic_opt = format!("topic={}", branch); - let args = ["push", "-o", &topic_opt, remote, &refspec]; - let start = Instant::now(); - let output = Command::new("git") - .current_dir(workdir) - .args(args) - .output()?; - - let duration_ms = start.elapsed().as_millis(); - let stderr = String::from_utf8_lossy(&output.stderr); - let cmd = args.join(" "); - loom_trace::log_command("git", &cmd, duration_ms, output.status.success(), &stderr); - - if !output.status.success() { - bail!("git push failed"); - } + let stderr = run_push_capture(workdir, &["push", "-o", &topic_opt, remote, &refspec])?; let mut message = format!( "Pushed `{}` to `{}` (Gerrit: `refs/for/{}`)", branch, remote, target_branch ); - for line in stderr.lines() { - if let Some(rest) = line.strip_prefix("remote:") { - let trimmed = rest.trim(); - if trimmed.starts_with("http://") || trimmed.starts_with("https://") { - message.push('\n'); - if trimmed.ends_with(']') { - if let Some(pos) = trimmed.rfind('[') { - let (before, tag) = trimmed.split_at(pos); - message.push_str(&format!("{}`{}`", before, tag)); - } else { - message.push_str(trimmed); - } - } else { - message.push_str(trimmed); - } - } - } - } + append_remote_urls(&mut message, &stderr); msg::success(&message); Ok(()) diff --git a/src/push_test.rs b/src/push_test.rs index f85c9be..d91fc85 100644 --- a/src/push_test.rs +++ b/src/push_test.rs @@ -301,6 +301,70 @@ fn detect_remote_type_gerrit_in_worktree() { ); } +#[test] +fn detect_remote_type_gitlab_by_config() { + let test_repo = TestRepo::new_with_remote(); + let workdir = test_repo.workdir(); + test_repo.commit("C1", "c1.txt"); + + test_repo.set_config("loom.remote-type", "gitlab"); + + let result = super::detect_remote_type(&test_repo.repo, &workdir, "origin/main"); + assert!(result.is_ok()); + assert_eq!(result.unwrap(), super::RemoteType::GitLab); +} + +#[test] +fn detect_remote_type_gitlab_by_url() { + let test_repo = TestRepo::new_with_remote(); + let workdir = test_repo.workdir(); + test_repo.commit("C1", "c1.txt"); + + test_repo + .repo + .remote_set_url("origin", "git@gitlab.com:group/repo.git") + .unwrap(); + + let result = super::detect_remote_type(&test_repo.repo, &workdir, "origin/main"); + assert!(result.is_ok()); + assert_eq!(result.unwrap(), super::RemoteType::GitLab); +} + +// ── append_remote_urls tests ───────────────────────────────────────────── + +#[test] +fn append_remote_urls_extracts_gitlab_mr_link() { + let stderr = "remote: \n\ + remote: To create a merge request for feature-a, visit:\n\ + remote: https://gitlab.com/group/repo/-/merge_requests/new?x=1\n\ + remote: \n"; + let mut message = String::from("Pushed"); + super::append_remote_urls(&mut message, stderr); + assert_eq!( + message, + "Pushed\nhttps://gitlab.com/group/repo/-/merge_requests/new?x=1" + ); +} + +#[test] +fn append_remote_urls_wraps_gerrit_tag() { + let stderr = "remote: https://gerrit.example.com/c/proj/+/123 [NEW]\n"; + let mut message = String::from("Pushed"); + super::append_remote_urls(&mut message, stderr); + assert_eq!( + message, + "Pushed\nhttps://gerrit.example.com/c/proj/+/123 `[NEW]`" + ); +} + +#[test] +fn append_remote_urls_ignores_non_url_lines() { + let stderr = "remote: Counting objects: 5, done.\nSwitched to branch\n"; + let mut message = String::from("Pushed"); + super::append_remote_urls(&mut message, stderr); + assert_eq!(message, "Pushed"); +} + #[test] fn detect_remote_type_azure_by_url() { let test_repo = TestRepo::new_with_remote();