git: spawn a separate git process for network operations

.github/workflows/build-nix.yml

@@ -4,7 +4,6 @@ on:
   push:
     branches:
       - main
-  pull_request:
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}

.github/workflows/build.yml

@@ -20,7 +20,7 @@ jobs:
       fail-fast: false
       matrix:
         # macos-13 is x86; macos-14 is ARM
-        os: [ubuntu-24.04, macos-13, macos-14, windows-latest]
+        os: [ubuntu-latest, macos-13, macos-14, windows-latest]
         cargo_flags: [""]
         include:
         - os: ubuntu-24.04
@@ -72,10 +72,24 @@ jobs:
       uses: dtolnay/rust-toolchain@a54c7afa936fefeb4456b2dd8068152669aa8203
       with:
         toolchain: 1.76
+    - name: Test [windows]
+      if: ${{ matrix.os == 'windows-latest' }}
+      run: |
+        git.exe --version
+        Get-Command -Name git
+        where git
+        which git
+        $env:TEST_GIT_PATH='/bin/git'
+        cargo test --workspace --all-targets --verbose ${{ matrix.cargo_flags }} test_git_clone
+      env:
+        RUST_BACKTRACE: 1
     - name: Build
       run: cargo build --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
-    - name: Test
-      run: cargo test --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
+    - name: Test [non-windows]
+      if: ${{ matrix.os != 'windows-latest' }}
+      run: |
+        export TEST_GIT_PATH='/usr/bin/git';
+        cargo test --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
       env:
         RUST_BACKTRACE: 1
 

cli/src/commands/git/clone.rs

@@ -34,6 +34,7 @@ use crate::command_error::user_error;
 use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::commands::git::maybe_add_gitignore;
+use crate::git_util::get_config_git_path;
 use crate::git_util::get_git_repo;
 use crate::git_util::map_git_error;
 use crate::git_util::print_git_import_stats;
@@ -129,6 +130,7 @@ pub fn cmd_git_clone(
     // `/some/path/.`
     let canonical_wc_path = dunce::canonicalize(&wc_path)
         .map_err(|err| user_error_with_message(format!("Failed to create {wc_path_str}"), err))?;
+    let git_executable_path = get_config_git_path(command)?;
     let clone_result = do_git_clone(
         ui,
         command,
@@ -137,6 +139,7 @@ pub fn cmd_git_clone(
         remote_name,
         &source,
         &canonical_wc_path,
+        git_executable_path,
     );
     if clone_result.is_err() {
         let clean_up_dirs = || -> io::Result<()> {
@@ -196,6 +199,7 @@ fn do_git_clone(
     remote_name: &str,
     source: &str,
     wc_path: &Path,
+    git_executable_path: Option<String>,
 ) -> Result<(WorkspaceCommandHelper, GitFetchStats), CommandError> {
     let settings = command.settings_for_new_workspace(wc_path)?;
     let (workspace, repo) = if colocate {
@@ -215,7 +219,7 @@ fn do_git_clone(
     let git_settings = workspace_command.settings().git_settings()?;
     let mut fetch_tx = workspace_command.start_transaction();
 
-    let stats = with_remote_git_callbacks(ui, None, |cb| {
+    let stats = with_remote_git_callbacks(ui, None, git_executable_path.is_some(), |cb| {
         git::fetch(
             fetch_tx.repo_mut(),
             &git_repo,
@@ -224,14 +228,34 @@ fn do_git_clone(
             cb,
             &git_settings,
             depth,
+            git_executable_path,
         )
     })
     .map_err(|err| match err {
-        GitFetchError::NoSuchRemote(_) => {
-            panic!("shouldn't happen as we just created the git remote")
+        GitFetchError::NoSuchRemote(repo_name) => {
+            user_error(format!("Could not find repository at '{repo_name}'"))
+        }
+        GitFetchError::GitNotFound(path) => {
+            if let Some(git_path) = path {
+                user_error(format!("Could not execute provided git path: `{git_path}`"))
+            } else {
+                user_error("Could not find a `git` executable in the OS path")
+            }
         }
         GitFetchError::GitImportError(err) => CommandError::from(err),
         GitFetchError::InternalGitError(err) => map_git_error(err),
+        GitFetchError::GitForkError(err) => CommandError::with_message(
+            crate::command_error::CommandErrorKind::Internal,
+            "External git process failed",
+            err,
+        ),
+        GitFetchError::GitExternalError(err) => {
+            CommandError::new(crate::command_error::CommandErrorKind::Internal, err)
+        }
+        GitFetchError::PathConversionError(path) => CommandError::new(
+            crate::command_error::CommandErrorKind::Internal,
+            format!("Failed to convert path {} to string", path.display()),
+        ),
         GitFetchError::InvalidBranchPattern => {
             unreachable!("we didn't provide any globs")
         }

cli/src/commands/git/fetch.rs

@@ -23,6 +23,7 @@ use crate::cli_util::CommandHelper;
 use crate::command_error::CommandError;
 use crate::commands::git::get_single_remote;
 use crate::complete;
+use crate::git_util::get_config_git_path;
 use crate::git_util::get_git_repo;
 use crate::git_util::git_fetch;
 use crate::ui::Ui;
@@ -69,6 +70,7 @@ pub fn cmd_git_fetch(
     args: &GitFetchArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
+    let git_executable_path = get_config_git_path(command)?;
     let git_repo = get_git_repo(workspace_command.repo().store())?;
     let remotes = if args.all_remotes {
         get_all_remotes(&git_repo)?
@@ -78,7 +80,14 @@ pub fn cmd_git_fetch(
         args.remotes.clone()
     };
     let mut tx = workspace_command.start_transaction();
-    git_fetch(ui, &mut tx, &git_repo, &remotes, &args.branch)?;
+    git_fetch(
+        ui,
+        &mut tx,
+        &git_repo,
+        &remotes,
+        &args.branch,
+        git_executable_path,
+    )?;
     tx.finish(
         ui,
         format!("fetch from git remote(s) {}", remotes.iter().join(",")),

cli/src/commands/git/push.rs

@@ -49,6 +49,7 @@ use crate::command_error::CommandError;
 use crate::commands::git::get_single_remote;
 use crate::complete;
 use crate::formatter::Formatter;
+use crate::git_util::get_config_git_path;
 use crate::git_util::get_git_repo;
 use crate::git_util::map_git_error;
 use crate::git_util::with_remote_git_callbacks;
@@ -166,6 +167,7 @@ pub fn cmd_git_push(
     args: &GitPushArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
+    let git_executable_path = get_config_git_path(command)?;
     let git_repo = get_git_repo(workspace_command.repo().store())?;
 
     let remote = if let Some(name) = &args.remote {
@@ -313,9 +315,22 @@ pub fn cmd_git_push(
     let mut sideband_progress_callback = |progress_message: &[u8]| {
         _ = writer.write(ui, progress_message);
     };
-    with_remote_git_callbacks(ui, Some(&mut sideband_progress_callback), |cb| {
-        git::push_branches(tx.repo_mut(), &git_repo, &remote, &targets, cb)
-    })
+
+    with_remote_git_callbacks(
+        ui,
+        Some(&mut sideband_progress_callback),
+        git_executable_path.is_some(),
+        |cb| {
+            git::push_branches(
+                tx.repo_mut(),
+                &git_repo,
+                &remote,
+                &targets,
+                cb,
+                git_executable_path,
+            )
+        },
+    )
     .map_err(|err| match err {
         GitPushError::InternalGitError(err) => map_git_error(err),
         GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint(
@@ -327,6 +342,13 @@ pub fn cmd_git_push(
             "Try fetching from the remote, then make the bookmark point to where you want it to \
              be, and push again.",
         ),
+        GitPushError::GitNotFound(path) => {
+            if let Some(git_path) = path {
+                user_error(format!("Could not execute provided git path: `{git_path}`"))
+            } else {
+                user_error("Could not find a `git` executable in the OS path")
+            }
+        }
         _ => user_error(err),
     })?;
     writer.flush(ui)?;

cli/src/config-schema.json

@@ -362,6 +362,15 @@
                     "type": "string",
                     "description": "The remote to which commits are pushed",
                     "default": "origin"
+                },
+                "shell": {
+                    "type": "boolean",
+                    "description": "Whether jj shells out to git for network operations (push/fetch/clone)",
+                    "default": false
+                },
+                "path": {
+                    "type": "string",
+                    "description": "Path to the git executable"
                 }
             }
         },

cli/src/git_util.rs

@@ -24,6 +24,7 @@ use std::process::Stdio;
 use std::time::Instant;
 
 use itertools::Itertools;
+use jj_lib::config::ConfigGetError;
 use jj_lib::git;
 use jj_lib::git::FailedRefExport;
 use jj_lib::git::FailedRefExportReason;
@@ -40,6 +41,7 @@ use jj_lib::str_util::StringPattern;
 use jj_lib::workspace::Workspace;
 use unicode_width::UnicodeWidthStr;
 
+use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandTransaction;
 use crate::command_error::user_error;
 use crate::command_error::user_error_with_hint;
@@ -257,29 +259,35 @@ type SidebandProgressCallback<'a> = &'a mut dyn FnMut(&[u8]);
 pub fn with_remote_git_callbacks<T>(
     ui: &Ui,
     sideband_progress_callback: Option<SidebandProgressCallback<'_>>,
+    shell: bool,
     f: impl FnOnce(git::RemoteCallbacks<'_>) -> T,
 ) -> T {
-    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);
-        });
+    if shell {
+        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)
     }
-    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(
@@ -462,11 +470,12 @@ pub fn git_fetch(
     git_repo: &git2::Repository,
     remotes: &[String],
     branch: &[StringPattern],
+    git_executable_path: Option<String>,
 ) -> Result<(), CommandError> {
     let git_settings = tx.settings().git_settings()?;
 
     for remote in remotes {
-        let stats = with_remote_git_callbacks(ui, None, |cb| {
+        let stats = with_remote_git_callbacks(ui, None, git_executable_path.is_some(), |cb| {
             git::fetch(
                 tx.repo_mut(),
                 git_repo,
@@ -475,6 +484,7 @@ pub fn git_fetch(
                 cb,
                 &git_settings,
                 None,
+                git_executable_path.clone(),
             )
         })
         .map_err(|err| match err {
@@ -493,6 +503,13 @@ pub fn git_fetch(
             }
             GitFetchError::GitImportError(err) => err.into(),
             GitFetchError::InternalGitError(err) => map_git_error(err),
+            GitFetchError::GitNotFound(path) => {
+                if let Some(git_path) = path {
+                    user_error(format!("Could not execute provided git path: `{git_path}`"))
+                } else {
+                    user_error("Could not find a `git` executable in the OS path")
+                }
+            }
             _ => user_error(err),
         })?;
         print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
@@ -535,3 +552,33 @@ fn warn_if_branches_not_found(
 
     Ok(())
 }
+
+pub(crate) fn get_config_git_shell(command: &CommandHelper) -> Result<bool, ConfigGetError> {
+    command.settings().get_bool("git.shell").or_else(|e| {
+        if matches!(e, ConfigGetError::NotFound { .. }) {
+            Ok(false)
+        } else {
+            Err(e)
+        }
+    })
+}
+
+pub(crate) fn get_config_git_path(
+    command: &CommandHelper,
+) -> Result<Option<String>, ConfigGetError> {
+    if get_config_git_shell(command)? {
+        command
+            .settings()
+            .get_string("git.path")
+            .map(|x| Some(x))
+            .or_else(|e| {
+                if matches!(e, ConfigGetError::NotFound { .. }) {
+                    Ok(Some("git".to_string()))
+                } else {
+                    Err(e)
+                }
+            })
+    } else {
+        Ok(None)
+    }
+}

cli/tests/cli-reference@.md.snap

@@ -1,6 +1,7 @@
 ---
 source: cli/tests/test_generate_md_cli_help.rs
 description: "AUTO-GENERATED FILE, DO NOT EDIT. This cli reference is generated by a test as an `insta` snapshot. MkDocs includes this snapshot from docs/cli-reference.md."
+snapshot_kind: text
 ---
 <!-- BEGIN MARKDOWN-->
 

cli/tests/common/mod.rs

@@ -80,11 +80,21 @@ impl Default for TestEnvironment {
 'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()'
         "#,
         );
+
         env
     }
 }
 
 impl TestEnvironment {
+    pub fn add_git(&self) {
+        self.add_config("git.shell = true");
+
+        // add a git path from env: this is only used if git.shell = true
+        if let Ok(git_path) = std::env::var("TEST_GIT_PATH") {
+            self.add_config(format!("git.path = '{}'", git_path.trim()));
+        }
+    }
+
     pub fn jj_cmd(&self, current_dir: &Path, args: &[&str]) -> assert_cmd::Command {
         let mut cmd = assert_cmd::Command::cargo_bin("jj").unwrap();
         cmd.current_dir(current_dir);