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>
```

<!--
There's no need to add anything here, but feel free to add a personal message.
Please describe the changes in this PR in the commit message(s) instead, with
each commit representing one logical change. Address code review comments by
rewriting the commits rather than adding commits on top. Use force-push when
pushing the updated commits (`jj git push` does that automatically when you
rewrite commits). Merge the PR at will once it's been approved. See
https://github.com/jj-vcs/jj/blob/main/docs/contributing.md for details.
Note that you need to sign Google's CLA to contribute.
-->

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,9 +86,19 @@ jobs:
         tool: nextest
     - name: Build
       run: cargo build --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
-    - name: Test
+    - 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: cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
       env:
+        TEST_GIT_PATH: 'C:\Program Files\Git\bin\git.exe'
         RUST_BACKTRACE: 1
         CARGO_TERM_COLOR: always
 

CHANGELOG.md

@@ -49,6 +49,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.
@@ -64,6 +66,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

@@ -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;
@@ -97,6 +98,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 +137,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 +203,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 +232,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 +245,12 @@ 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::GitImportError(err) => CommandError::from(err),
         GitFetchError::InternalGitError(err) => map_git_error(err),
+        GitFetchError::Subprocess(err) => user_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(

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,
@@ -686,6 +701,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;

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,6 +80,7 @@ impl Default for TestEnvironment {
 'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()'
         "#,
         );
+
         env
     }
 }
@@ -280,6 +281,15 @@ impl TestEnvironment {
         self.env_vars.insert(key.into(), val.into());
     }
 
+    pub fn set_up_git_subprocessing(&self) {
+        self.add_config("git.subprocess = true");
+
+        // add a git path from env: this is only used if git.subprocess = true
+        if let Ok(git_path) = std::env::var("TEST_GIT_PATH") {
+            self.add_config(format!("git.executable_path = '{}'", git_path.trim()));
+        }
+    }
+
     pub fn current_operation_id(&self, repo_path: &Path) -> String {
         let id_and_newline =
             self.jj_cmd_success(repo_path, &["debug", "operation", "--display=id"]);