git: spawn a separate git process for network operations #5228

bsdinis · 2025-01-02T01:38:22Z

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:

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

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

google-cla · 2025-01-02T01:38:26Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

yuja · 2025-01-02T09:07:12Z

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 unavoidable

That seems okay. It's also good to reorganize the current error enum as needed.

it is using a new feature flag shell to switch on to shelling out. This doesn't seem the best approach, and it would be great to get some feedback on what would be best. A flag on jj git + adding it to the jj config seems to be a good enough idea

I think it's best to add a config knob to turn the shelling-out backend on. If the implementation gets stable enough, we can change the default, and eventually remove the git2 backend. I think the flag can be checked by CLI layer, but that's not a requirement.
This refactoring PR #4960 might help as it splits fetch() into "fetch from remote" part and import_refs().

Regarding the code layout, maybe better to add new module for shelling-out implementation? The current git.rs isn't small, and there wouldn't be many codes to be shared.

Thanks!

bnjmnt4n · 2025-01-02T18:43:12Z

cli/tests/test_git_clone.rs

+#[duplicate_item(
+    test_git_clone shell_git_config;
+    [test_git_clone_shell] ["git.shell = true"];
+    [test_git_clone_git2] [""];
+)]


We currently already use test-case to parametrize various test cases in the codebase. I don't think it makes sense to pull in another dependency to do this.

this sounds amazing, will look into it

bsdinis · 2025-01-02T18:45:46Z

Hi! Thanks for the feedback!

In the meantime, I was trying to make CI pass and did away with the feature flag (which was always my intention anyways but wanted to receive feedback).

~~Regarding the knob being present in config, I would argue for it:~~

~~I'd basically always have this on, so passing --shell on every command seems impractical~~
I'm not sure removing git2 would be necessarily the best, it might get good support for the ssh problem and overall seems it'd follow the codebase either way. So keeping the knob in config would allow a way to specify intended behaviour

EDIT: I misread a comment where I thought it was saying no config knob. sorry about that

PhilipMetzger · 2025-01-02T18:57:29Z

Can you also squash it down to one commit and change the commit message/PR title to cli: shell out to git to conform to our commit message style outlined here https://github.com/jj-vcs/jj/blob/main/docs/contributing.md#code-reviews.

thoughtpolice · 2025-01-02T19:31:49Z

I'm not sure removing git2 would be necessarily the best, it might get good support for the ssh problem and overall seems it'd follow the codebase either way. So keeping the knob in config would allow a way to specify intended behaviour

Getting rid of libgit2 is likely inevitable at this rate. It is only used for push/pull and is a large 3rd party dependency that we have probably outgrown by now, I'm almost certain the number one most-repeated bug report is "push does not work", with many users compiling themselves to fix. In reality I think it will probably remain a long tail of issues, things that won't work right, because the existing Git code, tools, and installers have a lot of quirks figured out for many various OS/workflow combinations, e.g. something like Git for Windows Credential Manager, or auto-ssh key unlock via secure enclaves, etc.

In the future if it supported this stuff really well, bringing back the code probably wouldn't be too bad. Or maybe Gitoxide will get good support, which would be even better. But in the meantime, assuming this improves the user experience now, and assuming we turn it on by default at some point, it's probably no longer worth keeping git2 around.

emilazy · 2025-01-02T19:50:48Z

Thanks for taking up this task! I’ll try to do a more thorough review later but for now I wanted to mention that this should use the fancy Git --force-with-lease=<refname>:<expect> option to preserve the current behaviour of only pushing to a ref if it matches Jujutsu’s expectation for the bookmark, as discussed on the Discord a while ago. I also agree with Yuya that it makes sense to base this on the refactors in #4960.

(Also +1 to getting rid of git2 – I think this should be a configuration option, at first defaulting to git2, then the next release to shelling out, then hopefully we can just get rid of the old path entirely the release after if all goes well. In the long run, Gitoxide growing proper push support or Git librarification may offer nicer options that don’t involve shelling out, but I doubt that libgit2 will ever be the right thing for us, based on our extensive experience using it so far, and as long as we don’t run into any unexpected blockers with shelling out I don’t think it makes sense to keep a C dependency on an entirely separate Git implementation just for “jj git {fetch,push}, but worse”.)

arxanas

Exciting work!

arxanas · 2025-01-02T19:35:29Z

cli/src/commands/git/clone.rs

+    /// Run git clone as a forked git process
+    #[arg(long)]
+    shell: bool,


suggestion: I'd recommend you actually drop the --shell argument altogether (from here and elsewhere). For one-off invocations, I believe you can write something like jj --config='git.shell = true git push. I agree with your comment in the top-level thread that it seems unlikely to want to toggle it frequently, so using the more verbose --config` version of the command seems like an acceptable trade-off vs polluting the command-line interface.

arxanas · 2025-01-02T19:38:32Z

cli/tests/test_git_clone.rs

+// this test has slightly different behaviour from git2, because it doesn't support shallow clones
+// on a local transport


comment: IIRC you can get it to support shallow clones if you use a file URI instead of just a path (like file:///path/to/repo). Is there a specific reason to prefer to do or not do that?

This refers to

jj/cli/tests/test_git_clone.rs

Line 597 in cc01531

// local transport does not support shallow clones so we just test that the

Basically, the git2 implementation errors out here, because of the underlying git2 behaviour regarding shallow clones on local transports. In its test this is fine, but git cannot replicate the erroneous git2 behaviour, hence why the tests are manually separated out (instead of using test_case)

arxanas · 2025-01-02T19:39:42Z

lib/src/git.rs

+// there are two options:
+//
+// a bare git repo, which has a parent named .jj that sits on the workspace root
+// or
+// a colocated .git dir, which is already on the workspace root


suggestion (non-blocking): I think it's probably better to promote this to a doc-comment (///)? (Assuming that this function makes it into the final version.)

arxanas · 2025-01-02T19:42:13Z

lib/src/git.rs

+        let mut it = git_dir.ancestors();
+        for path in it.by_ref() {
+            if path.file_name() == Some(OsStr::new(".jj")) {
+                break;
+            }
+        }
+
+        it.next().map(|x| x.to_path_buf()).ok_or(format!(
+            "could not find .jj dir in git dir path: {}",
+            git_dir.display()
+        ))


suggestion: Perhaps you want something like it.skip_while(/* isn't .jj */).next()? (Assuming that this function makes it into the final version.)

arxanas · 2025-01-02T19:46:07Z

lib/src/git.rs

+        // about it on clone.
+
+        let default_branch = fork_git_remote_show(git_dir, &work_tree, remote_name)?;
+        tracing::debug!(default_branch = default_branch);


suggestion: I'd include a message in the debug! here to make it easier to understand what operation it's associated with.

All the spawn_ functions start with a tracing::debug! which includes the command it's going to fork to. Are you suggesting adding something here too?

arxanas · 2025-01-02T19:49:53Z

lib/src/git.rs

+        git_err
+    };
+    if let Some(e) = git_err {
+        return e;


suggestion: It seems like you don't need to assign git_err to a variable, and can just return it directly on line 2271?

arxanas · 2025-01-02T19:51:56Z

lib/src/git.rs

+) -> Result<(), GitFetchError> {
+    let depth_arg = depth.map(|x| format!("--depth={x}"));
+    tracing::debug!(
+        "shelling out to `git --git-dir={} --work-tree={} fetch --prune{} {} {}",


suggestion: You may want to print this as part of regular output.
issue: I would strongly recommend constructing the args vec and then logging/rendering/printing from it directly, so that there's a single source of truth. Otherwise, we risk having the reported command invocation and the actual command invocation falling out of sync, which will be very confusing to debug. (You can use cmd.args instead of cmd.arg to provide a list of arguments. You can also use cmd.get_args to get the list of arguments it's about to be called with for reporting purposes, if you don't want to maintain a separate variable.)

+1 on constructing it from the args

on printing it as part as the regular output, this would break all the tests, as they rely on the output matching. Any suggestions on how to work around that?

I personally don’t think that printing it by default is a good idea. I think we should just try and match the existing jj git push UX as closely as possible; I was even thinking about passing verbose flags to parse the terminal output for progress bar updates. By the way you’ll probably also have an easier time parsing the final output if you pass --porcelain, since Git doesn’t guarantee stable machine‐readable output otherwise. (Sorry if some of this is already stuff you’re doing, just sending scattershot thoughts from when I was looking into this before I can give the code a proper read‐through!)

The only two commands where we are getting output from currently (git remote show and git ls-remote) don't have a porcelain flag. We ignore the stdout output from both fetch, push and branch.

To be honest, the main problem parsing is with errors, where --porcelain doesn't really help.

Progress bar updates (to match the current UX) sound really hard.
Not sure how to live parse it (especially with the \r being thrown around). I checked if porcelain would help there, but it doesn't.

I wasn’t meaning to suggest you try to parse the updates; it’s something I was interested in doing but it’s certainly gnarly and not required for an MVP :)

I’m surprised you don’t use the final output from git fetch, though.

On success it's not a problem, only need to do it for errors, which are not affected by --porcelain :-)

It's also due to the fact it fetches one at a time, so we can check success on each one of them

arxanas · 2025-01-02T19:53:44Z

lib/src/git.rs

+    }?;
+
+    let output = remote_git.wait_with_output()?;
+    let _ = convert_git_fetch_output_to_fetch_result(output, prunes)?;


suggestion: I would annotate the type of the result variable here (or use the result), as it can catch issues when the return value of the function changes and now it's meant to be used.

arxanas · 2025-01-02T19:54:21Z

lib/src/git.rs

+        return Ok(output);
+    }
+
+    let lossy_err = String::from_utf8_lossy(&output.stderr).to_string();


comment: A lot of the error-detection here seems to be duplicated with other functions in this file; maybe worth unifying in the final version.

Part of the reason for getting it split like this was due to the fact that some commands issue different errors (while others issue the same). I've reworked it to address your comments but feel that having them separate by call is the right move. If we have a particular error we want to handle that comes from only one git command we can then parse it out just there.

arxanas · 2025-01-02T19:55:12Z

lib/src/git.rs

+    )))
+}
+
+fn fork_git_fetch(


question: Why are these function called fork_*? Does fork here just mean "spawn a subprocess"? I might call them spawn_* in that case.

no good reason: renamed

bsdinis · 2025-01-02T20:53:47Z

Thanks for taking up this task! I’ll try to do a more thorough review later but for now I wanted to mention that this should use the fancy Git --force-with-lease=<refname>:<expect> option to preserve the current behaviour of only pushing to a ref if it matches Jujutsu’s expectation for the bookmark, as discussed on the Discord a while ago.

I looked into this, but am unsure this completely mimics the behaviour as it stands right now. The patch uses git ls-remote to find the remote ref and then uses the existing logic to figure out if jj accepts it or not

bsdinis · 2025-01-02T20:56:09Z

This refactoring PR #4960 might help as it splits fetch() into "fetch from remote" part and import_refs().

I agree. In fact, there is a current clippy warning about a function with too many arguments that is hard to go around without it.

PS: I intend to work on the remaining issues on top of main and then rebasing onto the PR when that's the only thing left

emilazy · 2025-01-02T20:56:12Z

The version with the argument lets you specify the exact expected commit for each remote ref, so you should be able to implement the current logic on top of it. I think git ls-remote won’t work correctly as it introduces a race condition.

martinvonz · 2025-01-02T23:34:59Z

cli/src/commands/git/clone.rs

-        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}'"))


nit: i think we usually start messages with uppercase

martinvonz · 2025-01-02T23:40:31Z

cli/src/git_util.rs

+
+pub(crate) fn get_config_git_shell(command: &CommandHelper) -> Result<bool, ConfigGetError> {
+    command
+        .config_env()


Use .settings() instead. raw_config()'s help says "Use this only if the unmodified config data is needed."

thanks so much! ran around for a long time looking for exactly this and used raw_config as a last resort

martinvonz · 2025-01-03T00:32:35Z

lib/src/git_shell.rs

nit: I'm not sure if "shell out" necessarily means subprocessing to a shell, but "subprocess" seems clearer either way, so maybe git_subprocess.rs?

do you think this should be done patch-wise? the config + variable names + function names + test names all refer to this process as shelling out

I think we should do it consistently. So if we decide to not call it "shell out", then we should consistently not call it that. The alternative is that we decide that it's okay to call it that even if it's inaccurate.

I think, a possible approach is:

the functions that spawn a git process would be, e.g., GitSubprocessContext::spawn_fetch

rework other function names from shell --> subprocess

joyously · 2025-01-03T01:30:59Z

Does this share functionality with #4759 ? At a surface level, it seems similar.

bsdinis · 2025-01-03T02:43:14Z

Does this share functionality with #4759 ? At a surface level, it seems similar.

I don't think it does. This PR is about using git as a subprocess for remote network operations, instead of the git2 library. The mentioned PR seems to be about more generic exec capabilities (if I understand correctly, related with the scripting language).

bsdinis · 2025-01-03T03:07:29Z

Re: --force-with-lease.
It worked better than I expected, but there is a (slight) mismatch in behaviour.

The particular test case that fails regards if we are pushing a branch deletion, and the branch was already deleted on the remote (and as such, is not at the expected commit) jj accepts it (with PushAllowReason::UnexpectedNoop) whilst git does not. There are a couple things we could do here:

Accept the differing behaviour;
Make jj error out here.

Would be great to get perspective on what to do here!

cc @emilazy

yuja · 2025-01-03T07:06:20Z

Re: --force-with-lease. It worked better than I expected, but there is a (slight) mismatch in behaviour.

Accept the differing behaviour;

Make jj error out here.

I think minor behavior difference is acceptable if that reduces the complexity. (We can also start with a simple implementation, and fill a gap later if needed.)

martinvonz · 2025-01-04T13:45:50Z

cli/src/commands/git/clone.rs

cli: shell out to git for network operations

Nit on the commit message: This feature is not just about the CLI. I'd say the most important changes are in the library. "git" seems like a better topic.

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

bnjmnt4n reviewed Jan 2, 2025

View reviewed changes

arxanas reviewed Jan 2, 2025

View reviewed changes

bsdinis force-pushed the shell-out branch from 807bc10 to 9eae40b Compare January 2, 2025 20:01

bsdinis force-pushed the shell-out branch from 9eae40b to 7eaf92c Compare January 2, 2025 21:01

bsdinis changed the title ~~feat(shell-out): allow shelling out to git for network related calls~~ cli: shell out to git Jan 2, 2025

bsdinis changed the title ~~cli: shell out to git~~ cli: shell out to git for network operations Jan 2, 2025

bsdinis force-pushed the shell-out branch 4 times, most recently from daa498e to 5696825 Compare January 2, 2025 22:48

martinvonz reviewed Jan 3, 2025

View reviewed changes

bsdinis force-pushed the shell-out branch from 5696825 to fe605df Compare January 3, 2025 02:41

bsdinis force-pushed the shell-out branch 6 times, most recently from 9a733d2 to bb5e58c Compare January 3, 2025 04:51

bsdinis force-pushed the shell-out branch 3 times, most recently from 6ccb197 to 16004f1 Compare January 3, 2025 05:48

bsdinis force-pushed the shell-out branch 3 times, most recently from b4a0bcf to 6a1ea14 Compare January 4, 2025 04:55

martinvonz reviewed Jan 4, 2025

View reviewed changes

bsdinis changed the title ~~cli: shell out to git for network operations~~ git: spawn a separate git process for network operations Jan 4, 2025

bsdinis force-pushed the shell-out branch from 6a1ea14 to 19eb17a Compare January 4, 2025 17:38

bnjmnt4n mentioned this pull request Jan 5, 2025

SSH: Use OpenSSH instead of libssh2 for authentication with Git hosts #3191

Draft

4 tasks

bsdinis force-pushed the shell-out branch 6 times, most recently from 1e8cbe7 to fccd31b Compare January 6, 2025 16:57

fix windows workflow

ab55bb0

bsdinis force-pushed the shell-out branch from fccd31b to ab55bb0 Compare January 6, 2025 17:12

		// this test has slightly different behaviour from git2, because it doesn't support shallow clones
		// on a local transport

git: spawn a separate git process for network operations #5228

Are you sure you want to change the base?

git: spawn a separate git process for network operations #5228

Conversation

bsdinis commented Jan 2, 2025 • edited Loading

Reasoning:

Implementation Details:

Testing:

Build:

Clone a private repo:

Create new commit and push

Checklist

google-cla bot commented Jan 2, 2025

yuja commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis commented Jan 2, 2025 • edited Loading

PhilipMetzger commented Jan 2, 2025

thoughtpolice commented Jan 2, 2025 • edited Loading

emilazy commented Jan 2, 2025 • edited Loading

arxanas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis commented Jan 2, 2025

bsdinis commented Jan 2, 2025 • edited Loading

emilazy commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdinis Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

joyously commented Jan 3, 2025

bsdinis commented Jan 3, 2025

bsdinis commented Jan 3, 2025 • edited Loading

yuja commented Jan 3, 2025

Choose a reason for hiding this comment

bsdinis commented Jan 2, 2025 •

edited

Loading

bsdinis commented Jan 2, 2025 •

edited

Loading

thoughtpolice commented Jan 2, 2025 •

edited

Loading

emilazy commented Jan 2, 2025 •

edited

Loading

bsdinis Jan 2, 2025 •

edited

Loading

bsdinis Jan 2, 2025 •

edited

Loading

bsdinis Jan 3, 2025 •

edited

Loading

bsdinis Jan 2, 2025 •

edited

Loading

bsdinis commented Jan 2, 2025 •

edited

Loading

bsdinis Jan 4, 2025 •

edited

Loading

bsdinis commented Jan 3, 2025 •

edited

Loading