Use libgit2 directly instead of ObjectiveGit - #727
Open
mssun wants to merge 16 commits into
Open
Conversation
ObjectiveGit types formed part of the public API of passKit: GTCommit and GTSignature crossed PasswordStore, the clone, pull and push options were untyped dictionaries carrying a GTCredentialProvider, and the progress handlers took raw libgit2 pointers. passKit re-exports ObjectiveGit, so the controllers used GTSignature, git_transfer_progress and GTPullMergeConflictedFiles without importing anything, which hid the coupling from a search for imports. Introduce value types in GitTypes.swift -- GitCommit, GitSignature and the three progress structs -- and express the public surface of GitRepository in terms of them. The option dictionaries become an opaque GitCredentialOptions, and signature validation and merge conflict details are reached through GitSignature.isValid and GitError.mergeConflictPaths, so that the two remaining rules of libgit2 they depend on stay in the backend. Conversion lives in a bridging section at the bottom of GitRepository.swift. Progress fractions are computed by the progress types instead of at every call site, which also avoids dividing by a total that is still zero. ObjectiveGit is now imported by GitRepository.swift and GitCredential.swift alone, plus their tests, which exercise that layer directly. Replacing the backend no longer reaches beyond those files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Objective-C wrapper is no longer used. GitRepository and GitCredential now call libgit2 directly, and Libgit2.swift holds what the C API needs around it: the one-time library initialisation, translation of return codes into errors carrying the message libgit2 produced, conversion of commits and signatures into the value types, and the callback bridge that carries the Swift progress and credential handlers through the void pointer payload of the libgit2 callbacks. libgit2 has no pull, so it is assembled here: fetch, then merge analysis, then either nothing when already up to date, a reference update when the merge is a fast-forward, or a merge commit. Conflicts abort the merge, clean up the repository state and report the conflicting paths, as before. Push reports references the remote refused, which the wrapper did and nothing in the app noticed until now. Credentials no longer depend on the backend at all. GitCredential produces a value describing what to authenticate with, and only the callback in Libgit2.swift turns that into a git_cred. A user name is answered when libgit2 asks for one before the credential itself, which the ObjectiveGit provider never did. The callback context is passed to libgit2 unretained, so every call that uses it runs inside withExtendedLifetime. Errors are Swift values now and carry no NSUnderlyingErrorKey, so the sync error handler reads the libgit2 message itself when looking for a rejected SSH passphrase, and takes the conflicting paths from the error rather than from a userInfo key. The libgit2 headers still come from the ObjectiveGit framework, which bundles the library. Building libgit2 separately is what remains to drop the dependency; no Swift outside these files is involved in that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the build that has to replace the prebuilt ObjectiveGit framework, which is where libgit2 currently comes from. It produces an xcframework holding libgit2 statically linked against libssh2 and the OpenSSL libcrypto libssh2 needs, for arm64 and both simulator architectures, alongside the git2 headers and a module map so that it can be imported from Swift. Output lands in a gitignored directory, as the gopenpgp build next to it does. The configuration follows what the framework being replaced actually does, read off its symbol table rather than assumed: HTTPS goes through SecureTransport, so OpenSSL is only ever reached by the SSH transport. That keeps ed25519 keys and keys held in memory working, and avoids having to ship a certificate bundle, which using OpenSSL for TLS would require since it cannot reach the system trust store. Two settings are not the obvious ones. The architecture is selected by picking the matching OpenSSL target rather than by passing -arch, because Configure reads an argument that does not start with a dash as a second target and fails. And the regular expression backend is the PCRE bundled with libgit2, because regcomp_l, which CMake chooses by default on Apple platforms, is marked unavailable on iOS. libgit2 is pinned to 1.9.6, which still provides the names the git layer uses -- git_cred_*, git_transfer_progress and the *_init_options functions -- as deprecated aliases. Building it with DEPRECATE_HARD would remove them, so it is left off deliberately. Nothing consumes the xcframework yet; the project still links ObjectiveGit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The project now links the xcframework built by scripts/libgit2_build.sh, referenced from its build directory the way the GopenPGP one next to it is, and the ObjectiveGit package is gone. It is linked into passKit and the two extensions, which is where the git layer actually lives; the package had been attached to the app target instead. libgit2 is built with iconv and zlib, so both are linked alongside it. Nothing complained about zlib because another dependency happened to pull it in, which is not something to rely on. Now that the libgit2 version is ours to choose, the git layer uses the current names rather than the ones kept for compatibility: git_indexer_progress, git_credential_* and the *_options_init functions. This was forced by GIT_CREDENTIAL_USERNAME, whose compatibility spelling is a macro over an enum constant and therefore invisible to Swift, but the rest are renamed too so that nothing depends on deprecated aliases staying available. The bridging header of the app target existed only to import ObjectiveGit and is now empty. The build setting pointing at it is left in place. libgit2, libssh2 and OpenSSL replace ObjectiveGit in the list of open source components, as all three are now linked into the app. Building the app needs scripts/libgit2_build.sh to have been run, as it already needs the GopenPGP one; the README says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
libgit2 caps its SecureTransport stream at TLS 1.2. Raising it looks like a one line change, but SecureTransport on iOS rejects kTLSProtocol13 with errSSLIllegalParam, and libgit2 sets the maximum version inside a chain of calls it aborts the connection on, so the patch would not fall back to TLS 1.2 -- it would stop every HTTPS remote from connecting. Measured on the simulator, where a handshake with a host that offers TLS 1.3 settles on 1.2 in any case. None of the tests would have caught it, as they all use file:// remotes. Reaching TLS 1.3 means using OpenSSL for HTTPS and shipping a CA bundle with the app, since OpenSSL cannot read the trust store of the system. That trade is written down next to the option it applies to. Also stop rebuilding OpenSSL on every run. It is pinned and takes longer than everything else combined, so an existing install is now reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both workflows built GopenPGP but nothing built libgit2, so every run would have failed on the missing xcframework as soon as Xcode opened the project. Only the finished xcframework is cached, not the whole build tree as the GopenPGP step does. The tree is 666M, almost all of it OpenSSL sources and object files, against 35M for the framework itself, and nothing outside libgit2/dist is needed to build the app -- verified by building and testing with every other directory moved away, which is the state a cache hit leaves behind. CMake is already on the macOS runners, so no tooling step is needed. The prepare lane builds libgit2 too, so that it still does what its name says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lures A merge that is not carried through to a commit left conflicts in the index and a half-merged work tree. Nothing cleaned that up, so every later commit failed in git_index_write_tree with "cannot create a tree from a not fully merged index", and the app could no longer save any password change until the user found the discard action. The same held when the merge commit itself could not be created, with the added twist that MERGE_HEAD survived and the next commit recorded the merged content against HEAD alone, losing the second parent. The merge is now undone unless it reaches a commit, which resets the index and work tree to HEAD and leaves the local commits alone. A push the remote refuses is reported since this branch, and the sync handler deleted the stored password for any error at all, so a protected branch or a push race threw away a credential the remote had just accepted. Only failures that can plausibly be blamed on the credential delete it now. The credential callback returned -1 without leaving a message, so cancelling the passphrase prompt surfaced whatever an earlier operation had put in the error slot of libgit2, or "Git error -1". It now says what happened. git_error_set_str lives in a header git2.h does not include, so the module map exposes it and the framework has to be built again. Also assign the branch name only once the branch is known to exist, and validate the address in the email field rather than the name twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SwiftLint build phase only passes --strict when CI is set, so these two violations failed the workflow while local builds stayed green. The callbacks that use these members are file scope functions rather than members of the class, so private does not reach them; internal does, and the class is internal already, so nothing is exposed that was not before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
libgit2 verifies the host key of an SSH remote against a known_hosts file and fails the connection when it finds no match. iOS has no such file, so every SSH remote failed with "invalid or unknown remote ssh hostkey" -- the whole SSH transport was broken, and nothing noticed because the tests all talk to a repository on disk. The 0.28 inside the wrapper this branch replaces did not verify host keys at all, so accepting them restores the behaviour the app has always had. The callback passes through for everything that is not an SSH host key, so libgit2 keeps the verdict it reached for a TLS certificate and an untrusted HTTPS remote is still refused. Verifying host keys properly would mean storing them and asking the user on first connect, which is worth doing but is a feature, not part of swapping the backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every test so far cloned from a repository on disk, so libssh2, the TLS stream of libgit2 and the credential callbacks that only remote operations reach had no coverage at all. That is how a broken SSH transport reached this branch unnoticed. scripts/git_servers.sh starts an sshd and a git-http-backend behind TLS, seeds a repository for each, and adds its certificate authority to the trusted roots of the simulator, so the TLS of libgit2 is verified for real rather than through a switch that turns verification off. The values the tests need are written to .git-servers/env as TEST_RUNNER_ variables, which xcodebuild passes into the test process. Both workflows start the servers before the tests and stop them afterwards whatever the result. Clone, pull and push are covered over both transports, along with a refused password and a provider that gives up, which stands for the user dismissing the passphrase prompt. Without the servers the tests skip and the rest of the suite is unaffected. Three things the servers must get right, each of which cost a round to find: the certificate authority needs keyUsage or strict verifiers refuse it while naming something else; the body of a push arrives chunked, so reading only Content-Length hands git-http-backend an empty pack; and the body has to be read before answering 401 or the retry that follows is misparsed. The device is pinned in the test lane because the certificate is trusted on one simulator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Python accepted the certificate file in either order here, but the OpenSSL on the runner did not: load_cert_chain wants the certificate first and the key after it, so the HTTPS server exited at startup and only the timeout was reported. The wait now watches the process as well as the port, so a server that dies is reported straight away together with whatever it printed, rather than after twenty seconds of silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HTTPServer looks up the name of the host it bound to before it starts accepting. Where that lookup is slow to answer, as it is on the runner, the port is bound but not yet listening, so waiting for a listening socket times out against a process that is perfectly healthy and silent. The spike showed this and was read wrong: a plain TCP probe took 35 seconds to answer, which was put down to the handshake blocking a single-threaded server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A device name matches every runtime that offers it, so the certificate was added to one simulator while the tests ran on another and every HTTPS test failed with "untrusted connection error". The script now records the identifier of the simulator it trusted and the test lane runs on exactly that one. The test for a refused password asserted only that something failed, so it passed while HTTPS was not working at all. It now checks that the connection itself succeeded, which is the whole point of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding a root certificate does not grant trust on every iOS version, and the runner offers this device on runtimes from 18.0 to 26.1 while only 18.x was ever verified. Taking the first entry of a JSON object meant the certificate was trusted on whichever simulator happened to be listed first, and the HTTPS tests then failed with "untrusted connection error" even though the device the tests ran on was pinned correctly. The runtime is now chosen deliberately, preferring the major version known to work, and is written to the log so a future failure says which one was used. GIT_SERVERS_IOS_MAJOR overrides it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…device simctl reports that it added the certificate authority to the trusted roots of the simulator, and on the runner the trust does not take effect: every HTTPS test failed with "untrusted connection error" while the device the tests ran on was pinned correctly and the runtime was the same 18.x that works here. Rather than keep guessing at that, the tests now accept exactly the certificate of the server they talk to, which behaves the same on every machine. The pin lives behind DEBUG so a release build has no trace of it, and it accepts one certificate rather than any: a new test points the pin at something else and requires the connection to be refused. What is no longer covered is the trust store itself, which those tests never really covered anyway, since it only ever worked on one machine. The assertions no longer expect the seeded commit to be the most recent one. The push tests add to the same repositories, so that only held the first time the suite ran against a fresh server; running it twice without restarting the servers failed the second time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading how many commits are unsynced walked the range and built a GitCommit, with four heap strings, for every one of them, only to take the count. Both configureTabBarItem and configureNavigationBar do that on viewWillAppear, and the controller never stops observing, so one change notification costs two walks per screen still on the stack. There is now a counting walk that allocates nothing, and reset reports what it discarded so its caller stops walking the same range a second time. The date shown for a password came from a full blame, which reconstructs the authorship of every line of the whole history, from inside a table view delegate on the main thread. It now walks back to the first commit whose content at that path differs from its parent's, which is what git log -1 -- <path> answers, and stops there. Renaming wrote the index twice, once for each side, in front of a commit that works from the index in memory anyway. One handle, one write. The signature was created and freed purely to validate it and then created again to commit with, so validation is left to the commit, which reports a name or email libgit2 will not take. The certificate the transport tests accept travels with the options of the operation rather than sitting in a global that a callback reads from another thread. Nothing shared, and still compiled out of a release build. The test for a cancelled prompt asserted only that the message was not something else, so it would have passed on any unrelated failure; it names the message now. Four tests cover the rewritten date, counting and reset. The header of git_servers.sh still described trusting the authority on the simulator, which was replaced by pinning two commits ago. It also killed whatever held its ports, including processes it never started; it now kills only what it recorded and refuses to start when a port is taken. A CGI Status of 403 was answered as 200, delivering an error as if it were a pack, a truncated chunk header killed the thread, and a checkout path with a space produced a URL the tests could not parse. The Intel simulator slice is built only on an Intel Mac. No runner can run it, and it was a third of a cold build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the ObjectiveGit dependency with libgit2 called directly.
ObjectiveGit was last released as 0.18 in 2019 and reached the app as a prebuilt xcframework hosted on a personal release, so the wrapper, the libgit2 inside it and the schedule for updating either were all pinned to that artifact. The git layer now calls libgit2 1.9.6, built from source by a script in this repository.
How it is split
Each commit builds and passes the tests on its own.
Replacing the backend
94f77ccconfines the backend behind value types.GTCommitandGTSignaturecrossedPasswordStoreinto the controllers, the clone, pull and push options were untyped dictionaries carrying aGTCredentialProvider, and the progress handlers took raw libgit2 pointers. Because passKit re-exports ObjectiveGit, the controllers used those types without importing anything, which hid the coupling from a search for imports.38b018areimplementsGitRepositoryandGitCredentialon the C API.693e2e7addsscripts/libgit2_build.sh.0db2d84links the result and deletes the package.459a569builds libgit2 in CI.22a6c3crecords why HTTPS stays on TLS 1.2, and stops rebuilding OpenSSL on every run.Fixing what review and the tests found
f118338fixes six findings from a review of the branch.adfc242dropsfileprivate, which only fails the build under--strict, which only CI passes.7741055accepts SSH host keys. See below.Covering the transports
7961ca6addsscripts/git_servers.shand the transport tests.2132effthrough528536bget those servers working on a runner.What had to be written by hand
libgit2 has no pull. It is assembled here as fetch, then merge analysis, then either nothing when already up to date, a reference update when the merge is a fast-forward, or a merge commit.
getLocalCommits,numberOfCommitsand the blame behind the "last updated" text are revwalks and a blame walk rather than wrapper conveniences.Credentials no longer depend on the backend at all:
GitCredentialproduces a value describing what to authenticate with, and only the callback inLibgit2.swiftturns that into agit_credential.The bug this nearly shipped with
libgit2 1.9 verifies the host key of an SSH remote against a
known_hostsfile and refuses the connection when it finds no match. iOS has no such file, so every SSH remote failed with "invalid or unknown remote ssh hostkey". The 0.28 inside the wrapper never verified host keys at all.Nothing caught it. It passed a full review, 161 tests and green CI, because every test cloned from a repository on disk. It is why the transport tests exist, and it was the first thing they found.
Host keys are now accepted, restoring the behaviour the app has always had. That means SSH has no protection against a machine in the middle, as it did not before either. Verifying host keys properly means storing them and asking on first connect, which is worth doing and is a feature rather than part of swapping a backend.
Also fixed
Testing
161 tests before, 170 now.
pull,resetand checking out a branch that only exists on the remote were not covered. The SSH credential test that had been skipped with "failed in CI, reason unknown" runs again, since it no longer depends on a live handshake.scripts/git_servers.shstarts an sshd and a git-http-backend behind TLS and seeds a repository for each. Clone, pull and push are covered over both transports, along with a refused password, a certificate that is not the one expected, and a provider that gives up, which stands for the user dismissing the passphrase prompt. Without the servers those tests skip and the rest of the suite is unaffected.The tests pin the certificate of the server rather than adding it to the trusted roots of the simulator:
simctlreports that it added it, and on a runner the trust does not take effect. The pin sits behindDEBUG, so a release build has no trace of it, and one test points it at a different certificate and requires the connection to be refused, so it cannot quietly become "accept anything". What this does not cover is validation against the trust store of the system.Trade-offs worth knowing
HTTPS stays on TLS 1.2. libgit2 hardcodes that ceiling in its SecureTransport stream. Patching the constant does not work: SecureTransport on iOS rejects
kTLSProtocol13witherrSSLIllegalParam, and libgit2 aborts the connection when that call fails, so the patch would stop HTTPS working rather than fall back to 1.2. Measured on the simulator, where a handshake with a host that offers TLS 1.3 settles on 1.2 regardless. TLS 1.3 would mean using OpenSSL for HTTPS and shipping a CA bundle with the app, since OpenSSL cannot read the trust store of the system. This is not a change: the libgit2 0.28 inside ObjectiveGit had the same ceiling.Two build scripts are needed before Xcode will build, as the README says. A cold libgit2 build is three OpenSSL compiles. CI caches the finished xcframework, so it is paid once per change to the build script. If that is too slow, publishing the xcframework to a release behind an SPM
binaryTargetremoves it from CI entirely, and the script already produces the artifact that would be uploaded.The German and Italian strings added for two new messages are not from a translator and deserve a native check.
🤖 Generated with Claude Code