git: forward remote messages to output

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 {

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 {

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(

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<T>(
     ui: &Ui,
     sideband_progress_callback: Option<SidebandProgressCallback<'_>>,
-    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(

lib/src/git.rs

@@ -1536,10 +1536,10 @@ impl<'a> GitFetch<'a> {
 
     fn git2_fetch(
         &mut self,
-        callbacks: RemoteCallbacks<'_>,
-        depth: Option<NonZeroU32>,
         remote_name: &str,
         branch_names: &[StringPattern],
+        callbacks: RemoteCallbacks<'_>,
+        depth: Option<NonZeroU32>,
     ) -> Result<Option<String>, 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<NonZeroU32>,
         remote_name: &str,
         branch_names: &[StringPattern],
+        mut callbacks: RemoteCallbacks<'_>,
+        depth: Option<NonZeroU32>,
     ) -> Result<Option<String>, 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<NonZeroU32>,
     ) -> Result<Option<String>, 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<String> = 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());

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<PathBuf>, 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<Output, GitSubprocessError> {
+    fn spawn_cmd(&self, mut git_cmd: Command) -> Result<Child, GitSubprocessError> {
         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<NonZeroU32>,
         refspecs: &[RefSpec],
+        callbacks: &mut RemoteCallbacks<'_>,
+        depth: Option<NonZeroU32>,
     ) -> Result<Option<String>, 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<String>, Vec<String>), 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<String> {
 // 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<Option<String>, GitSubprocessError> {
+fn parse_git_fetch_output(
+    output: Output,
+    sideband_progress: &mut Option<SidebandProgressCallback<'_>>,
+) -> Result<Option<String>, 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<String>, Vec<String>), 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<String>, Vec<String>), GitSubprocessError> {
+fn parse_git_push_output(
+    output: Output,
+    sideband_progress: &mut Option<SidebandProgressCallback<'_>>,
+) -> Result<(Vec<String>, Vec<String>), 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<String>, Vec<String>), 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<SidebandProgressCallback<'_>>,
+) {
+    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<Output, GitSubprocessError> {
+    child.wait_with_output().map_err(GitSubprocessError::Wait)
+}
+
 #[cfg(test)]
 mod test {
     use super::*;