git: spawn a separate git process for network operations

Reasoning:

`jj` fails to push/fetch over ssh depending on the system.
Issue #4979 lists over 20 related issues on this and proposes spawning
a `git` subprocess for tasks related to the network (in fact, just push/fetch
are enough).

This PR implements this.
Users can either enable shelling out to git in a config file:

```toml
[git]
subprocess = true
```

Implementation Details:

This PR implements shelling out to `git` via `std::process::Command`.
There are 2 sharp edges with the patch:
 - it relies on having to parse out git errors to match the error codes
   (and parsing git2's errors in one particular instance to match the
   error behaviour). This seems mostly unavoidable

 - to ensure matching behaviour with git2, the tests are maintained across the
   two implementations. This is done using test_case, as with the rest
   of the codebase

Testing:

Run the rust tests:
```
$ cargo test
```

Build:
```
$ cargo build
```

Clone a private repo:
```
$ path/to/jj git clone --shell <REPO_SSH_URL>
```

Create new commit and push
```
$ echo "TEST" > this_is_a_test_file.txt
$ path/to/jj describe -m 'test commit'
$ path/to/jj git push --shell -b <branch>
```

Issues Closed

With a grain of salt, but most of these problems should be fixed (or at least checked if they are fixed). They are the ones listed in #4979 .

SSH:
- #63
- #440
- #1455
- #1507
- #2931
- #2958
- #3322
- #4101
- #4333
- #4386
- #4488
- #4591
- #4802
- #4870
- #4937
- #4978
- #5120
- #5166

Clone/fetch/push/pull:
- #360
- #1278
- #1957
- #2295
- #3851
- #4177
- #4682
- #4719
- #4889
- #5147
- #5238

Notable Holdouts:
 - Interactive HTTP authentication (#401, #469)
 - libssh2-sys dependency on windows problem (can only be removed if/when we get rid of libgit2): #3984

.github/workflows/build.yml

@@ -86,8 +86,19 @@ jobs:
         tool: nextest
     - name: Build
       run: cargo build --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
-    - name: Test
-      run: cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
+    - name: Test [non-windows]
+      if: ${{ matrix.os != 'windows-latest' }}
+      run: |
+        export TEST_GIT_PATH="$(which git)";
+        cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
+      env:
+        RUST_BACKTRACE: 1
+        CARGO_TERM_COLOR: always
+    - name: Test [windows]
+      if: ${{ matrix.os == 'windows-latest' }}
+      run: |
+        $env:TEST_GIT_PATH='C:\Program Files\Git\bin\git.exe'
+        cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
       env:
         RUST_BACKTRACE: 1
         CARGO_TERM_COLOR: always

CHANGELOG.md

@@ -46,6 +46,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### New features
 
+* `jj git {push,clone,fetch}` can now spawn an external `git` subprocess, via the `git.subprocess = true` config knob.
+
 * `jj evolog` now accepts `--reversed`.
 
 * `jj restore` now supports `-i`/`--interactive` selection.
@@ -61,6 +63,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed bugs
 
+* Fixes ssh bugs when interacting with git remotes due to `libgit2`'s limitations.
+  [#4979](https://github.com/jj-vcs/jj/issues/4979)
+
 * Fixed diff selection by external tools with `jj split`/`commit -i FILESETS`.
   [#5252](https://github.com/jj-vcs/jj/issues/5252)
 

cli/src/commands/git/clone.rs

@@ -30,10 +30,12 @@ use super::write_repository_level_trunk_alias;
 use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandHelper;
 use crate::command_error::cli_error;
+use crate::command_error::internal_error;
 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;
@@ -97,6 +99,13 @@ fn is_empty_dir(path: &Path) -> bool {
     }
 }
 
+struct CloneArgs<'a> {
+    depth: Option<NonZeroU32>,
+    remote_name: &'a str,
+    source: &'a str,
+    wc_path: &'a Path,
+}
+
 pub fn cmd_git_clone(
     ui: &mut Ui,
     command: &CommandHelper,
@@ -129,14 +138,18 @@ 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,
+        git_executable_path.as_deref(),
         args.colocate,
-        args.depth,
-        remote_name,
-        &source,
-        &canonical_wc_path,
+        CloneArgs {
+            depth: args.depth,
+            remote_name,
+            source: &source,
+            wc_path: &canonical_wc_path,
+        },
     );
     if clone_result.is_err() {
         let clean_up_dirs = || -> io::Result<()> {
@@ -191,12 +204,17 @@ pub fn cmd_git_clone(
 fn do_git_clone(
     ui: &mut Ui,
     command: &CommandHelper,
+    git_executable_path: Option<&Path>,
     colocate: bool,
-    depth: Option<NonZeroU32>,
-    remote_name: &str,
-    source: &str,
-    wc_path: &Path,
+    clone_args: CloneArgs<'_>,
 ) -> Result<(WorkspaceCommandHelper, GitFetchStats), CommandError> {
+    let CloneArgs {
+        depth,
+        remote_name,
+        source,
+        wc_path,
+    } = clone_args;
+
     let settings = command.settings_for_new_workspace(wc_path)?;
     let (workspace, repo) = if colocate {
         Workspace::init_colocated_git(&settings, wc_path)?
@@ -215,10 +233,11 @@ 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,
+            git_executable_path,
             remote_name,
             &[StringPattern::everything()],
             cb,
@@ -227,11 +246,22 @@ fn do_git_clone(
         )
     })
     .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::GitCommandNotFound(path) => {
+            if path == Path::new("git") {
+                user_error("Could not find a `git` executable in the OS path")
+            } else {
+                user_error(format!(
+                    "Could not execute provided git path: `{}`",
+                    path.display()
+                ))
+            }
         }
         GitFetchError::GitImportError(err) => CommandError::from(err),
         GitFetchError::InternalGitError(err) => map_git_error(err),
+        GitFetchError::Subprocess(err) => internal_error(err),
         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,
+        git_executable_path.as_deref(),
+        &remotes,
+        &args.branch,
+    )?;
     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,
+                git_executable_path.as_deref(),
+                &remote,
+                &targets,
+                cb,
+            )
+        },
+    )
     .map_err(|err| match err {
         GitPushError::InternalGitError(err) => map_git_error(err),
         GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint(
@@ -327,6 +342,16 @@ 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::GitCommandNotFound(path) => {
+            if path == std::path::Path::new("git") {
+                user_error("Could not find a `git` executable in the OS path")
+            } else {
+                user_error(format!(
+                    "Could not execute provided git path: `{}`",
+                    path.display()
+                ))
+            }
+        }
         _ => 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"
+                },
+                "subprocess": {
+                    "type": "boolean",
+                    "description": "Whether jj spawns a git subprocess for network operations (push/fetch/clone)",
+                    "default": false
+                },
+                "path": {
+                    "type": "string",
+                    "description": "Path to the git executable"
                 }
             }
         },

cli/src/git_util.rs

@@ -28,6 +28,7 @@ use std::time::Instant;
 use crossterm::terminal::Clear;
 use crossterm::terminal::ClearType;
 use itertools::Itertools;
+use jj_lib::config::ConfigGetError;
 use jj_lib::fmt_util::binary_prefix;
 use jj_lib::git;
 use jj_lib::git::FailedRefExport;
@@ -46,6 +47,7 @@ use jj_lib::workspace::Workspace;
 use unicode_width::UnicodeWidthStr;
 
 use crate::cleanup_guard::CleanupGuard;
+use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandTransaction;
 use crate::command_error::user_error;
 use crate::command_error::user_error_with_hint;
@@ -263,29 +265,40 @@ type SidebandProgressCallback<'a> = &'a mut dyn FnMut(&[u8]);
 pub fn with_remote_git_callbacks<T>(
     ui: &Ui,
     sideband_progress_callback: Option<SidebandProgressCallback<'_>>,
+    subprocess: 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 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)
     }
-    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(
@@ -610,16 +623,18 @@ pub fn git_fetch(
     ui: &mut Ui,
     tx: &mut WorkspaceCommandTransaction,
     git_repo: &git2::Repository,
+    git_executable_path: Option<&Path>,
     remotes: &[String],
     branch: &[StringPattern],
 ) -> 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,
+                git_executable_path,
                 remote,
                 branch,
                 cb,
@@ -643,6 +658,16 @@ pub fn git_fetch(
             }
             GitFetchError::GitImportError(err) => err.into(),
             GitFetchError::InternalGitError(err) => map_git_error(err),
+            GitFetchError::GitCommandNotFound(path) => {
+                if path == std::path::Path::new("git") {
+                    user_error("Could not find a `git` executable in the OS path")
+                } else {
+                    user_error(format!(
+                        "Could not execute provided git path: `{}`",
+                        path.display()
+                    ))
+                }
+            }
             _ => user_error(err),
         })?;
         print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
@@ -686,6 +711,23 @@ fn warn_if_branches_not_found(
     Ok(())
 }
 
+pub(crate) fn get_config_git_subprocess(command: &CommandHelper) -> Result<bool, ConfigGetError> {
+    command.settings().get_bool("git.subprocess")
+}
+
+pub(crate) fn get_config_git_path(
+    command: &CommandHelper,
+) -> Result<Option<PathBuf>, ConfigGetError> {
+    if get_config_git_subprocess(command)? {
+        command
+            .settings()
+            .get::<PathBuf>("git.executable_path")
+            .map(Some)
+    } else {
+        Ok(None)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use insta::assert_snapshot;