From 120b9cc76646150d2775402a14136b4cdf3597d9 Mon Sep 17 00:00:00 2001 From: Baltasar Dinis Date: Thu, 23 Jan 2025 20:06:55 +0000 Subject: [PATCH] git: forward remote messages to output --- cli/src/commands/git/clone.rs | 2 +- cli/src/commands/git/fetch.rs | 2 +- cli/src/commands/git/push.rs | 25 +++++------ cli/src/git_util.rs | 52 +++++++++------------- lib/src/git.rs | 18 +++++--- lib/src/git_subprocess.rs | 82 +++++++++++++++++++++++++---------- 6 files changed, 103 insertions(+), 78 deletions(-) diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 94cd0b1f5..71238f7cc 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -227,7 +227,7 @@ fn fetch_new_remote( let git_settings = workspace_command.settings().git_settings()?; let mut fetch_tx = workspace_command.start_transaction(); let mut git_fetch = GitFetch::new(fetch_tx.repo_mut(), &git_repo, &git_settings); - let default_branch = with_remote_git_callbacks(ui, None, &git_settings, |cb| { + let default_branch = with_remote_git_callbacks(ui, None, |cb| { git_fetch .fetch(remote_name, &[StringPattern::everything()], cb, depth) .map_err(|err| match err { diff --git a/cli/src/commands/git/fetch.rs b/cli/src/commands/git/fetch.rs index e5f118c79..2ab9b56fc 100644 --- a/cli/src/commands/git/fetch.rs +++ b/cli/src/commands/git/fetch.rs @@ -139,7 +139,7 @@ fn do_git_fetch( let mut git_fetch = GitFetch::new(tx.repo_mut(), git_repo, &git_settings); for remote_name in remotes { - with_remote_git_callbacks(ui, None, &git_settings, |callbacks| { + with_remote_git_callbacks(ui, None, |callbacks| { git_fetch .fetch(remote_name, branch_names, callbacks, None) .map_err(|err| match err { diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 72707a8ab..7c5f91d0e 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -377,21 +377,16 @@ pub fn cmd_git_push( }; let git_settings = tx.settings().git_settings()?; - with_remote_git_callbacks( - ui, - Some(&mut sideband_progress_callback), - &git_settings, - |cb| { - git::push_branches( - tx.repo_mut(), - &git_repo, - &git_settings, - &remote, - &targets, - cb, - ) - }, - ) + with_remote_git_callbacks(ui, Some(&mut sideband_progress_callback), |cb| { + git::push_branches( + tx.repo_mut(), + &git_repo, + &git_settings, + &remote, + &targets, + cb, + ) + }) .map_err(|err| match err { GitPushError::InternalGitError(err) => map_git_error(err), GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint( diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs index 72285a649..e661eb964 100644 --- a/cli/src/git_util.rs +++ b/cli/src/git_util.rs @@ -40,7 +40,6 @@ use jj_lib::op_store::RefTarget; use jj_lib::op_store::RemoteRef; use jj_lib::repo::ReadonlyRepo; use jj_lib::repo::Repo; -use jj_lib::settings::GitSettings; use jj_lib::store::Store; use jj_lib::workspace::Workspace; use unicode_width::UnicodeWidthStr; @@ -284,40 +283,29 @@ type SidebandProgressCallback<'a> = &'a mut dyn FnMut(&[u8]); pub fn with_remote_git_callbacks( ui: &Ui, sideband_progress_callback: Option>, - git_settings: &GitSettings, f: impl FnOnce(git::RemoteCallbacks<'_>) -> T, ) -> T { - if git_settings.subprocess { - // TODO: with git2, we are able to display progress from the data that is given - // With the git processes themselves, this is significantly harder, as it - // requires parsing the output directly - // - // In any case, this would be the place to add that funcionalty - f(git::RemoteCallbacks::default()) - } else { - let mut callbacks = git::RemoteCallbacks::default(); - let mut progress_callback = None; - if let Some(mut output) = ui.progress_output() { - let mut progress = Progress::new(Instant::now()); - progress_callback = Some(move |x: &git::Progress| { - _ = progress.update(Instant::now(), x, &mut output); - }); - } - callbacks.progress = progress_callback - .as_mut() - .map(|x| x as &mut dyn FnMut(&git::Progress)); - callbacks.sideband_progress = - sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8])); - let mut get_ssh_keys = get_ssh_keys; // Coerce to unit fn type - callbacks.get_ssh_keys = Some(&mut get_ssh_keys); - let mut get_pw = - |url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url)); - callbacks.get_password = Some(&mut get_pw); - let mut get_user_pw = - |url: &str| Some((terminal_get_username(ui, url)?, terminal_get_pw(ui, url)?)); - callbacks.get_username_password = Some(&mut get_user_pw); - f(callbacks) + let mut callbacks = git::RemoteCallbacks::default(); + let mut progress_callback = None; + if let Some(mut output) = ui.progress_output() { + let mut progress = Progress::new(Instant::now()); + progress_callback = Some(move |x: &git::Progress| { + _ = progress.update(Instant::now(), x, &mut output); + }); } + callbacks.progress = progress_callback + .as_mut() + .map(|x| x as &mut dyn FnMut(&git::Progress)); + callbacks.sideband_progress = sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8])); + let mut get_ssh_keys = get_ssh_keys; // Coerce to unit fn type + callbacks.get_ssh_keys = Some(&mut get_ssh_keys); + let mut get_pw = + |url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url)); + callbacks.get_password = Some(&mut get_pw); + let mut get_user_pw = + |url: &str| Some((terminal_get_username(ui, url)?, terminal_get_pw(ui, url)?)); + callbacks.get_username_password = Some(&mut get_user_pw); + f(callbacks) } pub fn print_git_import_stats( diff --git a/lib/src/git.rs b/lib/src/git.rs index 1ccc23b0f..37822c734 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1536,10 +1536,10 @@ impl<'a> GitFetch<'a> { fn git2_fetch( &mut self, - callbacks: RemoteCallbacks<'_>, - depth: Option, remote_name: &str, branch_names: &[StringPattern], + callbacks: RemoteCallbacks<'_>, + depth: Option, ) -> Result, GitFetchError> { let mut remote = self.git_repo.find_remote(remote_name).map_err(|err| { if is_remote_not_found_err(&err) { @@ -1592,9 +1592,10 @@ impl<'a> GitFetch<'a> { fn subprocess_fetch( &mut self, - depth: Option, remote_name: &str, branch_names: &[StringPattern], + mut callbacks: RemoteCallbacks<'_>, + depth: Option, ) -> Result, GitFetchError> { // check the remote exists // TODO: we should ideally find a way to do this without git2 @@ -1625,7 +1626,7 @@ impl<'a> GitFetch<'a> { // even more unfortunately, git errors out one refspec at a time, // meaning that the below cycle runs in O(#failed refspecs) while let Some(failing_refspec) = - git_ctx.spawn_fetch(remote_name, depth, &remaining_refspecs)? + git_ctx.spawn_fetch(remote_name, &remaining_refspecs, &mut callbacks, depth)? { remaining_refspecs.retain(|r| r.source.as_ref() != Some(&failing_refspec)); @@ -1660,9 +1661,9 @@ impl<'a> GitFetch<'a> { depth: Option, ) -> Result, GitFetchError> { let default_branch = if self.git_settings.subprocess { - self.subprocess_fetch(depth, remote_name, branch_names) + self.subprocess_fetch(remote_name, branch_names, callbacks, depth) } else { - self.git2_fetch(callbacks, depth, remote_name, branch_names) + self.git2_fetch(remote_name, branch_names, callbacks, depth) }; self.fetched.push(FetchedBranches { @@ -1832,6 +1833,7 @@ pub fn push_updates( remote_name, &qualified_remote_refs_expected_locations, &refspecs, + callbacks, ) } else { let refspecs: Vec = refspecs.iter().map(RefSpec::to_git_format).collect(); @@ -1978,6 +1980,7 @@ fn subprocess_push_refs( remote_name: &str, qualified_remote_refs_expected_locations: &HashMap<&str, Option<&CommitId>>, refspecs: &[RefSpec], + callbacks: RemoteCallbacks<'_>, ) -> Result<(), GitPushError> { // check the remote exists // TODO: we should ideally find a way to do this without git2 @@ -2000,7 +2003,8 @@ fn subprocess_push_refs( .map(|full_refspec| RefToPush::new(full_refspec, qualified_remote_refs_expected_locations)) .collect(); - let (failed_ref_matches, successful_pushes) = git_ctx.spawn_push(remote_name, &refs_to_push)?; + let (failed_ref_matches, successful_pushes) = + git_ctx.spawn_push(remote_name, &refs_to_push, callbacks)?; for remote_ref in successful_pushes { remaining_remote_refs.remove(remote_ref.as_str()); diff --git a/lib/src/git_subprocess.rs b/lib/src/git_subprocess.rs index 3b7029f7d..bbaa65fe6 100644 --- a/lib/src/git_subprocess.rs +++ b/lib/src/git_subprocess.rs @@ -15,6 +15,7 @@ use std::num::NonZeroU32; use std::path::Path; use std::path::PathBuf; +use std::process::Child; use std::process::Command; use std::process::Output; use std::process::Stdio; @@ -24,6 +25,7 @@ use thiserror::Error; use crate::git::RefSpec; use crate::git::RefToPush; +use crate::git::RemoteCallbacks; /// Error originating by a Git subprocess #[derive(Error, Debug)] @@ -54,6 +56,8 @@ pub(crate) struct GitSubprocessContext<'a> { git_executable_path: &'a Path, } +pub(crate) type SidebandProgressCallback<'a> = &'a mut dyn FnMut(&[u8]); + impl<'a> GitSubprocessContext<'a> { pub(crate) fn new(git_dir: impl Into, git_executable_path: &'a Path) -> Self { GitSubprocessContext { @@ -84,9 +88,9 @@ impl<'a> GitSubprocessContext<'a> { } /// Spawn the git command - fn spawn_cmd(&self, mut git_cmd: Command) -> Result { + fn spawn_cmd(&self, mut git_cmd: Command) -> Result { tracing::debug!(cmd = ?git_cmd, "spawning a git subprocess"); - let child_git = git_cmd.spawn().map_err(|error| { + git_cmd.spawn().map_err(|error| { if self.git_executable_path.is_absolute() { GitSubprocessError::Spawn { path: self.git_executable_path.to_path_buf(), @@ -98,11 +102,7 @@ impl<'a> GitSubprocessContext<'a> { error, } } - })?; - - child_git - .wait_with_output() - .map_err(GitSubprocessError::Wait) + }) } /// Perform a git fetch @@ -112,8 +112,9 @@ impl<'a> GitSubprocessContext<'a> { pub(crate) fn spawn_fetch( &self, remote_name: &str, - depth: Option, refspecs: &[RefSpec], + callbacks: &mut RemoteCallbacks<'_>, + depth: Option, ) -> Result, GitSubprocessError> { if refspecs.is_empty() { return Ok(None); @@ -130,9 +131,9 @@ impl<'a> GitSubprocessContext<'a> { command.arg("--").arg(remote_name); command.args(refspecs.iter().map(|x| x.to_git_format())); - let output = self.spawn_cmd(command)?; + let output = wait_with_output(self.spawn_cmd(command)?)?; - parse_git_fetch_output(output) + parse_git_fetch_output(output, &mut callbacks.sideband_progress) } /// Prune particular branches @@ -148,7 +149,7 @@ impl<'a> GitSubprocessContext<'a> { command.args(["branch", "--remotes", "--delete", "--"]); command.args(branches_to_prune); - let output = self.spawn_cmd(command)?; + let output = wait_with_output(self.spawn_cmd(command)?)?; // we name the type to make sure that it is not meant to be used let () = parse_git_branch_prune_output(output)?; @@ -169,7 +170,7 @@ impl<'a> GitSubprocessContext<'a> { let mut command = self.create_command(); command.stdout(Stdio::piped()); command.args(["remote", "show", "--", remote_name]); - let output = self.spawn_cmd(command)?; + let output = wait_with_output(self.spawn_cmd(command)?)?; let output = parse_git_remote_show_output(output)?; @@ -189,6 +190,7 @@ impl<'a> GitSubprocessContext<'a> { &self, remote_name: &str, references: &[RefToPush], + mut callbacks: RemoteCallbacks<'_>, ) -> Result<(Vec, Vec), GitSubprocessError> { let mut command = self.create_command(); command.stdout(Stdio::piped()); @@ -207,9 +209,9 @@ impl<'a> GitSubprocessContext<'a> { .map(|r| r.refspec.to_git_format_not_forced()), ); - let output = self.spawn_cmd(command)?; + let output = wait_with_output(self.spawn_cmd(command)?)?; - parse_git_push_output(output) + parse_git_push_output(output, &mut callbacks.sideband_progress) } } @@ -287,8 +289,12 @@ fn parse_no_remote_tracking_branch(stderr: &[u8]) -> Option { // return the fully qualified ref that failed to fetch // // note that git fetch only returns one error at a time -fn parse_git_fetch_output(output: Output) -> Result, GitSubprocessError> { +fn parse_git_fetch_output( + output: Output, + sideband_progress: &mut Option>, +) -> Result, GitSubprocessError> { if output.status.success() { + report_remote_messages(&output.stderr, sideband_progress); return Ok(None); } @@ -452,15 +458,13 @@ fn parse_ref_pushes(stdout: &[u8]) -> Result<(Vec, Vec), GitSubp // on Ok, return a tuple with // 1. list of failed references from test and set // 2. list of successful references pushed -fn parse_git_push_output(output: Output) -> Result<(Vec, Vec), GitSubprocessError> { +fn parse_git_push_output( + output: Output, + sideband_progress: &mut Option>, +) -> Result<(Vec, Vec), GitSubprocessError> { if output.status.success() { - // TODO: figure out how to report `remote: ` logging nicely - // In sum, git apparently supports the remote repo sending a message to the - // local, which we should probably forward nicely - // - // If we support this, we can be a bit more strict, and say that messages other - // than these constitute an error let ref_pushes = parse_ref_pushes(&output.stdout)?; + report_remote_messages(&output.stderr, sideband_progress); return Ok(ref_pushes); } @@ -478,6 +482,40 @@ fn parse_git_push_output(output: Output) -> Result<(Vec, Vec), G } } +/// Report Git remote messages on sideband progress +/// +/// Git remotes can send custom messages on fetch and push, which the `git` +/// command prepends with `remote: `. +/// +/// For instance, these messages can provide URLs to create Pull Requests +/// e.g.: +/// ```ignore +/// $ jj git push -c @ +/// [...] +/// remote: +/// remote: Create a pull request for 'branch' on GitHub by visiting: +/// remote: https://github.com/user/repo/pull/new/branch +/// remote: +/// ``` +fn report_remote_messages( + stderr: &[u8], + sideband_progress: &mut Option>, +) { + if let Some(progress) = sideband_progress { + stderr + .lines() + .filter_map(|line| line.strip_prefix(b"remote: ")) + .for_each(|line| { + progress(line); + progress(b"\n"); + }); + } +} + +fn wait_with_output(child: Child) -> Result { + child.wait_with_output().map_err(GitSubprocessError::Wait) +} + #[cfg(test)] mod test { use super::*;