Radish alpha
h
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
fetch: remove .keep files
Merged fintohaps opened 1 year ago

The .keep files for packs were never being removed, resulting in all pack files never being cleaned up by git gc.

Referring to Gitoxide[0][1], the same idea for removing the .keep file is applied when performing write_pack.

Signed-off-by: Fintan Halpenny fintan.halpenny@gmail.com X-Clacks-Overhead: GNU Terry Pratchett

6 files changed +66 -5 ca7db162 d56d619f
modified radicle-fetch/src/git.rs
@@ -1,5 +1,6 @@
pub(crate) mod mem;
pub(crate) mod repository;
+
pub(crate) mod packfile;

pub mod refs;

added radicle-fetch/src/git/packfile.rs
@@ -0,0 +1,32 @@
+
use std::fs;
+
use std::path::{Path, PathBuf};
+

+
/// The [`PathBuf`] which points to a `*.keep` file, which should correspond to
+
/// a packfile.
+
///
+
/// Upon drop, it attempts to remove the [`PathBuf`] to release the lock on the
+
/// packfile index, allowing it to be garbage collected.
+
#[derive(Clone, Debug)]
+
pub struct Keepfile {
+
    path: PathBuf,
+
}
+

+
impl Keepfile {
+
    pub fn new<P: AsRef<Path>>(path: P) -> Option<Self> {
+
        let path = path.as_ref();
+
        match path.extension() {
+
            Some(ext) if ext == "keep" => Some(Self {
+
                path: path.to_path_buf(),
+
            }),
+
            _ => None,
+
        }
+
    }
+
}
+

+
impl Drop for Keepfile {
+
    fn drop(&mut self) {
+
        if let Err(e) = fs::remove_file(&self.path) {
+
            log::warn!(target: "fetch", "Failed to remove {:?}: {e}", self.path);
+
        }
+
    }
+
}
modified radicle-fetch/src/state.rs
@@ -14,6 +14,7 @@ use radicle::storage::{
};

use crate::git;
+
use crate::git::packfile::Keepfile;
use crate::git::refs::{Applied, Update};
use crate::git::repository;
use crate::sigrefs::SignedRefsAt;
@@ -160,6 +161,10 @@ pub struct FetchState {
    sigrefs: SigrefTips,
    /// Seen reference tips, per remote.
    tips: BTreeMap<PublicKey, Vec<Update<'static>>>,
+
    /// The `.keep` files created during packfile transfers. They are kept
+
    /// within the state, so that when the state is dropped, it also attempts to
+
    /// delete the files to release the locks on the packfiles.
+
    keepfiles: Vec<Keepfile>,
}

impl FetchState {
@@ -233,9 +238,11 @@ impl FetchState {

        let wants_haves = step.wants_haves(&handle.repo, &refs)?;
        if !wants_haves.wants.is_empty() {
-
            handle
-
                .transport
-
                .fetch(wants_haves, handle.interrupt.clone(), handshake)?;
+
            let keepfile =
+
                handle
+
                    .transport
+
                    .fetch(wants_haves, handle.interrupt.clone(), handshake)?;
+
            self.keepfiles.extend(keepfile);
        } else {
            log::trace!(target: "fetch", "Nothing to fetch")
        };
modified radicle-fetch/src/transport.rs
@@ -21,6 +21,7 @@ use radicle::storage::git::Repository;
use thiserror::Error;

use crate::git::oid;
+
use crate::git::packfile::Keepfile;
use crate::git::repository;

/// Open a reader and writer stream to pass to the ls-refs and fetch
@@ -120,7 +121,7 @@ where
        wants_haves: WantsHaves,
        interrupt: Arc<AtomicBool>,
        handshake: &handshake::Outcome,
-
    ) -> io::Result<()> {
+
    ) -> io::Result<Option<Keepfile>> {
        log::trace!(
            target: "fetch",
            "Running fetch wants={:?}, haves={:?}",
@@ -170,7 +171,7 @@ where
            }
        }

-
        Ok(())
+
        Ok(out.keepfile)
    }

    /// Signal to the server side that we are done sending ls-refs and
modified radicle-fetch/src/transport/fetch.rs
@@ -17,6 +17,8 @@ use gix_transport::client;
use gix_transport::client::{ExtendedBufRead, MessageKind};
use gix_transport::Protocol;

+
use crate::git::packfile;
+

use super::{agent_name, indicate_end_of_interaction, Connection, WantsHaves};

pub type Error = gix_protocol::fetch::Error;
@@ -102,6 +104,7 @@ pub struct Fetch {
pub struct FetchOut {
    pub refs: Vec<Ref>,
    pub pack: Option<pack::bundle::write::Outcome>,
+
    pub keepfile: Option<packfile::Keepfile>,
}

// FIXME: the delegate pattern will be removed in the near future and
@@ -127,6 +130,7 @@ impl<'a> Delegate for &'a mut Fetch {
            .pack_writer
            .write_pack(input, progress)
            .map_err(|err| io::Error::new(io::ErrorKind::Other, err))?;
+
        self.out.keepfile = pack.keep_path.as_ref().and_then(packfile::Keepfile::new);
        self.out.pack = Some(pack);
        Ok(())
    }
@@ -204,6 +208,7 @@ where
        out: FetchOut {
            refs: Vec::new(),
            pack: None,
+
            keepfile: None,
        },
    };

modified radicle-node/src/tests/e2e.rs
@@ -216,6 +216,21 @@ fn test_replication() {
        alice.storage.repository(acme).unwrap().validate(),
        Ok(validations) if validations.is_empty()
    );
+

+
    // Ensure that .keep files are deleted upon replication
+
    {
+
        let repo = alice.storage.repository(acme).unwrap();
+
        let pack_dir = repo.path().join("objects").join("pack");
+
        for entry in std::fs::read_dir(pack_dir).unwrap() {
+
            let entry = entry.unwrap();
+
            let path = entry.path();
+
            assert_ne!(
+
                path.extension(),
+
                Some("keep".as_ref()),
+
                "found .keep file after fetch: {path:?}"
+
            );
+
        }
+
    }
}

#[test]