Merge pull request #3593 from Byron/locks-for-writers

use tempfiles for file mutations
This commit is contained in:
Sebastian Thiel 2024-04-24 10:55:10 +02:00 committed by GitHub
commit 813ba8f2d6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 126 additions and 108 deletions

View File

@ -113,7 +113,8 @@ impl Helper {
}
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Self {
pub fn from_path(path: impl Into<PathBuf>) -> Self {
let path = path.into();
let keys = keys::Controller::from_path(&path);
let users = users::Controller::from_path(path);
let home_dir = std::env::var_os("HOME").map(PathBuf::from);

View File

@ -1,4 +1,5 @@
use anyhow::Context;
use std::path::PathBuf;
use super::{storage::Storage, PrivateKey};
@ -12,7 +13,7 @@ impl Controller {
Self { storage }
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Self {
pub fn from_path(path: impl Into<PathBuf>) -> Self {
Self::new(Storage::from_path(path))
}

View File

@ -1,42 +1,40 @@
use super::PrivateKey;
use crate::storage;
use std::path::PathBuf;
// TODO(ST): get rid of this type, it's more trouble than it's worth.
#[derive(Clone)]
pub struct Storage {
storage: storage::Storage,
inner: storage::Storage,
}
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("IO error: {0}")]
Storage(#[from] storage::Error),
#[error(transparent)]
Storage(#[from] std::io::Error),
#[error("SSH key error: {0}")]
SSHKey(#[from] ssh_key::Error),
}
impl Storage {
pub fn new(storage: storage::Storage) -> Storage {
Storage { storage }
Storage { inner: storage }
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Storage {
pub fn from_path(path: impl Into<PathBuf>) -> Storage {
Storage::new(storage::Storage::new(path))
}
pub fn get(&self) -> Result<Option<PrivateKey>, Error> {
self.storage
.read("keys/ed25519")
.map_err(Error::Storage)
.and_then(|s| s.map(|s| s.parse().map_err(Error::SSHKey)).transpose())
let key = self.inner.read("keys/ed25519")?;
key.map(|s| s.parse().map_err(Into::into)).transpose()
}
// TODO(ST): see if Key should rather deal with bytes instead for this kind of serialization.
pub fn create(&self, key: &PrivateKey) -> Result<(), Error> {
self.storage
.write("keys/ed25519", &key.to_string())
.map_err(Error::Storage)?;
self.storage
.write("keys/ed25519.pub", &key.public_key().to_string())
.map_err(Error::Storage)?;
self.inner.write("keys/ed25519", &key.to_string())?;
self.inner
.write("keys/ed25519.pub", &key.public_key().to_string())?;
Ok(())
}
}

View File

@ -46,12 +46,12 @@ impl Controller {
}
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Self {
let pathbuf = path.as_ref().to_path_buf();
pub fn from_path(path: impl Into<PathBuf>) -> Self {
let path = path.into();
Self {
local_data_dir: pathbuf.clone(),
projects_storage: storage::Storage::from_path(&pathbuf),
users: users::Controller::from_path(&pathbuf),
projects_storage: storage::Storage::from_path(&path),
users: users::Controller::from_path(&path),
local_data_dir: path,
watchers: None,
}
}

View File

@ -1,4 +1,5 @@
use serde::{Deserialize, Serialize};
use std::path::PathBuf;
use crate::{
projects::{project, ProjectId},
@ -9,7 +10,7 @@ const PROJECTS_FILE: &str = "projects.json";
#[derive(Debug, Clone)]
pub struct Storage {
storage: storage::Storage,
inner: storage::Storage,
}
#[derive(Debug, Serialize, Deserialize, Default, Clone)]
@ -30,7 +31,7 @@ pub struct UpdateRequest {
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error(transparent)]
Storage(#[from] storage::Error),
Storage(#[from] std::io::Error),
#[error(transparent)]
Json(#[from] serde_json::Error),
#[error("project not found")]
@ -38,16 +39,16 @@ pub enum Error {
}
impl Storage {
pub fn new(storage: storage::Storage) -> Storage {
Storage { storage }
pub fn new(storage: storage::Storage) -> Self {
Self { inner: storage }
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Storage {
Storage::new(storage::Storage::new(path))
pub fn from_path(path: impl Into<PathBuf>) -> Self {
Self::new(storage::Storage::new(path))
}
pub fn list(&self) -> Result<Vec<project::Project>, Error> {
match self.storage.read(PROJECTS_FILE)? {
match self.inner.read(PROJECTS_FILE)? {
Some(projects) => {
let all_projects: Vec<project::Project> = serde_json::from_str(&projects)?;
let all_projects: Vec<project::Project> = all_projects
@ -128,7 +129,7 @@ impl Storage {
project.omit_certificate_check = Some(omit_certificate_check);
}
self.storage
self.inner
.write(PROJECTS_FILE, &serde_json::to_string_pretty(&projects)?)?;
Ok(projects
@ -142,7 +143,7 @@ impl Storage {
let mut projects = self.list()?;
if let Some(index) = projects.iter().position(|p| p.id == *id) {
projects.remove(index);
self.storage
self.inner
.write(PROJECTS_FILE, &serde_json::to_string_pretty(&projects)?)?;
}
Ok(())
@ -152,7 +153,7 @@ impl Storage {
let mut projects = self.list()?;
projects.push(project.clone());
let projects = serde_json::to_string_pretty(&projects)?;
self.storage.write(PROJECTS_FILE, &projects)?;
self.inner.write(PROJECTS_FILE, &projects)?;
Ok(())
}
}

View File

@ -1,71 +1,85 @@
#[cfg(target_family = "unix")]
use std::os::unix::prelude::*;
use gix::tempfile::create_dir::Retries;
use gix::tempfile::{AutoRemove, ContainingDirectory};
use std::io::Write;
use std::{
fs,
path::{Path, PathBuf},
sync::{Arc, RwLock},
};
#[derive(Debug, Default, Clone)]
/// A facility to read, write and delete files.
#[derive(Debug, Clone)]
pub struct Storage {
local_data_dir: Arc<RwLock<PathBuf>>,
}
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error(transparent)]
IO(#[from] std::io::Error),
/// The directory into which all of or files will be written or read-from.
local_data_dir: PathBuf,
}
impl Storage {
pub fn new<P: AsRef<Path>>(local_data_dir: P) -> Storage {
pub fn new(local_data_dir: impl Into<PathBuf>) -> Storage {
Storage {
local_data_dir: Arc::new(RwLock::new(local_data_dir.as_ref().to_path_buf())),
local_data_dir: local_data_dir.into(),
}
}
pub fn read<P: AsRef<Path>>(&self, path: P) -> Result<Option<String>, Error> {
let local_data_dir = self.local_data_dir.read().unwrap();
let file_path = local_data_dir.join(path);
if !file_path.exists() {
return Ok(None);
/// Read the content of the file at `rela_path` which is a path relative to our root directory.
/// Return `Ok(None)` if the file doesn't exist.
// TODO(ST): make all these operations write bytes.
pub fn read(&self, rela_path: impl AsRef<Path>) -> std::io::Result<Option<String>> {
match fs::read_to_string(self.local_data_dir.join(rela_path)) {
Ok(content) => Ok(Some(content)),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None),
Err(err) => Err(err),
}
let contents = fs::read_to_string(&file_path).map_err(Error::IO)?;
Ok(Some(contents))
}
pub fn write<P: AsRef<Path>>(&self, path: P, content: &str) -> Result<(), Error> {
let local_data_dir = self.local_data_dir.write().unwrap();
let file_path = local_data_dir.join(path);
/// Write `content` to `rela_path` atomically, so it's either written completely, or not at all.
/// Creates the file and intermediate directories.
///
/// ### On Synchronization
///
/// Mutating operations are assumed to be synchronized by the caller,
/// even though all writes will be atomic.
///
/// If these operations are not synchronized, they will be racy as it's undefined
/// which *whole* write will win. Thus, operations which touch multiple files and
/// need them to be consistent *need* to synchronize by some mean.
///
/// Generally, the filesystem is used for synchronization, not in-memory primitives.
pub fn write(&self, rela_path: impl AsRef<Path>, content: &str) -> std::io::Result<()> {
let file_path = self.local_data_dir.join(rela_path);
let dir = file_path.parent().unwrap();
if !dir.exists() {
fs::create_dir_all(dir).map_err(Error::IO)?;
// NOTE: This creates a 0o600 files on Unix by default.
let mut tempfile = gix::tempfile::new(
dir,
ContainingDirectory::CreateAllRaceProof(Retries::default()),
AutoRemove::Tempfile,
)?;
tempfile.write_all(content.as_bytes())?;
match tempfile.persist(file_path) {
Ok(Some(_opened_file)) => Ok(()),
Ok(None) => unreachable!("BUG: a signal has caused the tempfile to be removed, but we didn't install a handler"),
Err(err) => Err(err.error)
}
fs::write(&file_path, content).map_err(Error::IO)?;
// Set the permissions to be user-only. We can't actually
// do this on Windows, so we ignore that platform.
#[cfg(target_family = "unix")]
{
let metadata = fs::metadata(file_path.clone())?;
let mut permissions = metadata.permissions();
permissions.set_mode(0o600); // User read/write
fs::set_permissions(file_path.clone(), permissions)?;
}
Ok(())
}
pub fn delete<P: AsRef<Path>>(&self, path: P) -> Result<(), Error> {
let local_data_dir = self.local_data_dir.write().unwrap();
let file_path = local_data_dir.join(path);
if !file_path.exists() {
return Ok(());
}
if file_path.is_dir() {
fs::remove_dir_all(file_path.clone()).map_err(Error::IO)?;
/// Delete the file or directory at `rela_path`.
///
/// ### Panics
///
/// If a symlink is encountered.
pub fn delete(&self, rela_path: impl AsRef<Path>) -> std::io::Result<()> {
let file_path = self.local_data_dir.join(rela_path);
let md = match file_path.symlink_metadata() {
Ok(md) => md,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(err) => return Err(err),
};
if md.is_dir() {
fs::remove_dir_all(file_path)?;
} else if md.is_file() {
fs::remove_file(file_path)?;
} else {
fs::remove_file(file_path.clone()).map_err(Error::IO)?;
unreachable!("BUG: we do not create or work with symlinks")
}
Ok(())
}

View File

@ -1,7 +1,9 @@
use anyhow::Context;
use std::path::PathBuf;
use super::{storage::Storage, User};
/// TODO(ST): useless intermediary - remove
#[derive(Clone)]
pub struct Controller {
storage: Storage,
@ -12,7 +14,7 @@ impl Controller {
Controller { storage }
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Controller {
pub fn from_path(path: impl Into<PathBuf>) -> Controller {
Controller::new(Storage::from_path(path))
}

View File

@ -1,4 +1,5 @@
use anyhow::Result;
use std::path::PathBuf;
use crate::{storage, users::user};
@ -6,28 +7,28 @@ const USER_FILE: &str = "user.json";
#[derive(Debug, Clone)]
pub struct Storage {
storage: storage::Storage,
inner: storage::Storage,
}
#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error(transparent)]
Storage(#[from] storage::Error),
Storage(#[from] std::io::Error),
#[error(transparent)]
Json(#[from] serde_json::Error),
}
impl Storage {
pub fn new(storage: storage::Storage) -> Storage {
Storage { storage }
Storage { inner: storage }
}
pub fn from_path<P: AsRef<std::path::Path>>(path: P) -> Storage {
pub fn from_path(path: impl Into<PathBuf>) -> Storage {
Storage::new(storage::Storage::new(path))
}
pub fn get(&self) -> Result<Option<user::User>, Error> {
match self.storage.read(USER_FILE)? {
match self.inner.read(USER_FILE)? {
Some(data) => Ok(Some(serde_json::from_str(&data)?)),
None => Ok(None),
}
@ -35,12 +36,12 @@ impl Storage {
pub fn set(&self, user: &user::User) -> Result<(), Error> {
let data = serde_json::to_string(user)?;
self.storage.write(USER_FILE, &data)?;
self.inner.write(USER_FILE, &data)?;
Ok(())
}
pub fn delete(&self) -> Result<(), Error> {
self.storage.delete(USER_FILE)?;
self.inner.delete(USER_FILE)?;
Ok(())
}
}

View File

@ -19,14 +19,14 @@ impl TestCase<'_> {
fn run(&self) -> Vec<(String, Vec<Credential>)> {
let local_app_data = temp_dir();
let users = users::Controller::from_path(&local_app_data);
let users = users::Controller::from_path(local_app_data.path());
let user = users::User {
github_access_token: self.github_access_token.map(ToString::to_string),
..Default::default()
};
users.set_user(&user).unwrap();
let keys = keys::Controller::from_path(&local_app_data);
let keys = keys::Controller::from_path(local_app_data.path());
let helper = Helper::new(keys, users, self.home_dir.clone());
let (repo, _tmp) = test_repository();

View File

@ -12,7 +12,7 @@ mod init {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())
@ -32,7 +32,7 @@ mod init {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())
@ -54,7 +54,7 @@ mod init {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())
@ -84,7 +84,7 @@ mod flush {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())
@ -107,7 +107,7 @@ mod flush {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())
@ -131,7 +131,7 @@ mod flush {
let test_project = TestProject::default();
let data_dir = paths::data_dir();
let projects = projects::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(data_dir.path());
let project = projects
.add(test_project.path())

View File

@ -5,7 +5,7 @@ use gitbutler_testsupport::{self, paths};
pub fn new() -> (Controller, TempDir) {
let data_dir = paths::data_dir();
let controller = Controller::from_path(&data_dir);
let controller = Controller::from_path(data_dir.path());
(controller, data_dir)
}

View File

@ -3,10 +3,10 @@ use super::*;
#[tokio::test]
async fn twice() {
let data_dir = paths::data_dir();
let keys = keys::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(&data_dir);
let users = users::Controller::from_path(&data_dir);
let helper = git::credentials::Helper::from_path(&data_dir);
let keys = keys::Controller::from_path(data_dir.path());
let projects = projects::Controller::from_path(data_dir.path());
let users = users::Controller::from_path(data_dir.path());
let helper = git::credentials::Helper::from_path(data_dir.path());
let test_project = TestProject::default();

View File

@ -29,10 +29,10 @@ impl Drop for Test {
impl Default for Test {
fn default() -> Self {
let data_dir = paths::data_dir();
let keys = keys::Controller::from_path(&data_dir);
let projects = projects::Controller::from_path(&data_dir);
let users = users::Controller::from_path(&data_dir);
let helper = git::credentials::Helper::from_path(&data_dir);
let keys = keys::Controller::from_path(data_dir.path());
let projects = projects::Controller::from_path(data_dir.path());
let users = users::Controller::from_path(data_dir.path());
let helper = git::credentials::Helper::from_path(data_dir.path());
let test_project = TestProject::default();
let project = projects

View File

@ -27,10 +27,10 @@ impl Drop for Suite {
impl Default for Suite {
fn default() -> Self {
let local_app_data = temp_dir();
let storage = gitbutler_core::storage::Storage::new(&local_app_data);
let users = gitbutler_core::users::Controller::from_path(&local_app_data);
let projects = gitbutler_core::projects::Controller::from_path(&local_app_data);
let keys = gitbutler_core::keys::Controller::from_path(&local_app_data);
let storage = gitbutler_core::storage::Storage::new(local_app_data.path());
let users = gitbutler_core::users::Controller::from_path(local_app_data.path());
let projects = gitbutler_core::projects::Controller::from_path(local_app_data.path());
let keys = gitbutler_core::keys::Controller::from_path(local_app_data.path());
Self {
storage,
local_app_data: Some(local_app_data),