From 6baefdb13fa58f4f299b40d915bc27394f20e434 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 20 Dec 2023 15:05:30 +0000 Subject: [PATCH] cli: use `git` for anonymous fetch Shell out to `git`, avoiding using `git2`. This is to avoid an issue where somewhere within the fetch there is an attempt to lookup a `rad/sigrefs` object, which says that the object is missing. It is suspected that this is due to the object being localised in the same packfile as other objects we are fetching. Since the `rad/sigrefs` object is never needed nor used in the working copy, this will always result in the object missing. This seems to only be an issue with `libgit2`/`git2` and not `git` itself. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/examples/rad-patch-fetch-1.md | 1 + radicle-cli/src/commands/patch/checkout.rs | 12 +++----- radicle-remote-helper/src/fetch.rs | 29 +++++++----------- radicle-remote-helper/src/lib.rs | 12 +++++++- radicle/src/git.rs | 34 ++++++++++++++++++++++ 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/radicle-cli/examples/rad-patch-fetch-1.md b/radicle-cli/examples/rad-patch-fetch-1.md index f2eb0b71..28fdc9fc 100644 --- a/radicle-cli/examples/rad-patch-fetch-1.md +++ b/radicle-cli/examples/rad-patch-fetch-1.md @@ -20,6 +20,7 @@ $ git fetch --all Fetching rad Fetching alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi $ cat .git/FETCH_HEAD +f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 branch 'master' of rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji 7461703ce0fda972df450d071d1d3702057a6352 not-for-merge branch 'alice/1' of rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 not-for-merge branch 'master' of rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi $ git merge FETCH_HEAD diff --git a/radicle-cli/src/commands/patch/checkout.rs b/radicle-cli/src/commands/patch/checkout.rs index affa6327..b43d9239 100644 --- a/radicle-cli/src/commands/patch/checkout.rs +++ b/radicle-cli/src/commands/patch/checkout.rs @@ -6,7 +6,6 @@ use radicle::cob::patch::RevisionId; use radicle::git::RefString; use radicle::patch::PatchId; use radicle::storage::git::Repository; -use radicle::storage::ReadRepository; use radicle::{git, rad}; use crate::terminal as term; @@ -107,17 +106,14 @@ fn find_patch_commit<'a>( working: &'a git::raw::Repository, ) -> anyhow::Result> { let head = *revision.head(); + let workdir = working + .workdir() + .ok_or(anyhow::anyhow!("repository is a bare git repository "))?; match working.find_commit(head) { Ok(commit) => Ok(commit), Err(e) if git::ext::is_not_found_err(&e) => { - let url = git::url::File::new(stored.path()); - - working.remote_anonymous(url.to_string().as_str())?.fetch( - &[head.to_string()], - None, - None, - )?; + git::process::fetch_local(workdir, stored, [head.into()])?; working.find_commit(head).map_err(|e| e.into()) } Err(e) => Err(e.into()), diff --git a/radicle-remote-helper/src/fetch.rs b/radicle-remote-helper/src/fetch.rs index c6c66600..d8b625ac 100644 --- a/radicle-remote-helper/src/fetch.rs +++ b/radicle-remote-helper/src/fetch.rs @@ -5,7 +5,6 @@ use std::str::FromStr; use thiserror::Error; use radicle::git; -use radicle::storage::git::transport::local::Url; use radicle::storage::ReadRepository; use crate::read_line; @@ -30,7 +29,6 @@ pub enum Error { pub fn run( mut refs: Vec<(git::Oid, git::RefString)>, working: &Path, - url: Url, stored: R, stdin: &io::Stdin, ) -> Result<(), Error> { @@ -53,23 +51,18 @@ pub fn run( } // Verify them and prepare the final refspecs. - let mut refspecs = Vec::new(); - for (oid, refstr) in refs { - if let Some(nid) = url.namespace { - refspecs.push(nid.to_namespace().join(refstr).to_string()); - } else { - // Just fetch the object directly in this case, it's simpler and faster. - refspecs.push(oid.to_string()); - }; - } + let oids = refs.into_iter().map(|(oid, _)| oid); - let mut opts = git::raw::FetchOptions::new(); - // Setting this to false ensures that the FETCH_HEAD is set correctly. Go figure. - opts.update_fetchhead(false); - - git::raw::Repository::open(working)? - .remote_anonymous(&git::url::File::new(stored.path()).to_string())? - .fetch(&refspecs, Some(&mut opts), None)?; + // N.b. we shell out to `git`, avoiding using `git2`. This is to + // avoid an issue where somewhere within the fetch there is an + // attempt to lookup a `rad/sigrefs` object, which says that the + // object is missing. We suspect that this is due to the object + // being localised in the same packfile as other objects we are + // fetching. Since the `rad/sigrefs` object is never needed nor + // used in the working copy, this will always result in the object + // missing. This seems to only be an issue with `libgit2`/`git2` + // and not `git` itself. + git::process::fetch_local(working, &stored, oids)?; // Nb. An empty line means we're done. println!(); diff --git a/radicle-remote-helper/src/lib.rs b/radicle-remote-helper/src/lib.rs index f9b7f308..b42f305a 100644 --- a/radicle-remote-helper/src/lib.rs +++ b/radicle-remote-helper/src/lib.rs @@ -45,6 +45,9 @@ pub enum Error { /// The `GIT_DIR` env var is not set. #[error("the `GIT_DIR` environment variable is not set")] NoGitDir, + /// No parent of `GIT_DIR` was found. + #[error("expected parent of .git but found {path:?}")] + NoWorkingCopy { path: PathBuf }, /// Git error. #[error("git: {0}")] Git(#[from] git::raw::Error), @@ -153,7 +156,14 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> { let oid = git::Oid::from_str(oid)?; let refstr = git::RefString::try_from(*refstr)?; - return fetch::run(vec![(oid, refstr)], &working, url, stored, &stdin) + // N.b. `working` is the `.git` folder and `fetch::run` + // requires the working directory. + let working = working.canonicalize()?; + let working = working.parent().ok_or_else(|| Error::NoWorkingCopy { + path: working.clone(), + })?; + + return fetch::run(vec![(oid, refstr)], working, stored, &stdin) .map_err(Error::from); } ["push", refspec] => { diff --git a/radicle/src/git.rs b/radicle/src/git.rs index f56c8a60..bde9fb74 100644 --- a/radicle/src/git.rs +++ b/radicle/src/git.rs @@ -669,6 +669,40 @@ where )) } +/// Functions that call to the `git` CLI instead of `git2`. +pub mod process { + use std::io; + use std::path::Path; + + use crate::storage::ReadRepository; + + use super::{run, url, Oid}; + + /// Perform a local fetch, i.e. `file://`. + /// + /// `oids` are the set of [`Oid`]s that are being fetched from the + /// `storage`. + pub fn fetch_local( + working: &Path, + storage: &R, + oids: impl IntoIterator, + ) -> Result<(), io::Error> + where + R: ReadRepository, + { + let mut fetch = vec![ + "fetch".to_string(), + url::File::new(storage.path()).to_string(), + // N.b. avoid writing fetch head since we're only fetching objects + "--no-write-fetch-head".to_string(), + ]; + fetch.extend(oids.into_iter().map(|oid| oid.to_string())); + // N.b. `.` is used since we're fetching within the working copy + run::<_, _, &str, &str>(working, fetch, [])?; + Ok(()) + } +} + /// Git URLs. pub mod url { use std::path::PathBuf;