Skip to content

Update libgit2 fork to v1.9.0#2683

Merged
lucasderraugh merged 10 commits into
git-up:masterfrom
Cykelero:update-libgit2-fork-to-1.9.0
May 20, 2025
Merged

Update libgit2 fork to v1.9.0#2683
lucasderraugh merged 10 commits into
git-up:masterfrom
Cykelero:update-libgit2-fork-to-1.9.0

Conversation

@Cykelero
Copy link
Copy Markdown
Contributor

@Cykelero Cykelero commented Mar 31, 2025

Summary

This PR updates the libgit2 fork to v1.9.0, and makes accompanying changes in the GitUp build config.
This PR is for the GitUp repo, and relies on a libgit2 branch currently in my fork of the repo.

This update:

  • Adds support for the core.manyFiles/index.skipHash Git options, which forced index version 4, and made the index unreadable to GitUpKit.
  • Replaces libssh2 with OpenSSH, as a SSH backend for libgit2. This means that .ssh/config files are now supported: complex setups where different keys are used depending on the domain/username should now work in GitUp, and integration with third-party SSH agents like 1Password should be simpler.
  • Many smaller changes since libgit 1.4.4

Of note, switching to OpenSSH seems to have broken the credentials callback, which doesn't called anymore when SSH auth fails.
GitUp already displayed a cryptic error in that case (Failed connecting to "origin" remote: authentication required but no callback set) but the error is now even less clear: Failed connecting to "origin" remote: could not read refs from remote repository.
This is something I'm still investigating.

Changes

In the libgit2 repo

The update-to-1.9.0 branch is a rebase of the gitup_libgit_updated branch onto the latest libgit2 v1.9.0 commit.

The first commit contains most of the fork's changes. Some conflicts were resolved while rebasing it. Code that was calling libssh2_session_set_timeout() to set the timeout to 30 has been dropped, as libssh2 is no longer used. (that function call was superseded by the git_socket_stream__timeout variable, too)

These commits have been dropped because they're now part of the main libgit2 tree:

  • “Cherry-pick of 3847522e86e9c65be674f1372cefefdbfbe9ba2b” (69bd034) (“filter: Fix Segfault”)
  • “GitUp: Fix revwalk to not give up when it hits a symbolic reference. Should investigate this further to figure out if this is a bug in mainline libgit2.” (3ade0d5)

A couple of fix commits have been added.

In the GitUpKit repo

This is what's listed in this PR.

Most of the changes are to the build settings of the embedded Third-Party Swift package, which is used to embed libgit2. Since libgit2 relies on CMake but the GitUpKit build does not run CMake, we need to inline resolved settings (included source files, and compiler options) in Package.swift.

Testing the PR

To test the PR locally, you need to fetch the libgit2 update branch from the other repo, before checking out this PR's branch:

git remote add cykelero git@github.com:Cykelero/gitup-libgit2.git
git fetch cykelero

libgit2 changed which one it embeds.
- Update source excludes (mostly, libgit2 source files were moved into new `libgit2, `util` and `cli` subfolders)
- Update compiler options to reflect changes in libgit2's CMake config
This replaces the libssh2 SSH backend, and enables support of .ssh/config.
We're not interested in these when working on GitUp itself.
These warnings are emitted *after* the actual libgit2 build, so these compiler options need to be in the project build settings, rather than the Third-Party package build settings.

This commit only adds -Wno-documentation-deprecated-sync to the each list; previously, we were using the default values, which are now explicit.
These tests failed if you'd set the default branch name to e.g. main in your global Git config.

This builds on “Specify `initial_head` when initializing a repository for tests so the global git config doesn't make it fail (git-up#988)” (aabbc8a).
@Cykelero Cykelero force-pushed the update-libgit2-fork-to-1.9.0 branch from 45d2a88 to 91abdb9 Compare March 31, 2025 18:54
@clicube
Copy link
Copy Markdown

clicube commented May 2, 2025

Hi,

I've built and tested the branch associated with this PR, and wanted to share my feedback.

Firstly, thank you for adding support for .ssh/config! Being able to use different GitHub accounts and authentication keys per repository was something I really needed, and this update is fantastic. I'm using a self-built version with 1Password, and it's working well in that regard.

However, I encountered an issue where GitUp would sometimes termination with Signal 13 (SIGPIPE) when libgit2 is involved. In my testing environment, this consistently occurred under these specific conditions:

  • When a remote branch has new updates.
  • Performing a fetch operation within GitUp.

Tracing the issue, it seems to happen around the call to git_remote_disconnect() within the _transfer() function (GCRemote.m:167). This eventually leads to the write() system call within libgit2's git_process_write() function (process.c, around line 517). The debugger message I receive is: Message from debugger: Terminated due to signal 13.

As a workaround, I added signal(SIGPIPE, SIG_IGN); to the initialize method in AppDelegate.m. This successfully prevents the termination by ignoring the SIGPIPE signal, allowing me to use the application continuously.

Interestingly, it looks like libgit2 has its own disable_signals() function which might be intended to handle SIGPIPE, but it doesn't seem to be preventing the termination in this specific scenario.

While my workaround with signal(SIGPIPE, SIG_IGN) seems effective for me, I must admit I'm not entirely sure about its potential side effects or if it's the correct long-term solution.

Thanks again for your work on this valuable feature!

This seems to happen whenever we pull from a remote with changes.
@Cykelero
Copy link
Copy Markdown
Contributor Author

Cykelero commented May 6, 2025

Thank you for investigating, and writing it all up!

Turns out, we implemented a similar solution in Retcon (a Git client that relies on GitUp's core framework—and that was exhibiting the same issue since switching over to this GitUp PR). It's encouraging that you ended up with the same fix when you looked into the bug. Also, the Retcon update has been in testing (and then publicly released) for a bit over a month, so the fix seems safe, as heavy-handed as it feels.

So, I've updated the PR to add the call to GitUp. (also, sorry, I could have done it sooner! I don't daily-drive GitUp, and the crash slipped my mind)

You found a bunch of context I wasn't aware of, too. It's interesting that libgit2 has something that looks like a solution but isn't working. If the signal ignore does become a problem at some point, that'll be worth looking into more deeply.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

Sorry for dragging my feet on this. I'll dedicate Friday morning to testing it out and hopefully merging.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

Sorry, meant to update on Friday. I'm going to live on the branch today just to make sure there isn't anything glaring, but the review looks good. Code itself looks great!

@Cykelero Do you want me to open a PR on the libgit2 side to pull in your work?

@Cykelero
Copy link
Copy Markdown
Contributor Author

That's great to hear, thank you!

As for updating the libgit2 repo, maybe you can just fetch the update-to-1.9.0 branch from my fork? That would be enough for this here PR to work, if I understand things right.

You could also hard-reset master to the update branch, but I think this wasn't done for the last update, not sure why.

@lucasderraugh
Copy link
Copy Markdown
Collaborator

@Cykelero Will do. Just wasn't sure if you wanted to open one yourself for any personal reasons. If not, I'm happy to move the code to the right place.

@Cykelero
Copy link
Copy Markdown
Contributor Author

Ahh, of course, thank you for the consideration! Plus, it'll serve as a bit of documentation, why not.

git-up/libgit2#36

@lucasderraugh lucasderraugh merged commit 6687b5f into git-up:master May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants