Skip to content

Commit 244f0f8

Browse files
bsdinisbsdinis
committedJan 16, 2025
git: spawn a separate git process for network operations
Reasoning: `jj` fails to push/fetch over ssh depending on the system. Issue jj-vcs#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 --config='git.subprocess=true' <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 --config='git.subprocess=true' -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 jj-vcs#4979 . SSH: - jj-vcs#63 - jj-vcs#440 - jj-vcs#1455 - jj-vcs#1507 - jj-vcs#2931 - jj-vcs#2958 - jj-vcs#3322 - jj-vcs#4101 - jj-vcs#4333 - jj-vcs#4386 - jj-vcs#4488 - jj-vcs#4591 - jj-vcs#4802 - jj-vcs#4870 - jj-vcs#4937 - jj-vcs#4978 - jj-vcs#5120 - jj-vcs#5166 Clone/fetch/push/pull: - jj-vcs#360 - jj-vcs#1278 - jj-vcs#1957 - jj-vcs#2295 - jj-vcs#3851 - jj-vcs#4177 - jj-vcs#4682 - jj-vcs#4719 - jj-vcs#4889 - jj-vcs#5147 - jj-vcs#5238 Notable Holdouts: - Interactive HTTP authentication (jj-vcs#401, jj-vcs#469) - libssh2-sys dependency on windows problem (can only be removed if/when we get rid of libgit2): jj-vcs#3984
1 parent f743165 commit 244f0f8

18 files changed

+2455
-283
lines changed
 

‎.github/workflows/build.yml

+11-1
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,19 @@ jobs:
8686
tool: nextest
8787
- name: Build
8888
run: cargo build --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
89-
- name: Test
89+
- name: Test [non-windows]
90+
if: ${{ matrix.os != 'windows-latest' }}
91+
run: |
92+
export TEST_GIT_EXECUTABLE_PATH="$(which git)";
93+
cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
94+
env:
95+
RUST_BACKTRACE: 1
96+
CARGO_TERM_COLOR: always
97+
- name: Test [windows]
98+
if: ${{ matrix.os == 'windows-latest' }}
9099
run: cargo nextest run --workspace --all-targets --verbose ${{ matrix.cargo_flags }}
91100
env:
101+
TEST_GIT_EXECUTABLE_PATH: 'C:\Program Files\Git\bin\git.exe'
92102
RUST_BACKTRACE: 1
93103
CARGO_TERM_COLOR: always
94104

‎CHANGELOG.md

+5
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
6262

6363
### New features
6464

65+
* `jj git {push,clone,fetch}` can now spawn an external `git` subprocess, via the `git.subprocess = true` config knob.
66+
6567
* `jj evolog` and `jj op log` now accept `--reversed`.
6668

6769
* `jj restore` now supports `-i`/`--interactive` selection.
@@ -100,6 +102,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
100102

101103
### Fixed bugs
102104

105+
* Fixes SSH bugs when interacting with Git remotes due to `libgit2`'s limitations.
106+
[#4979](https://github.com/jj-vcs/jj/issues/4979)
107+
103108
* Fixed diff selection by external tools with `jj split`/`commit -i FILESETS`.
104109
[#5252](https://github.com/jj-vcs/jj/issues/5252)
105110

‎cli/src/commands/git/clone.rs

+30-11
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ use crate::command_error::user_error_with_message;
3434
use crate::command_error::CommandError;
3535
use crate::commands::git::maybe_add_gitignore;
3636
use crate::git_util::absolute_git_url;
37+
use crate::git_util::get_config_git_executable_path;
3738
use crate::git_util::get_git_repo;
3839
use crate::git_util::map_git_error;
3940
use crate::git_util::print_git_import_stats;
@@ -83,6 +84,13 @@ fn is_empty_dir(path: &Path) -> bool {
8384
}
8485
}
8586

87+
struct CloneArgs<'a> {
88+
depth: Option<NonZeroU32>,
89+
remote_name: &'a str,
90+
source: &'a str,
91+
wc_path: &'a Path,
92+
}
93+
8694
pub fn cmd_git_clone(
8795
ui: &mut Ui,
8896
command: &CommandHelper,
@@ -115,14 +123,18 @@ pub fn cmd_git_clone(
115123
// `/some/path/.`
116124
let canonical_wc_path = dunce::canonicalize(&wc_path)
117125
.map_err(|err| user_error_with_message(format!("Failed to create {wc_path_str}"), err))?;
126+
let git_executable_path = get_config_git_executable_path(command)?;
118127
let clone_result = do_git_clone(
119128
ui,
120129
command,
130+
git_executable_path.as_deref(),
121131
args.colocate,
122-
args.depth,
123-
remote_name,
124-
&source,
125-
&canonical_wc_path,
132+
CloneArgs {
133+
depth: args.depth,
134+
remote_name,
135+
source: &source,
136+
wc_path: &canonical_wc_path,
137+
},
126138
);
127139
if clone_result.is_err() {
128140
let clean_up_dirs = || -> io::Result<()> {
@@ -177,12 +189,17 @@ pub fn cmd_git_clone(
177189
fn do_git_clone(
178190
ui: &mut Ui,
179191
command: &CommandHelper,
192+
git_executable_path: Option<&Path>,
180193
colocate: bool,
181-
depth: Option<NonZeroU32>,
182-
remote_name: &str,
183-
source: &str,
184-
wc_path: &Path,
194+
clone_args: CloneArgs<'_>,
185195
) -> Result<(WorkspaceCommandHelper, GitFetchStats), CommandError> {
196+
let CloneArgs {
197+
depth,
198+
remote_name,
199+
source,
200+
wc_path,
201+
} = clone_args;
202+
186203
let settings = command.settings_for_new_workspace(wc_path)?;
187204
let (workspace, repo) = if colocate {
188205
Workspace::init_colocated_git(&settings, wc_path)?
@@ -201,10 +218,11 @@ fn do_git_clone(
201218
let git_settings = workspace_command.settings().git_settings()?;
202219
let mut fetch_tx = workspace_command.start_transaction();
203220

204-
let stats = with_remote_git_callbacks(ui, None, |cb| {
221+
let stats = with_remote_git_callbacks(ui, None, git_executable_path.is_some(), |cb| {
205222
git::fetch(
206223
fetch_tx.repo_mut(),
207224
&git_repo,
225+
git_executable_path,
208226
remote_name,
209227
&[StringPattern::everything()],
210228
cb,
@@ -213,11 +231,12 @@ fn do_git_clone(
213231
)
214232
})
215233
.map_err(|err| match err {
216-
GitFetchError::NoSuchRemote(_) => {
217-
panic!("shouldn't happen as we just created the git remote")
234+
GitFetchError::NoSuchRemote(repo_name) => {
235+
user_error(format!("Could not find repository at '{repo_name}'"))
218236
}
219237
GitFetchError::GitImportError(err) => CommandError::from(err),
220238
GitFetchError::InternalGitError(err) => map_git_error(err),
239+
GitFetchError::Subprocess(err) => user_error(err),
221240
GitFetchError::InvalidBranchPattern => {
222241
unreachable!("we didn't provide any globs")
223242
}

‎cli/src/commands/git/fetch.rs

+10-1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use crate::cli_util::CommandHelper;
2323
use crate::command_error::CommandError;
2424
use crate::commands::git::get_single_remote;
2525
use crate::complete;
26+
use crate::git_util::get_config_git_executable_path;
2627
use crate::git_util::get_git_repo;
2728
use crate::git_util::git_fetch;
2829
use crate::ui::Ui;
@@ -69,6 +70,7 @@ pub fn cmd_git_fetch(
6970
args: &GitFetchArgs,
7071
) -> Result<(), CommandError> {
7172
let mut workspace_command = command.workspace_helper(ui)?;
73+
let git_executable_path = get_config_git_executable_path(command)?;
7274
let git_repo = get_git_repo(workspace_command.repo().store())?;
7375
let remotes = if args.all_remotes {
7476
get_all_remotes(&git_repo)?
@@ -78,7 +80,14 @@ pub fn cmd_git_fetch(
7880
args.remotes.clone()
7981
};
8082
let mut tx = workspace_command.start_transaction();
81-
git_fetch(ui, &mut tx, &git_repo, &remotes, &args.branch)?;
83+
git_fetch(
84+
ui,
85+
&mut tx,
86+
&git_repo,
87+
git_executable_path.as_deref(),
88+
&remotes,
89+
&args.branch,
90+
)?;
8291
tx.finish(
8392
ui,
8493
format!("fetch from git remote(s) {}", remotes.iter().join(",")),

‎cli/src/commands/git/push.rs

+18-3
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ use crate::command_error::CommandError;
5454
use crate::commands::git::get_single_remote;
5555
use crate::complete;
5656
use crate::formatter::Formatter;
57+
use crate::git_util::get_config_git_executable_path;
5758
use crate::git_util::get_git_repo;
5859
use crate::git_util::map_git_error;
5960
use crate::git_util::with_remote_git_callbacks;
@@ -178,6 +179,7 @@ pub fn cmd_git_push(
178179
args: &GitPushArgs,
179180
) -> Result<(), CommandError> {
180181
let mut workspace_command = command.workspace_helper(ui)?;
182+
let git_executable_path = get_config_git_executable_path(command)?;
181183
let git_repo = get_git_repo(workspace_command.repo().store())?;
182184

183185
let remote = if let Some(name) = &args.remote {
@@ -357,9 +359,22 @@ pub fn cmd_git_push(
357359
let mut sideband_progress_callback = |progress_message: &[u8]| {
358360
_ = writer.write(ui, progress_message);
359361
};
360-
with_remote_git_callbacks(ui, Some(&mut sideband_progress_callback), |cb| {
361-
git::push_branches(tx.repo_mut(), &git_repo, &remote, &targets, cb)
362-
})
362+
363+
with_remote_git_callbacks(
364+
ui,
365+
Some(&mut sideband_progress_callback),
366+
git_executable_path.is_some(),
367+
|cb| {
368+
git::push_branches(
369+
tx.repo_mut(),
370+
&git_repo,
371+
git_executable_path.as_deref(),
372+
&remote,
373+
&targets,
374+
cb,
375+
)
376+
},
377+
)
363378
.map_err(|err| match err {
364379
GitPushError::InternalGitError(err) => map_git_error(err),
365380
GitPushError::RefInUnexpectedLocation(refs) => user_error_with_hint(

‎cli/src/config-schema.json

+9
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,15 @@
377377
"type": "boolean",
378378
"description": "Whether jj should sign commits before pushing",
379379
"default": "false"
380+
},
381+
"subprocess": {
382+
"type": "boolean",
383+
"description": "Whether jj spawns a git subprocess for network operations (push/fetch/clone)",
384+
"default": false
385+
},
386+
"path": {
387+
"type": "string",
388+
"description": "Path to the git executable"
380389
}
381390
}
382391
},

‎cli/src/git_util.rs

+53-21
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ use std::time::Instant;
2929
use crossterm::terminal::Clear;
3030
use crossterm::terminal::ClearType;
3131
use itertools::Itertools;
32+
use jj_lib::config::ConfigGetError;
3233
use jj_lib::fmt_util::binary_prefix;
3334
use jj_lib::git;
3435
use jj_lib::git::FailedRefExport;
@@ -47,6 +48,7 @@ use jj_lib::workspace::Workspace;
4748
use unicode_width::UnicodeWidthStr;
4849

4950
use crate::cleanup_guard::CleanupGuard;
51+
use crate::cli_util::CommandHelper;
5052
use crate::cli_util::WorkspaceCommandTransaction;
5153
use crate::command_error::cli_error;
5254
use crate::command_error::user_error;
@@ -282,29 +284,40 @@ type SidebandProgressCallback<'a> = &'a mut dyn FnMut(&[u8]);
282284
pub fn with_remote_git_callbacks<T>(
283285
ui: &Ui,
284286
sideband_progress_callback: Option<SidebandProgressCallback<'_>>,
287+
subprocess: bool,
285288
f: impl FnOnce(git::RemoteCallbacks<'_>) -> T,
286289
) -> T {
287-
let mut callbacks = git::RemoteCallbacks::default();
288-
let mut progress_callback = None;
289-
if let Some(mut output) = ui.progress_output() {
290-
let mut progress = Progress::new(Instant::now());
291-
progress_callback = Some(move |x: &git::Progress| {
292-
_ = progress.update(Instant::now(), x, &mut output);
293-
});
290+
if subprocess {
291+
// TODO: with git2, we are able to display progress from the data that is given
292+
// With the git processes themselves, this is significantly harder, as it
293+
// requires parsing the output directly
294+
//
295+
// In any case, this would be the place to add that funcionalty
296+
f(git::RemoteCallbacks::default())
297+
} else {
298+
let mut callbacks = git::RemoteCallbacks::default();
299+
let mut progress_callback = None;
300+
if let Some(mut output) = ui.progress_output() {
301+
let mut progress = Progress::new(Instant::now());
302+
progress_callback = Some(move |x: &git::Progress| {
303+
_ = progress.update(Instant::now(), x, &mut output);
304+
});
305+
}
306+
callbacks.progress = progress_callback
307+
.as_mut()
308+
.map(|x| x as &mut dyn FnMut(&git::Progress));
309+
callbacks.sideband_progress =
310+
sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8]));
311+
let mut get_ssh_keys = get_ssh_keys; // Coerce to unit fn type
312+
callbacks.get_ssh_keys = Some(&mut get_ssh_keys);
313+
let mut get_pw =
314+
|url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url));
315+
callbacks.get_password = Some(&mut get_pw);
316+
let mut get_user_pw =
317+
|url: &str| Some((terminal_get_username(ui, url)?, terminal_get_pw(ui, url)?));
318+
callbacks.get_username_password = Some(&mut get_user_pw);
319+
f(callbacks)
294320
}
295-
callbacks.progress = progress_callback
296-
.as_mut()
297-
.map(|x| x as &mut dyn FnMut(&git::Progress));
298-
callbacks.sideband_progress = sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8]));
299-
let mut get_ssh_keys = get_ssh_keys; // Coerce to unit fn type
300-
callbacks.get_ssh_keys = Some(&mut get_ssh_keys);
301-
let mut get_pw =
302-
|url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url));
303-
callbacks.get_password = Some(&mut get_pw);
304-
let mut get_user_pw =
305-
|url: &str| Some((terminal_get_username(ui, url)?, terminal_get_pw(ui, url)?));
306-
callbacks.get_username_password = Some(&mut get_user_pw);
307-
f(callbacks)
308321
}
309322

310323
pub fn print_git_import_stats(
@@ -629,16 +642,18 @@ pub fn git_fetch(
629642
ui: &mut Ui,
630643
tx: &mut WorkspaceCommandTransaction,
631644
git_repo: &git2::Repository,
645+
git_executable_path: Option<&Path>,
632646
remotes: &[String],
633647
branch: &[StringPattern],
634648
) -> Result<(), CommandError> {
635649
let git_settings = tx.settings().git_settings()?;
636650

637651
for remote in remotes {
638-
let stats = with_remote_git_callbacks(ui, None, |cb| {
652+
let stats = with_remote_git_callbacks(ui, None, git_executable_path.is_some(), |cb| {
639653
git::fetch(
640654
tx.repo_mut(),
641655
git_repo,
656+
git_executable_path,
642657
remote,
643658
branch,
644659
cb,
@@ -705,6 +720,23 @@ fn warn_if_branches_not_found(
705720
Ok(())
706721
}
707722

723+
pub(crate) fn get_config_git_subprocess(command: &CommandHelper) -> Result<bool, ConfigGetError> {
724+
command.settings().get_bool("git.subprocess")
725+
}
726+
727+
pub(crate) fn get_config_git_executable_path(
728+
command: &CommandHelper,
729+
) -> Result<Option<PathBuf>, ConfigGetError> {
730+
if get_config_git_subprocess(command)? {
731+
command
732+
.settings()
733+
.get::<PathBuf>("git.executable_path")
734+
.map(Some)
735+
} else {
736+
Ok(None)
737+
}
738+
}
739+
708740
#[cfg(test)]
709741
mod tests {
710742
use std::path::MAIN_SEPARATOR;

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

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
---
22
source: cli/tests/test_generate_md_cli_help.rs
33
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."
4+
snapshot_kind: text
45
---
56
<!-- BEGIN MARKDOWN-->
67

‎cli/tests/common/mod.rs

+17
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ impl Default for TestEnvironment {
8080
'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()'
8181
"#,
8282
);
83+
8384
env
8485
}
8586
}
@@ -280,6 +281,22 @@ impl TestEnvironment {
280281
self.env_vars.insert(key.into(), val.into());
281282
}
282283

284+
pub fn set_up_git_subprocessing(&self) {
285+
self.add_config("git.subprocess = true");
286+
287+
// add a git path from env: this is only used if git.subprocess = true
288+
if let Some(git_executable_path) = Self::get_git_executable_path() {
289+
self.add_config(format!(
290+
"git.executable_path = '{}'",
291+
git_executable_path.trim()
292+
));
293+
}
294+
}
295+
296+
pub fn get_git_executable_path() -> Option<String> {
297+
std::env::var("TEST_GIT_EXECUTABLE_PATH").ok()
298+
}
299+
283300
pub fn current_operation_id(&self, repo_path: &Path) -> String {
284301
let id_and_newline =
285302
self.jj_cmd_success(repo_path, &["debug", "operation", "--display=id"]);

0 commit comments

Comments
 (0)