From 5555a455aa5ba7769b95312c0eb22efab11b7652 Mon Sep 17 00:00:00 2001 From: Arvid Norlander Date: Fri, 20 Sep 2024 19:21:57 +0200 Subject: [PATCH] feat: Improve diff view when restoring to package manager state (fixes #91) --- crates/konfigkoll/src/apply.rs | 3 + crates/konfigkoll/src/main.rs | 11 ++- crates/konfigkoll_core/src/apply.rs | 68 ++++++++----------- crates/konfigkoll_core/src/diff.rs | 10 +++ crates/konfigkoll_core/src/utils.rs | 54 +++++++++++++++ .../arch/basic_config/expected/2_output.txt | 25 +++++++ 6 files changed, 132 insertions(+), 39 deletions(-) diff --git a/crates/konfigkoll/src/apply.rs b/crates/konfigkoll/src/apply.rs index 57d6f89..a031995 100644 --- a/crates/konfigkoll/src/apply.rs +++ b/crates/konfigkoll/src/apply.rs @@ -39,6 +39,9 @@ pub(crate) fn create_applicator( inner_applicator, diff_command, pager_command, + files_backend, + interner, + package_maps, )), Paranoia::DryRun => Box::new(konfigkoll_core::apply::NoopApplicator::default()), }; diff --git a/crates/konfigkoll/src/main.rs b/crates/konfigkoll/src/main.rs index c3710f4..308e138 100644 --- a/crates/konfigkoll/src/main.rs +++ b/crates/konfigkoll/src/main.rs @@ -15,6 +15,7 @@ use konfigkoll_core::apply::apply_packages; use konfigkoll_core::diff::show_fs_instr_diff; use konfigkoll_core::state::DiffGoal; use konfigkoll_core::state::FsEntries; +use konfigkoll_core::utils::pkg_backend_for_files; use konfigkoll_script::Phase; use konfigkoll_script::ScriptEngine; use konfigkoll_types::FsInstruction; @@ -303,9 +304,17 @@ async fn main() -> eyre::Result<()> { let diff_cmd = script_engine.state().settings().diff(); let pager_cmd = script_engine.state().settings().pager(); + let pkg_file_backend = pkg_backend_for_files(&package_maps, &*backend_files)?; for change in fs_changes { if change.path.starts_with(&path) { - show_fs_instr_diff(&change, &diff_cmd, &pager_cmd)?; + show_fs_instr_diff( + &change, + &diff_cmd, + &pager_cmd, + &interner, + &*backend_files, + &pkg_file_backend, + )?; } } // Let the OS clean these up, freeing in the program is slower diff --git a/crates/konfigkoll_core/src/apply.rs b/crates/konfigkoll_core/src/apply.rs index a2addb2..7a01528 100644 --- a/crates/konfigkoll_core/src/apply.rs +++ b/crates/konfigkoll_core/src/apply.rs @@ -3,6 +3,8 @@ use crate::confirm::Choices; use crate::confirm::MultiOptionConfirm; use crate::diff::show_fs_instr_diff; +use crate::utils::original_file_contents; +use crate::utils::pkg_backend_for_files; use crate::utils::IdKey; use crate::utils::NameToNumericResolveCache; use ahash::AHashMap; @@ -18,7 +20,6 @@ use konfigkoll_types::PkgInstruction; use konfigkoll_types::PkgOp; use paketkoll_types::backend::Backend; use paketkoll_types::backend::Files; -use paketkoll_types::backend::OriginalFileQuery; use paketkoll_types::backend::PackageBackendMap; use paketkoll_types::backend::PackageMap; use paketkoll_types::backend::PackageMapMap; @@ -225,31 +226,10 @@ impl InProcessApplicator { nix::unistd::chown(instr.path.as_std_path(), None, Some(gid))?; } FsOp::Restore => { - // Get package: - let owners = self - .file_backend - .owning_packages(&[instr.path.as_std_path()].into(), &self.interner) - .wrap_err_with(|| format!("Failed to find owner for {}", instr.path))?; - let package = owners - .get(instr.path.as_std_path()) - .ok_or_else(|| eyre::eyre!("Failed to find owner for {}", instr.path))? - .ok_or_else(|| eyre::eyre!("No owner for {}", instr.path))?; - let package = package.as_str(&self.interner); - // Get original contents: - let queries = [OriginalFileQuery { - package: package.into(), - path: instr.path.as_str().into(), - }]; - let original_contents = - self.file_backend - .original_files(&queries, pkg_map, &self.interner)?; + let contents = + original_file_contents(&*self.file_backend, &self.interner, instr, pkg_map)?; // Apply - for query in queries { - let contents = original_contents - .get(&query) - .ok_or_else(|| eyre::eyre!("No original contents for {:?}", query))?; - std::fs::write(&instr.path, contents)?; - } + std::fs::write(&instr.path, contents)?; } FsOp::Comment => { tracing::warn!( @@ -306,16 +286,7 @@ impl Applicator for InProcessApplicator { } fn apply_files(&mut self, instructions: &[FsInstruction]) -> eyre::Result<()> { - let pkg_map = self - .package_maps - .get(&self.file_backend.as_backend_enum()) - .ok_or_else(|| { - eyre::eyre!( - "No package map for file backend {:?}", - self.file_backend.as_backend_enum() - ) - })? - .clone(); + let pkg_map = pkg_backend_for_files(&self.package_maps, &*self.file_backend)?; for instr in instructions { self.apply_single_file(instr, &pkg_map).wrap_err_with(|| { format!("Failed to apply change for {}: {:?}", instr.path, instr.op) @@ -389,10 +360,20 @@ pub struct InteractiveApplicator { interactive_confirmer: MultiOptionConfirm, diff_command: Vec, pager_command: Vec, + file_backend: Arc, + interner: Arc, + package_maps: PackageMapMap, } impl InteractiveApplicator { - pub fn new(inner: Inner, diff_command: Vec, pager_command: Vec) -> Self { + pub fn new( + inner: Inner, + diff_command: Vec, + pager_command: Vec, + file_backend: &Arc, + interner: &Arc, + package_maps: &PackageMapMap, + ) -> Self { let mut prompt_builder = MultiOptionConfirm::builder(); prompt_builder.prompt("Do you want to apply these changes?"); let pkg_confirmer = prompt_builder.build(); @@ -411,6 +392,9 @@ impl InteractiveApplicator { interactive_confirmer, diff_command, pager_command, + file_backend: file_backend.clone(), + interner: interner.clone(), + package_maps: package_maps.clone(), } } } @@ -465,8 +449,9 @@ impl Applicator for InteractiveApplicator { + let pkg_map = pkg_backend_for_files(&self.package_maps, &*self.file_backend)?; for instr in instructions { - self.interactive_apply_single_file(instr)?; + self.interactive_apply_single_file(instr, &pkg_map)?; } Ok(()) } @@ -495,7 +480,11 @@ fn show_pkg_diff(backend: Backend, install: &[&str], mark_explicit: &[&str], uni } impl InteractiveApplicator { - fn interactive_apply_single_file(&mut self, instr: &FsInstruction) -> eyre::Result<()> { + fn interactive_apply_single_file( + &mut self, + instr: &FsInstruction, + pkg_map: &PackageMap, + ) -> eyre::Result<()> { println!( "Under consideration: {} with change {}", style(instr.path.as_str()).blue(), @@ -520,6 +509,9 @@ impl InteractiveApplicator { instr, self.diff_command.as_slice(), self.pager_command.as_slice(), + &self.interner, + &*self.file_backend, + pkg_map, )?; } }; diff --git a/crates/konfigkoll_core/src/diff.rs b/crates/konfigkoll_core/src/diff.rs index 9af6f02..bf74feb 100644 --- a/crates/konfigkoll_core/src/diff.rs +++ b/crates/konfigkoll_core/src/diff.rs @@ -2,6 +2,7 @@ //! //! This module implements a generic algorithm similar to comm(1) +use crate::utils::original_file_contents; use camino::Utf8Path; use camino::Utf8PathBuf; use console::style; @@ -9,6 +10,9 @@ use itertools::EitherOrBoth; use itertools::Itertools; use konfigkoll_types::FsInstruction; use konfigkoll_types::FsOp; +use paketkoll_types::backend::Files; +use paketkoll_types::backend::PackageMap; +use paketkoll_types::intern::Interner; use paketkoll_utils::MODE_MASK; use std::iter::FusedIterator; use std::os::unix::fs::MetadataExt; @@ -28,6 +32,9 @@ pub fn show_fs_instr_diff( instr: &FsInstruction, diff_command: &[String], pager_command: &[String], + interner: &Interner, + file_backend: &dyn Files, + pkg_map: &PackageMap, ) -> eyre::Result<()> { match &instr.op { FsOp::CreateFile(contents) => { @@ -137,6 +144,9 @@ pub fn show_fs_instr_diff( "{}: Would restore to original package manager state", style(&instr.path).color256(202) ); + let contents = original_file_contents(file_backend, interner, instr, pkg_map)?; + let contents = konfigkoll_types::FileContents::from_literal(contents.into()); + show_file_diff(&instr.path, &contents, diff_command, pager_command)?; } FsOp::Comment => (), }; diff --git a/crates/konfigkoll_core/src/utils.rs b/crates/konfigkoll_core/src/utils.rs index 68572f6..e120fcd 100644 --- a/crates/konfigkoll_core/src/utils.rs +++ b/crates/konfigkoll_core/src/utils.rs @@ -3,9 +3,17 @@ use clru::CLruCache; use compact_str::CompactString; use eyre::eyre; +use eyre::Context; +use konfigkoll_types::FsInstruction; +use paketkoll_types::backend::Files; +use paketkoll_types::backend::OriginalFileQuery; +use paketkoll_types::backend::PackageMap; +use paketkoll_types::backend::PackageMapMap; use paketkoll_types::files::Gid; use paketkoll_types::files::Uid; +use paketkoll_types::intern::Interner; use std::num::NonZeroUsize; +use std::sync::Arc; /// UID/GID to name resolver / cache #[derive(Debug)] @@ -105,3 +113,49 @@ where User(UserKey), Group(GroupKey), } + +/// Resolve the original contents of a file given a file backend and instruction +pub(crate) fn original_file_contents( + file_backend: &dyn Files, + interner: &Interner, + instr: &FsInstruction, + pkg_map: &PackageMap, +) -> Result, eyre::Error> { + // Get package owning the file + let owners = file_backend + .owning_packages(&[instr.path.as_std_path()].into(), interner) + .wrap_err_with(|| format!("Failed to find owner for {}", instr.path))?; + let package = owners + .get(instr.path.as_std_path()) + .ok_or_else(|| eyre::eyre!("Failed to find owner for {}", instr.path))? + .ok_or_else(|| eyre::eyre!("No owner for {}", instr.path))?; + let package = package.as_str(interner); + // Create query + let queries = [OriginalFileQuery { + package: package.into(), + path: instr.path.as_str().into(), + }]; + // Get file contents + let mut original_contents = file_backend.original_files(&queries, pkg_map, interner)?; + let original_contents = original_contents + .remove(&queries[0]) + .ok_or_else(|| eyre::eyre!("No original contents for {:?}", queries[0]))?; + Ok(original_contents) +} + +/// Helper to get the package map for the file backend +pub fn pkg_backend_for_files( + package_maps: &PackageMapMap, + file_backend: &dyn Files, +) -> Result, eyre::Error> { + let pkg_map = package_maps + .get(&file_backend.as_backend_enum()) + .ok_or_else(|| { + eyre::eyre!( + "No package map for file backend {:?}", + file_backend.as_backend_enum() + ) + })? + .clone(); + Ok(pkg_map) +} diff --git a/integration_tests/arch/basic_config/expected/2_output.txt b/integration_tests/arch/basic_config/expected/2_output.txt index c06f032..f0691b2 100644 --- a/integration_tests/arch/basic_config/expected/2_output.txt +++ b/integration_tests/arch/basic_config/expected/2_output.txt @@ -29,3 +29,28 @@ INFO konfigkoll: Waiting for file system scan results... INFO konfigkoll: Got file system scan results INFO konfigkoll: Computing diff /etc/passwd: Would restore to original package manager state +--- /etc/passwd 2024-09-07 07:56:54.000000000 +0000 ++++ /dev/stdin 2024-09-20 17:29:52.604937717 +0000 +@@ -1,22 +1 @@ + root:x:0:0::/root:/usr/bin/bash +-bin:x:1:1::/:/usr/bin/nologin +-daemon:x:2:2::/:/usr/bin/nologin +-mail:x:8:12::/var/spool/mail:/usr/bin/nologin +-ftp:x:14:11::/srv/ftp:/usr/bin/nologin +-http:x:33:33::/srv/http:/usr/bin/nologin +-nobody:x:65534:65534:Kernel Overflow User:/:/usr/bin/nologin +-dbus:x:81:81:System Message Bus:/:/usr/bin/nologin +-systemd-coredump:x:980:980:systemd Core Dumper:/:/usr/bin/nologin +-systemd-network:x:979:979:systemd Network Management:/:/usr/bin/nologin +-systemd-oom:x:978:978:systemd Userspace OOM Killer:/:/usr/bin/nologin +-systemd-journal-remote:x:977:977:systemd Journal Remote:/:/usr/bin/nologin +-systemd-resolve:x:976:976:systemd Resolver:/:/usr/bin/nologin +-systemd-timesync:x:975:975:systemd Time Synchronization:/:/usr/bin/nologin +-tss:x:974:974:tss user for tpm2:/:/usr/bin/nologin +-uuidd:x:68:68::/:/usr/bin/nologin +-avahi:x:973:973:Avahi mDNS/DNS-SD daemon:/:/usr/bin/nologin +-flatpak:x:972:972:Flatpak system helper:/:/usr/bin/nologin +-geoclue:x:971:971:Geoinformation service:/var/lib/geoclue:/usr/bin/nologin +-git:x:970:970:git daemon user:/:/usr/bin/git-shell +-polkitd:x:102:102:User for polkitd:/:/usr/bin/nologin +-rtkit:x:133:133:RealtimeKit:/proc:/usr/bin/nologin