cargo: bump git2 to 0.19.0

[bnjmnt4n]

This includes a bump of libgit2 to v1.8.1.

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

[bnjmnt4n]

Hmm, I'm not very sure how to fix the Windows failure. (I don't have a Windows machine to debug this.)

[yuja]

Maybe we can apply dunce::simplified() to the clone source path (if it is local)? I don't think Git fully supports UNC-like paths.

[ErichDonGubler]

@yuja: Looks like the problem is twofold:

1. We can't use backslashes at all; I've only ever seen forward-slash paths in Git configuration on Windows.
2. We can't use UNC/NT verbatim paths at all, because those force backslashes.

So, we'll need to use dunce::simplified, guarantee we don't have a UNC path (or otherwise bail out), and then replace all backslashes with forward slashes, I think?

[bnjmnt4n]

I think this is likely a regression in libgit2, since we didn't touch the code on our side at all.

[ErichDonGubler]

@bnjmnt4n: Actually, you'll get errors in vanilla Git if you try to use UNC paths for a remote, too, though only when you try to actually use it:

$ git init
Initialized empty Git repository in <snip>

$ git remote add origin '\\?\C:\<snip>\jj\'

$ git fetch origin
hostname contains invalid characters
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

So this is likely validation in libgit2 catching up with git's.

[yuja]

1. We can't use backslashes at all; I've only ever seen forward-slash paths in Git configuration on Windows.

If that's what git CLI mandates, we would have to copy that.

FWIW, I think git config would support some form of backslash escapes (because \ is a valid filename on Unix), so Windows path might work if properly escaped in config file. It might be a bug of libgit2 that doesn't escape backslashes?

[lishaduck]

Just FYI, jj isn't installable via Homebrew anymore. I know because I just ran brew upgrade, which upgrades libgit2 to 1.8.1, and breaks this 🙃😞
Reinstalling works?

[kevboh]

I am also having trouble installing via homebrew. Uninstalling and reinstalling does not seem to work for me:

dyld[81655]: Library not loaded: /opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib
  Referenced from: <E8960ED2-AAE3-34EB-9B6F-057F96828A15> /opt/homebrew/Cellar/jj/0.19.0/bin/jj
  Reason: tried: '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file)
fish: Job 1, 'jj git init' terminated by signal SIGABRT (Abort)

[lishaduck]

I am also having trouble installing via homebrew. Uninstalling and reinstalling does not seem to work for me:

dyld[81655]: Library not loaded: /opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib
  Referenced from: <E8960ED2-AAE3-34EB-9B6F-057F96828A15> /opt/homebrew/Cellar/jj/0.19.0/bin/jj
  Reason: tried: '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file)
fish: Job 1, 'jj git init' terminated by signal SIGABRT (Abort)

Oh yes, I spoke too hastily 😉 It installs, but doesn't run.

Details

~ 
❯ jj --version                                                  
dyld[12839]: Library not loaded: /opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib
  Referenced from: <E8960ED2-AAE3-34EB-9B6F-057F96828A15> /opt/homebrew/Cellar/jj/0.19.0/bin/jj
  Reason: tried: '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/opt/libgit2/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file), '/opt/homebrew/Cellar/libgit2/1.8.1/lib/libgit2.1.7.dylib' (no such file)
zsh: abort      jj --version

[bnjmnt4n]

Here's the issue for Homebrew: Homebrew/homebrew-core#177686. The temporary fix would be to update the formula to point to [email protected], but of course the better, long-term fix would be to merge this PR in to use the latest libgit2. Unfortunately, due to my lack of experience with Windows UNC and lack of an actual Windows machine to test on, I don't think I will be able to help out much with the Windows test failure. Perhaps someone else can take over this PR?

[ErichDonGubler]

@bnjmnt4n: While the test in CI has technically regressed, I'm not sure that this is a regression that's notable for users. Users who succeeded in cloning wouldn't have been able to use their local Git remotes anyway, which is behavior that I don't think this upgrade changes. Is that something that should block this upgrade, as opposed to having a separate issue filed?

[bnjmnt4n]

I can disable the test on Windows to get this merged in, if that's what's desired by other contributors.

[ilyagr]

For future reference, here's the last CI run that demonstrates the error if the test is not disabled on Windows:

https://github.com/martinvonz/jj/actions/runs/10114947983/job/27974690965

---- test_git_push::test_git_push_to_remote_named_git stdout ----
thread 'test_git_push::test_git_push_to_remote_named_git' panicked at cli\tests\test_git_push.rs:1245:45:
called `Result::unwrap()` on an `Err` value: Error { code: -1, klass: 7, message: "invalid escape at C:\\Users\\runneradmin\\AppData\\Local\\Temp\\jj-test-ZYqVPK\\origin\\.jj\\repo\\store\\git" }
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library\std\src\panicking.rs:645
   1: core::panicking::panic_fmt
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library\core\src\panicking.rs:72
   2: core::result::unwrap_failed
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library\core\src\result.rs:1649
   3: core::result::Result<T,E>::unwrap
   4: runner::test_git_push::test_git_push_to_remote_named_git::{{closure}}
   5: runner::test_git_push::test_git_push_to_remote_named_git::{{closure}}
   6: core::ops::function::FnOnce::call_once
   7: core::ops::function::FnOnce::call_once
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    test_git_push::test_git_push_to_remote_named_git

test result: FAILED. 564 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 171.59s

error: test failed, to rerun pass `-p jj-cli --test runner`
Error: Process completed with exit code 1.

[ilyagr]

For my "diffedit3", I was initially going to use https://docs.rs/path-slash/latest/path_slash/ to make tests pass on Windows, but then eventually settled on:

    fn to_slash_string_lossy(path: &Path) -> String {
        path.to_string_lossy().replace('\\', "/")
    }

I only did this to make the test output consistent, and here we also need git2 to parse the result, but I'm still hoping that one of these might work here. IIUC, the problem is in

jj/cli/tests/test_git_push.rs

Line 1243 in 304f6df

git2::Repository::open(&git_repo_path).unwrap()

and the other uses of Path.join work on Windows for some reason (maybe because they only get passed to gix and not libgit2?). Thought, perhaps there will be more problems later if we don't fix some deeper underlying issue, I'm unsure.

[ilyagr]

I'm not sure whether it's better to merge this in before the release next week or to wait until after, but I'm leaning towards the latter. What do people think?

I think it's fine to disable the test for a short time if we fix it later, but I'm slightly worried if there are other changes in behavior that the new libgit2 will cause.

This also affects packagers (though I think it's not a huge consideration; they'll upgrade eventually). From a glance, Homebrew has both versions of libgit2 packaged, but Nixpkgs doesn't yet have libgit2 1.8 packaged. I'm not sure whether Nixpkgs will go the same route as Homebrew and support two versions at once.

[emilazy]

We could use this merging for Nixpkgs, if there aren’t any blockers left.

[ilyagr]

There's some stuff with Windows test, but I think it might be a good idea to merge it now to have time to see what other stuff (if any) breaks.

We have three options for the Windows test:

• Just disable it (what this PR does)
• test_git_push: make tests work on newer libgit2-s #4184 (Update: fixed link), which doesn't solve the root problem, but could probably be merge with a TODO.
• Something better like fixes for paths on Windows #4188. It doesn't work as is, but could likely be fixed, it just takes a bit of work to try fixes.

[emilazy]

(Link should be #4184, for anyone wondering.)

Disabling the test or your fix both sound like good options to me. But of course I’m biased; I want to keep the Jujutsu package in Nixpkgs building by including the patch :) #4234 suggests that it’s also an issue for Arch.

(Of course, there’s a separate discussion to be had about vendoring libgit2, but that should probably be done under less time pressure.)

[ilyagr]

Judging by #4184 (review), it sounds like @yuja would prefer this PR (with its TODO and disabled test) over merging #4184.

That's fine by me.

Yuya, is that right? WDYT?

I might do some more work on #4188, but I don't want to promise any timeline (and anybody else is welcome to pick it up).

[emilazy]

Could always merge this now and do #4184 as another incremental step later if desired, right? (I’m assuming that the test failure is not actually substantive in such a way as to consider Windows broken after this PR.)

[ilyagr]

Could always merge this now and do #4184 as another incremental step later

Yes.

I’m assuming that the test failure is not actually substantive in such a way as to consider Windows broken after this PR.

I'm... not actually sure. Judging by our tests, it's not too bad, but there also tend to be Windows-specific issues that aren't covered by our tests. One thought I'm having is "let's merge this now and see whether we get any complaints about something crucial not working on Windows anymore". There is a non-zero chance this PR would get reverted (Update: or perhaps reverted only on Windows), but it's not a high chance, and let's be optimistic :)

[emilazy]

It needs rebasing for conflicts, but if you want me to hit the merge button and take the blame for breaking Windows I’m up for it!

[ilyagr]

I would prefer it if @yuja and/or @bnjmnt4n took a look. I think there's a reason this is not approved yet, I'm unsure whether we are all on the same page.

(also, I'd like a "note to maintainers" as I said in another comment, but it's not crucial and could also go into a separate PR)

@bnjmnt4n, let us know if you want one of us to take care of hitting the merge button when this is approved. By default, it's pretty much @bnjmnt4n 's sole right to hit the merge this AFAICT unless he tells us wants one of us to or we decide to make an exception (not that these rules have to by followed religiously, but these are the rules I usually go by).

[emilazy]

(Sorry if resolving the conflict was not the done thing!)

[yuja]

Judging by #4184 (review), it sounds like @yuja would prefer this PR (with its TODO and disabled test) over merging #4184.

Yes. Suppose it's an existing bug and these PRs don't fix the underlying problem, I would choose the minimal workaround.

[alerque]

This PR’s merge commit doesn’t appear in any tags

v0.21.0 has been tagged since this PR was merged, but I tried to build it on Arch Linux with linking the system libgit. Having hit an error I came back here because I thought I remembered this landing on main, but GitHub seems to thing it didn't make the release cut. I'm a bit puzzled since this merged to main and the tag was also released on main so ... anybody know what gives?

[emilazy]

We are building Jujutsu 0.21.0 with libgit2 1.8.1 in Nixpkgs without any problems, so I suspect something is wrong on the Arch end. 38f6ee8 shows up as being part of v0.21.0 on GitHub for me.

[alerque]

Thanks for the point of reference @emilazy, I'm looking into it.

Edit: My linking issue actually turned out to be libssh2 related, not libgit2. Enabling the latter requires properly setting up the former, so enabling libgit2 linking was actually working but the libssh2 stuff it dragged in with it wasn't yet. I got that resolved.