From dbf6c8c87cdca8169ac01aa89aefe56a33215142 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 12 Dec 2022 07:42:36 +0100 Subject: [PATCH 01/15] change!: rename `scripted_fixture_*` to not contain 'repo' in the name. (#650) Further make clear in the documentation that `bash` is used to execute the fixture scripts, previously it wasn't even implied and got lost in history. --- tests/tools/src/lib.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 8263b654097..0970067a7e9 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -83,7 +83,7 @@ static EXCLUDE_LUT: Lazy>>> = Lazy /// The major, minor and patch level of the git version on the system. pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_git_version().expect("git version to be parsable")); -/// Define how [scripted_fixture_repo_writable_with_args()] uses produces the writable copy. +/// Define how [scripted_fixture_writable_with_args()] uses produces the writable copy. pub enum Creation { /// Run the script once and copy the data from its output to the writable location. /// This is fast but won't work if absolute paths are produced by the script. @@ -205,7 +205,7 @@ pub fn fixture_bytes(path: impl AsRef) -> Vec { } } -/// Run the executable at `script_name`, like `make_repo.sh` to produce a read-only directory to which +/// Run the script file `script_name` using `bash`, like `make_repo.sh` to produce a read-only directory to which /// the path is returned. /// /// Note that it persists and the script at `script_name` will only be executed once if it ran without error. @@ -227,21 +227,21 @@ pub fn fixture_bytes(path: impl AsRef) -> Vec { /// or more specific `.gitignore` configurations in lower levels of the work tree. /// /// The latter is useful if the the script's output is platform specific. -pub fn scripted_fixture_repo_read_only(script_name: impl AsRef) -> Result { - scripted_fixture_repo_read_only_with_args(script_name, None::) +pub fn scripted_fixture_read_only(script_name: impl AsRef) -> Result { + scripted_fixture_read_only_with_args(script_name, None::) } /// Run the executable at `script_name`, like `make_repo.sh` to produce a writable directory to which /// the tempdir is returned. It will be removed automatically, courtesy of [`tempfile::TempDir`]. /// /// Note that `script_name` is only executed once, so the data can be copied from its read-only location. -pub fn scripted_fixture_repo_writable(script_name: &str) -> Result { - scripted_fixture_repo_writable_with_args(script_name, None::, Creation::CopyFromReadOnly) +pub fn scripted_fixture_writable(script_name: &str) -> Result { + scripted_fixture_writable_with_args(script_name, None::, Creation::CopyFromReadOnly) } -/// Like [`scripted_fixture_repo_writable()`], but passes `args` to `script_name` while providing control over +/// Like [`scripted_fixture_writable()`], but passes `args` to `script_name` while providing control over /// the way files are created with `mode`. -pub fn scripted_fixture_repo_writable_with_args( +pub fn scripted_fixture_writable_with_args( script_name: &str, args: impl IntoIterator>, mode: Creation, @@ -249,12 +249,12 @@ pub fn scripted_fixture_repo_writable_with_args( let dst = tempfile::TempDir::new()?; Ok(match mode { Creation::CopyFromReadOnly => { - let ro_dir = scripted_fixture_repo_read_only_with_args_inner(script_name, args, None)?; + let ro_dir = scripted_fixture_read_only_with_args_inner(script_name, args, None)?; copy_recursively_into_existing_dir(&ro_dir, dst.path())?; dst } Creation::ExecuteScript => { - scripted_fixture_repo_read_only_with_args_inner(script_name, args, dst.path().into())?; + scripted_fixture_read_only_with_args_inner(script_name, args, dst.path().into())?; dst } }) @@ -279,15 +279,15 @@ pub fn copy_recursively_into_existing_dir(src_dir: impl AsRef, dst_dir: im Ok(()) } -/// Like `scripted_fixture_repo_read_only()`], but passes `args` to `script_name`. -pub fn scripted_fixture_repo_read_only_with_args( +/// Like `scripted_fixture_read_only()`], but passes `args` to `script_name`. +pub fn scripted_fixture_read_only_with_args( script_name: impl AsRef, args: impl IntoIterator>, ) -> Result { - scripted_fixture_repo_read_only_with_args_inner(script_name, args, None) + scripted_fixture_read_only_with_args_inner(script_name, args, None) } -fn scripted_fixture_repo_read_only_with_args_inner( +fn scripted_fixture_read_only_with_args_inner( script_name: impl AsRef, args: impl IntoIterator>, destination_dir: Option<&Path>, @@ -397,7 +397,7 @@ fn scripted_fixture_repo_read_only_with_args_inner( } assert!( output.status.success(), - "repo script failed: stdout: {}\nstderr: {}", + "fixture script failed: stdout: {}\nstderr: {}", output.stdout.as_bstr(), output.stderr.as_bstr() ); From 4eb842c7150b980e1c2637217e1f9657a671cea7 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 12 Dec 2022 07:44:07 +0100 Subject: [PATCH 02/15] adjust to changes in `git-testtools` --- git-attributes/tests/match_group/mod.rs | 2 +- git-commitgraph/tests/commitgraph.rs | 4 ++-- git-config/tests/file/init/comfort.rs | 4 ++-- git-date/tests/time/baseline.rs | 2 +- git-diff/tests/tree/mod.rs | 2 +- git-discover/tests/is_git/mod.rs | 2 +- git-discover/tests/isolated.rs | 2 +- git-discover/tests/upwards/mod.rs | 8 ++++---- git-glob/tests/pattern/matching.rs | 2 +- git-index/tests/index/init.rs | 4 ++-- git-index/tests/index/mod.rs | 2 +- git-odb/tests/odb/mod.rs | 2 +- git-odb/tests/odb/regression/mod.rs | 2 +- git-odb/tests/odb/store/dynamic.rs | 6 +++--- git-pack/tests/pack/data/output/mod.rs | 2 +- git-pack/tests/pack/mod.rs | 2 +- git-pack/tests/pack/multi_index/mod.rs | 2 +- git-pathspec/tests/pathspec.rs | 2 +- git-ref/tests/file/log.rs | 8 +++----- git-ref/tests/file/mod.rs | 4 ++-- git-ref/tests/file/store/reflog.rs | 2 +- git-ref/tests/file/worktree.rs | 4 ++-- git-ref/tests/packed/iter.rs | 2 +- git-ref/tests/store/mod.rs | 2 +- git-refspec/tests/matching/mod.rs | 4 ++-- git-refspec/tests/parse/mod.rs | 4 ++-- .../src/remote/connection/fetch/update_refs/tests.rs | 10 +++++----- git-repository/tests/clone/mod.rs | 2 +- git-repository/tests/remote/fetch.rs | 8 ++++---- git-repository/tests/remote/mod.rs | 4 ++-- .../tests/repository/config/transport_options.rs | 2 +- git-repository/tests/repository/open.rs | 8 ++++---- git-repository/tests/repository/worktree.rs | 6 +++--- git-repository/tests/revision/spec/from_bytes/util.rs | 4 ++-- git-repository/tests/util/mod.rs | 8 ++++---- git-revision/tests/describe/mod.rs | 2 +- git-traverse/tests/commit/mod.rs | 4 ++-- git-traverse/tests/tree/mod.rs | 2 +- .../tests/worktree/fs/cache/ignore_and_attributes.rs | 4 ++-- git-worktree/tests/worktree/mod.rs | 3 +-- 40 files changed, 73 insertions(+), 76 deletions(-) diff --git a/git-attributes/tests/match_group/mod.rs b/git-attributes/tests/match_group/mod.rs index f70b448c233..950b27eacbe 100644 --- a/git-attributes/tests/match_group/mod.rs +++ b/git-attributes/tests/match_group/mod.rs @@ -32,7 +32,7 @@ mod ignore { #[test] fn from_git_dir() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_global_and_external_and_dir_ignores.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_global_and_external_and_dir_ignores.sh")?; let repo_dir = dir.join("repo"); let git_dir = repo_dir.join(".git"); let baseline = std::fs::read(git_dir.parent().unwrap().join("git-check-ignore.baseline"))?; diff --git a/git-commitgraph/tests/commitgraph.rs b/git-commitgraph/tests/commitgraph.rs index 777f3c4f46c..7119bcb3cb7 100644 --- a/git-commitgraph/tests/commitgraph.rs +++ b/git-commitgraph/tests/commitgraph.rs @@ -60,9 +60,9 @@ pub fn check_common(cg: &Graph, expected: &HashMap std::path::PathBuf { - scripted_fixture_repo_read_only(script_path).expect("script succeeds all the time") + scripted_fixture_read_only(script_path).expect("script succeeds all the time") } pub fn hex_to_id(hex: &[u8]) -> git_hash::ObjectId { diff --git a/git-config/tests/file/init/comfort.rs b/git-config/tests/file/init/comfort.rs index b07137278e3..d057ada7c18 100644 --- a/git-config/tests/file/init/comfort.rs +++ b/git-config/tests/file/init/comfort.rs @@ -21,7 +21,7 @@ fn from_environment_overrides() { #[test] #[serial] fn from_git_dir() -> crate::Result { - let worktree_dir = git_testtools::scripted_fixture_repo_read_only("make_config_repo.sh")?; + let worktree_dir = git_testtools::scripted_fixture_read_only("make_config_repo.sh")?; let git_dir = worktree_dir.join(".git"); let worktree_dir = worktree_dir.canonicalize()?; let _env = Env::new() @@ -84,7 +84,7 @@ fn from_git_dir() -> crate::Result { #[test] #[serial] fn from_git_dir_with_worktree_extension() -> crate::Result { - let git_dir = git_testtools::scripted_fixture_repo_read_only("config_with_worktree_extension.sh")? + let git_dir = git_testtools::scripted_fixture_read_only("config_with_worktree_extension.sh")? .join("main-worktree") .join(".git"); let config = git_config::File::from_git_dir(git_dir)?; diff --git a/git-date/tests/time/baseline.rs b/git-date/tests/time/baseline.rs index f993854853f..8bafb3eb641 100644 --- a/git-date/tests/time/baseline.rs +++ b/git-date/tests/time/baseline.rs @@ -13,7 +13,7 @@ struct Sample { static BASELINE: Lazy> = Lazy::new(|| { (|| -> Result<_> { - let base = git_testtools::scripted_fixture_repo_read_only("generate_git_date_baseline.sh")?; + let base = git_testtools::scripted_fixture_read_only("generate_git_date_baseline.sh")?; let mut map = HashMap::new(); let file = std::fs::read(base.join("baseline.git"))?; let baseline = std::str::from_utf8(&file).expect("valid utf"); diff --git a/git-diff/tests/tree/mod.rs b/git-diff/tests/tree/mod.rs index b4caf8b0b17..d04c1d593f6 100644 --- a/git-diff/tests/tree/mod.rs +++ b/git-diff/tests/tree/mod.rs @@ -11,7 +11,7 @@ mod changes { fn db(args: impl IntoIterator) -> crate::Result { git_odb::at( - git_testtools::scripted_fixture_repo_read_only_with_args("make_diff_repo.sh", args)? + git_testtools::scripted_fixture_read_only_with_args("make_diff_repo.sh", args)? .join(".git") .join("objects"), ) diff --git a/git-discover/tests/is_git/mod.rs b/git-discover/tests/is_git/mod.rs index d19b6b11731..412f6cce622 100644 --- a/git-discover/tests/is_git/mod.rs +++ b/git-discover/tests/is_git/mod.rs @@ -7,7 +7,7 @@ fn verify_on_exfat() -> crate::Result<()> { use git_discover::repository::Kind; - let fixtures = git_testtools::scripted_fixture_repo_read_only("make_exfat_repo_darwin.sh")?; + let fixtures = git_testtools::scripted_fixture_read_only("make_exfat_repo_darwin.sh")?; let mount_point = tempfile::tempdir()?; let _cleanup = { diff --git a/git-discover/tests/isolated.rs b/git-discover/tests/isolated.rs index 822d20bb307..feb90c62a0d 100644 --- a/git-discover/tests/isolated.rs +++ b/git-discover/tests/isolated.rs @@ -6,7 +6,7 @@ use serial_test::serial; #[test] #[serial] fn upwards_with_relative_directories_and_optional_ceiling() -> git_testtools::Result { - let repo = git_testtools::scripted_fixture_repo_read_only("make_basic_repo.sh")?; + let repo = git_testtools::scripted_fixture_read_only("make_basic_repo.sh")?; std::env::set_current_dir(repo.join("subdir"))?; let cwd = std::env::current_dir()?; diff --git a/git-discover/tests/upwards/mod.rs b/git-discover/tests/upwards/mod.rs index 4e882675210..37addfb118c 100644 --- a/git-discover/tests/upwards/mod.rs +++ b/git-discover/tests/upwards/mod.rs @@ -226,7 +226,7 @@ fn cross_fs() -> crate::Result { return Ok(()); } - let top_level_repo = git_testtools::scripted_fixture_repo_writable("make_basic_repo.sh")?; + let top_level_repo = git_testtools::scripted_fixture_writable("make_basic_repo.sh")?; let _cleanup = { // Create an empty dmg file @@ -307,7 +307,7 @@ fn do_not_shorten_absolute_paths() -> crate::Result { mod submodules { #[test] fn by_their_worktree_checkout() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_submodules.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_submodules.sh")?; let parent = dir.join("with-submodules"); let modules = parent.join(".git").join("modules"); for module in ["m1", "dir/m1"] { @@ -336,7 +336,7 @@ mod submodules { #[test] fn by_their_module_git_dir() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_submodules.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_submodules.sh")?; let modules = dir.join("with-submodules").join(".git").join("modules"); for module in ["m1", "dir/m1"] { let submodule_m1_gitdir = modules.join(module); @@ -353,5 +353,5 @@ mod submodules { } pub(crate) fn repo_path() -> crate::Result { - git_testtools::scripted_fixture_repo_read_only("make_basic_repo.sh") + git_testtools::scripted_fixture_read_only("make_basic_repo.sh") } diff --git a/git-glob/tests/pattern/matching.rs b/git-glob/tests/pattern/matching.rs index 4dddf804ecf..92f124461de 100644 --- a/git-glob/tests/pattern/matching.rs +++ b/git-glob/tests/pattern/matching.rs @@ -43,7 +43,7 @@ impl<'a> Baseline<'a> { #[test] fn compare_baseline_with_ours() { - let dir = git_testtools::scripted_fixture_repo_read_only("make_baseline.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_baseline.sh").unwrap(); let (mut total_matches, mut total_correct, mut panics) = (0, 0, 0); let mut mismatches = Vec::new(); for (input_file, expected_matches, case) in &[ diff --git a/git-index/tests/index/init.rs b/git-index/tests/index/init.rs index 0c83af5ff53..802b51d9b89 100644 --- a/git-index/tests/index/init.rs +++ b/git-index/tests/index/init.rs @@ -1,7 +1,7 @@ use git_index::{verify::extensions::no_find, State}; use git_repository as git; use git_repository::prelude::FindExt; -use git_testtools::scripted_fixture_repo_read_only; +use git_testtools::scripted_fixture_read_only; #[test] fn from_tree() -> crate::Result { @@ -13,7 +13,7 @@ fn from_tree() -> crate::Result { ]; for fixture in fixtures { - let repo_dir = scripted_fixture_repo_read_only(fixture)?; + let repo_dir = scripted_fixture_read_only(fixture)?; let repo = git::open(&repo_dir)?; let tree_id = repo.head_commit()?.tree_id()?; diff --git a/git-index/tests/index/mod.rs b/git-index/tests/index/mod.rs index 950a60d26d0..e2091712202 100644 --- a/git-index/tests/index/mod.rs +++ b/git-index/tests/index/mod.rs @@ -4,7 +4,7 @@ mod file; mod init; pub fn fixture_index_path(name: &str) -> PathBuf { - let dir = git_testtools::scripted_fixture_repo_read_only(Path::new("make_index").join(name).with_extension("sh")) + let dir = git_testtools::scripted_fixture_read_only(Path::new("make_index").join(name).with_extension("sh")) .expect("script works"); dir.join(".git").join("index") } diff --git a/git-odb/tests/odb/mod.rs b/git-odb/tests/odb/mod.rs index db18992ada5..3b4ddc87dac 100644 --- a/git-odb/tests/odb/mod.rs +++ b/git-odb/tests/odb/mod.rs @@ -1,4 +1,4 @@ -pub use git_testtools::{fixture_path, hex_to_id, scripted_fixture_repo_read_only}; +pub use git_testtools::{fixture_path, hex_to_id, scripted_fixture_read_only}; pub type Result = std::result::Result>; diff --git a/git-odb/tests/odb/regression/mod.rs b/git-odb/tests/odb/regression/mod.rs index 7e107ee0482..ed5e69d8a18 100644 --- a/git-odb/tests/odb/regression/mod.rs +++ b/git-odb/tests/odb/regression/mod.rs @@ -20,7 +20,7 @@ mod repo_with_small_packs { #[cfg(feature = "internal-testing-git-features-parallel")] fn multi_threaded_access_will_not_panic() { for arg in ["no", "without-multi-index"] { - let base = git_testtools::scripted_fixture_repo_read_only_with_args("make_repo_multi_index.sh", Some(arg)) + let base = git_testtools::scripted_fixture_read_only_with_args("make_repo_multi_index.sh", Some(arg)) .unwrap() .join(".git") .join("objects"); diff --git a/git-odb/tests/odb/store/dynamic.rs b/git-odb/tests/odb/store/dynamic.rs index d36dab2ea9c..3b7ebfcf5be 100644 --- a/git-odb/tests/odb/store/dynamic.rs +++ b/git-odb/tests/odb/store/dynamic.rs @@ -34,7 +34,7 @@ fn db_with_all_object_sources() -> crate::Result<(git_odb::Handle, tempfile::Tem #[test] fn multi_index_access() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_writable("make_repo_multi_index.sh")?; + let dir = git_testtools::scripted_fixture_writable("make_repo_multi_index.sh")?; let handle = git_odb::at(dir.path().join(".git/objects"))?; assert_eq!( @@ -130,7 +130,7 @@ fn multi_index_access() -> crate::Result { #[test] fn multi_index_keep_open() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_writable("make_repo_multi_index.sh")?; + let dir = git_testtools::scripted_fixture_writable("make_repo_multi_index.sh")?; let (stable_handle, handle) = { let mut stable_handle = git_odb::at(dir.path().join(".git/objects"))?; let handle = stable_handle.clone(); @@ -207,7 +207,7 @@ fn write() -> crate::Result { #[test] fn object_replacement() { - let dir = git_testtools::scripted_fixture_repo_read_only("make_replaced_history.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_replaced_history.sh").unwrap(); let handle = git_odb::at(dir.join(".git/objects")).unwrap(); let mut buf = Vec::new(); let short_history_link = hex_to_id("434e5a872d6738d1fffd1e11e52a1840b73668c6"); diff --git a/git-pack/tests/pack/data/output/mod.rs b/git-pack/tests/pack/data/output/mod.rs index 1d833eacd9e..5ca4e2a7ddc 100644 --- a/git-pack/tests/pack/data/output/mod.rs +++ b/git-pack/tests/pack/data/output/mod.rs @@ -31,7 +31,7 @@ fn db(kind: DbKind) -> crate::Result { DeterministicGeneratedContent => "make_pack_gen_repo.sh", DeterministicGeneratedContentMultiIndex => "make_pack_gen_repo_multi_index.sh", }; - let path: PathBuf = git_testtools::scripted_fixture_repo_read_only(name)? + let path: PathBuf = git_testtools::scripted_fixture_read_only(name)? .join(".git") .join("objects"); git_odb::Store::at_opts(path, Vec::new(), git_odb::store::init::Options::default()) diff --git a/git-pack/tests/pack/mod.rs b/git-pack/tests/pack/mod.rs index c1b3dbc7f4d..a23e066afbd 100644 --- a/git-pack/tests/pack/mod.rs +++ b/git-pack/tests/pack/mod.rs @@ -13,7 +13,7 @@ const PACKS_AND_INDICES: &[(&'static str, &'static str)] = const V2_PACKS_AND_INDICES: &[(&'static str, &'static str)] = &[(SMALL_PACK_INDEX, SMALL_PACK), (INDEX_V2, PACK_FOR_INDEX_V2)]; -pub use git_testtools::{fixture_path, hex_to_id, scripted_fixture_repo_read_only}; +pub use git_testtools::{fixture_path, hex_to_id, scripted_fixture_read_only}; pub type Result = std::result::Result>; diff --git a/git-pack/tests/pack/multi_index/mod.rs b/git-pack/tests/pack/multi_index/mod.rs index 8dc02bf728c..7975c7cd1b2 100644 --- a/git-pack/tests/pack/multi_index/mod.rs +++ b/git-pack/tests/pack/multi_index/mod.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use git_pack::multi_index::File; fn multi_index() -> (File, PathBuf) { - let path = git_testtools::scripted_fixture_repo_read_only("make_pack_gen_repo_multi_index.sh") + let path = git_testtools::scripted_fixture_read_only("make_pack_gen_repo_multi_index.sh") .expect("test fixture exists") .join(".git/objects/pack/multi-pack-index"); let file = git_pack::multi_index::File::at(&path).unwrap(); diff --git a/git-pathspec/tests/pathspec.rs b/git-pathspec/tests/pathspec.rs index 77e516a2ebc..54ba90bb09c 100644 --- a/git-pathspec/tests/pathspec.rs +++ b/git-pathspec/tests/pathspec.rs @@ -32,7 +32,7 @@ mod parse { } static BASELINE: Lazy> = Lazy::new(|| { - let base = git_testtools::scripted_fixture_repo_read_only("generate_pathspec_baseline.sh").unwrap(); + let base = git_testtools::scripted_fixture_read_only("generate_pathspec_baseline.sh").unwrap(); (|| -> crate::Result<_> { let mut map = HashMap::new(); diff --git a/git-ref/tests/file/log.rs b/git-ref/tests/file/log.rs index 81d07f713b3..4cef7c5d5d1 100644 --- a/git-ref/tests/file/log.rs +++ b/git-ref/tests/file/log.rs @@ -35,11 +35,9 @@ mod iter { use std::path::PathBuf; fn reflog_dir() -> crate::Result { - Ok( - git_testtools::scripted_fixture_repo_read_only("make_repo_for_reflog.sh")? - .join(".git") - .join("logs"), - ) + Ok(git_testtools::scripted_fixture_read_only("make_repo_for_reflog.sh")? + .join(".git") + .join("logs")) } fn reflog(name: &str) -> crate::Result> { Ok(std::fs::read(reflog_dir()?.join(name))?) diff --git a/git-ref/tests/file/mod.rs b/git-ref/tests/file/mod.rs index 88eaba711c5..d01037a8a7f 100644 --- a/git-ref/tests/file/mod.rs +++ b/git-ref/tests/file/mod.rs @@ -14,7 +14,7 @@ pub fn store_with_packed_refs() -> crate::Result { } pub fn store_at(name: &str) -> crate::Result { - let path = git_testtools::scripted_fixture_repo_read_only(name)?; + let path = git_testtools::scripted_fixture_read_only(name)?; Ok(Store::at( path.join(".git"), git_ref::store::WriteReflog::Normal, @@ -23,7 +23,7 @@ pub fn store_at(name: &str) -> crate::Result { } fn store_writable(name: &str) -> crate::Result<(git_testtools::tempfile::TempDir, Store)> { - let dir = git_testtools::scripted_fixture_repo_writable(name)?; + let dir = git_testtools::scripted_fixture_writable(name)?; let git_dir = dir.path().join(".git"); Ok(( dir, diff --git a/git-ref/tests/file/store/reflog.rs b/git-ref/tests/file/store/reflog.rs index 1330094acec..317a5a5c87d 100644 --- a/git-ref/tests/file/store/reflog.rs +++ b/git-ref/tests/file/store/reflog.rs @@ -1,6 +1,6 @@ fn store() -> crate::Result { Ok(crate::file::Store::at( - git_testtools::scripted_fixture_repo_read_only("make_repo_for_reflog.sh")?.join(".git"), + git_testtools::scripted_fixture_read_only("make_repo_for_reflog.sh")?.join(".git"), git_ref::store::WriteReflog::Disable, git_hash::Kind::Sha1, )) diff --git a/git-ref/tests/file/worktree.rs b/git-ref/tests/file/worktree.rs index 3ea4544a2b0..4a241939a0d 100644 --- a/git-ref/tests/file/worktree.rs +++ b/git-ref/tests/file/worktree.rs @@ -11,10 +11,10 @@ fn dir(packed: bool, writable: bool) -> crate::Result<(PathBuf, Option crate::Result { #[test] fn packed_refs_with_header() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_packed_ref_repository.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_packed_ref_repository.sh")?; let buf = std::fs::read(dir.join(".git").join("packed-refs"))?; let iter = packed::Iter::new(&buf)?; assert_eq!(iter.count(), 8, "it finds the right amount of items"); diff --git a/git-ref/tests/store/mod.rs b/git-ref/tests/store/mod.rs index 0e4c46b58dc..2d152649e08 100644 --- a/git-ref/tests/store/mod.rs +++ b/git-ref/tests/store/mod.rs @@ -2,7 +2,7 @@ #[cfg(feature = "internal-testing-git-features-parallel")] fn is_send_and_sync() { pub fn store_at(name: &str) -> crate::Result { - let path = git_testtools::scripted_fixture_repo_read_only(name)?; + let path = git_testtools::scripted_fixture_read_only(name)?; Ok(git_ref::file::Store::at( path.join(".git"), git_ref::store::WriteReflog::Normal, diff --git a/git-refspec/tests/matching/mod.rs b/git-refspec/tests/matching/mod.rs index 0d5807c27fe..816c36d18c0 100644 --- a/git-refspec/tests/matching/mod.rs +++ b/git-refspec/tests/matching/mod.rs @@ -204,7 +204,7 @@ pub mod baseline { } fn parse_input() -> crate::Result> { - let dir = git_testtools::scripted_fixture_repo_read_only("match_baseline.sh")?; + let dir = git_testtools::scripted_fixture_read_only("match_baseline.sh")?; let refs_buf = std::fs::read(dir.join("clone").join("remote-refs.list"))?; let mut out = Vec::new(); for line in refs_buf.lines() { @@ -228,7 +228,7 @@ pub mod baseline { } pub(crate) fn parse() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("match_baseline.sh")?; + let dir = git_testtools::scripted_fixture_read_only("match_baseline.sh")?; let buf = std::fs::read(dir.join("clone").join("baseline.git"))?; let mut map = HashMap::new(); diff --git a/git-refspec/tests/parse/mod.rs b/git-refspec/tests/parse/mod.rs index f7c0218f985..ce3ddf08031 100644 --- a/git-refspec/tests/parse/mod.rs +++ b/git-refspec/tests/parse/mod.rs @@ -2,11 +2,11 @@ use std::panic::catch_unwind; use bstr::ByteSlice; use git_refspec::parse::Operation; -use git_testtools::scripted_fixture_repo_read_only; +use git_testtools::scripted_fixture_read_only; #[test] fn baseline() { - let dir = scripted_fixture_repo_read_only("parse_baseline.sh").unwrap(); + let dir = scripted_fixture_read_only("parse_baseline.sh").unwrap(); let baseline = std::fs::read(dir.join("baseline.git")).unwrap(); let mut lines = baseline.lines(); let mut panics = 0; diff --git a/git-repository/src/remote/connection/fetch/update_refs/tests.rs b/git-repository/src/remote/connection/fetch/update_refs/tests.rs index 3d9e05ef59d..a148b0cdc8b 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/tests.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/tests.rs @@ -7,7 +7,7 @@ mod update { fn base_repo_path() -> String { git::path::realpath( - git_testtools::scripted_fixture_repo_read_only("make_remote_repos.sh") + git_testtools::scripted_fixture_read_only("make_remote_repos.sh") .unwrap() .join("base"), ) @@ -17,12 +17,12 @@ mod update { } fn repo(name: &str) -> git::Repository { - let dir = git_testtools::scripted_fixture_repo_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]) - .unwrap(); + let dir = + git_testtools::scripted_fixture_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]).unwrap(); git::open_opts(dir.join(name), git::open::Options::isolated()).unwrap() } fn repo_rw(name: &str) -> (git::Repository, git_testtools::tempfile::TempDir) { - let dir = git_testtools::scripted_fixture_repo_writable_with_args( + let dir = git_testtools::scripted_fixture_writable_with_args( "make_fetch_repos.sh", [base_repo_path()], git_testtools::Creation::ExecuteScript, @@ -169,7 +169,7 @@ mod update { #[test] fn checked_out_branches_in_worktrees_are_rejected_with_additional_information() -> Result { - let root = git_path::realpath(git_testtools::scripted_fixture_repo_read_only_with_args( + let root = git_path::realpath(git_testtools::scripted_fixture_read_only_with_args( "make_fetch_repos.sh", [base_repo_path()], )?)?; diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 862f2af0db1..0d286d194e1 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -199,7 +199,7 @@ mod blocking_io { fn fetch_and_checkout_empty_remote_repo() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; let mut prepare = git::prepare_clone( - git_testtools::scripted_fixture_repo_read_only("make_empty_repo.sh")?, + git_testtools::scripted_fixture_read_only("make_empty_repo.sh")?, tmp.path(), )?; let (mut checkout, out) = prepare diff --git a/git-repository/tests/remote/fetch.rs b/git-repository/tests/remote/fetch.rs index 10654c57fd9..8ef0acc3bd5 100644 --- a/git-repository/tests/remote/fetch.rs +++ b/git-repository/tests/remote/fetch.rs @@ -12,7 +12,7 @@ mod blocking_and_async_io { pub(crate) fn base_repo_path() -> String { git::path::realpath( - git_testtools::scripted_fixture_repo_read_only("make_remote_repos.sh") + git_testtools::scripted_fixture_read_only("make_remote_repos.sh") .unwrap() .join("base"), ) @@ -22,13 +22,13 @@ mod blocking_and_async_io { } pub(crate) fn repo_path(name: &str) -> std::path::PathBuf { - let dir = git_testtools::scripted_fixture_repo_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]) - .unwrap(); + let dir = + git_testtools::scripted_fixture_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]).unwrap(); dir.join(name) } pub(crate) fn repo_rw(name: &str) -> (git::Repository, git_testtools::tempfile::TempDir) { - let dir = git_testtools::scripted_fixture_repo_writable_with_args( + let dir = git_testtools::scripted_fixture_writable_with_args( "make_fetch_repos.sh", [base_repo_path()], git_testtools::Creation::ExecuteScript, diff --git a/git-repository/tests/remote/mod.rs b/git-repository/tests/remote/mod.rs index c9584a77c61..d832bc32c05 100644 --- a/git-repository/tests/remote/mod.rs +++ b/git-repository/tests/remote/mod.rs @@ -1,10 +1,10 @@ use std::{borrow::Cow, path::PathBuf}; use git_repository as git; -use git_testtools::scripted_fixture_repo_read_only; +use git_testtools::scripted_fixture_read_only; pub(crate) fn repo_path(name: &str) -> PathBuf { - let dir = scripted_fixture_repo_read_only("make_remote_repos.sh").unwrap(); + let dir = scripted_fixture_read_only("make_remote_repos.sh").unwrap(); dir.join(name) } diff --git a/git-repository/tests/repository/config/transport_options.rs b/git-repository/tests/repository/config/transport_options.rs index d3786f96321..ec3c708f6cf 100644 --- a/git-repository/tests/repository/config/transport_options.rs +++ b/git-repository/tests/repository/config/transport_options.rs @@ -7,7 +7,7 @@ mod http { use git_transport::client::http::options::{FollowRedirects, ProxyAuthMethod}; pub(crate) fn repo(name: &str) -> git::Repository { - let dir = git_testtools::scripted_fixture_repo_read_only("make_config_repos.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_config_repos.sh").unwrap(); git::open_opts(dir.join(name), git::open::Options::isolated()).unwrap() } diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index ef55776eb24..d2d7aabc198 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -38,7 +38,7 @@ mod not_a_repository { fn shows_proper_error() -> crate::Result { for name in ["empty-dir", "with-files"] { let name = format!("not-a-repo-{name}"); - let repo_path = git_testtools::scripted_fixture_repo_read_only("make_config_repos.sh")?.join(name); + let repo_path = git_testtools::scripted_fixture_read_only("make_config_repos.sh")?.join(name); let err = git::open_opts(&repo_path, git::open::Options::isolated()).unwrap_err(); assert!(matches!(err, git::open::Error::NotARepository { path, .. } if path == repo_path)); } @@ -53,7 +53,7 @@ mod submodules { #[test] fn by_their_worktree_checkout_and_git_modules_dir() { - let dir = git_testtools::scripted_fixture_repo_read_only("make_submodules.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_submodules.sh").unwrap(); let parent_repo = Path::new("with-submodules"); let modules = parent_repo.join(".git").join("modules"); for module in ["m1", "dir/m1"] { @@ -79,7 +79,7 @@ mod submodules { } fn discover_repo(name: impl AsRef) -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_submodules.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_submodules.sh")?; let repo_dir = dir.join(name); Ok(git::ThreadSafeRepository::discover_opts( repo_dir, @@ -280,7 +280,7 @@ mod worktree { #[test] fn with_worktree_configs() -> git_testtools::Result { let manifest_dir = std::path::PathBuf::from(std::env::var("CARGO_MANIFEST_DIR")?); - let fixture_dir = git_testtools::scripted_fixture_repo_read_only("make_worktree_repo_with_configs.sh")?; + let fixture_dir = git_testtools::scripted_fixture_read_only("make_worktree_repo_with_configs.sh")?; let worktree_base = manifest_dir.join(&fixture_dir).join("repo/.git/worktrees"); { diff --git a/git-repository/tests/repository/worktree.rs b/git-repository/tests/repository/worktree.rs index d9e07ac1118..215ffc031c4 100644 --- a/git-repository/tests/repository/worktree.rs +++ b/git-repository/tests/repository/worktree.rs @@ -100,7 +100,7 @@ mod with_core_worktree_config { } fn repo(name: &str) -> git::Repository { - let dir = git_testtools::scripted_fixture_repo_read_only("make_core_worktree_repo.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_core_worktree_repo.sh").unwrap(); git::open_opts(dir.join(name), crate::restricted()).unwrap() } @@ -197,7 +197,7 @@ fn from_bare_parent_repo() { if git_testtools::should_skip_as_git_version_is_smaller_than(2, 31, 0) { return; } - let dir = git_testtools::scripted_fixture_repo_read_only_with_args("make_worktree_repo.sh", ["bare"]).unwrap(); + let dir = git_testtools::scripted_fixture_read_only_with_args("make_worktree_repo.sh", ["bare"]).unwrap(); let repo = git::open(dir.join("repo.git")).unwrap(); run_assertions(repo, true /* bare */); @@ -208,7 +208,7 @@ fn from_nonbare_parent_repo() { if git_testtools::should_skip_as_git_version_is_smaller_than(2, 31, 0) { return; } - let dir = git_testtools::scripted_fixture_repo_read_only("make_worktree_repo.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_worktree_repo.sh").unwrap(); let repo = git::open(dir.join("repo")).unwrap(); run_assertions(repo, false /* bare */); diff --git a/git-repository/tests/revision/spec/from_bytes/util.rs b/git-repository/tests/revision/spec/from_bytes/util.rs index a72299c7980..4ec8a6df975 100644 --- a/git-repository/tests/revision/spec/from_bytes/util.rs +++ b/git-repository/tests/revision/spec/from_bytes/util.rs @@ -40,7 +40,7 @@ static BASELINE: Lazy( } pub fn repo(name: &str) -> crate::Result { - let base = git_testtools::scripted_fixture_repo_read_only(FIXTURE_NAME)?; + let base = git_testtools::scripted_fixture_read_only(FIXTURE_NAME)?; Ok(git::open(base.join(name))?) } diff --git a/git-repository/tests/util/mod.rs b/git-repository/tests/util/mod.rs index 3518ed5926e..ab481898e49 100644 --- a/git-repository/tests/util/mod.rs +++ b/git-repository/tests/util/mod.rs @@ -14,17 +14,17 @@ pub fn freeze_time() -> git_testtools::Env<'static> { .set("GIT_COMMITTER_DATE", frozen_time) } pub fn repo(name: &str) -> Result { - let repo_path = git_testtools::scripted_fixture_repo_read_only(name)?; + let repo_path = git_testtools::scripted_fixture_read_only(name)?; Ok(ThreadSafeRepository::open_opts(repo_path, restricted())?) } pub fn named_repo(name: &str) -> Result { - let repo_path = git_testtools::scripted_fixture_repo_read_only(name)?; + let repo_path = git_testtools::scripted_fixture_read_only(name)?; Ok(ThreadSafeRepository::open_opts(repo_path, restricted())?.to_thread_local()) } pub fn named_subrepo_opts(fixture: &str, name: &str, opts: open::Options) -> Result { - let repo_path = git_testtools::scripted_fixture_repo_read_only(fixture)?.join(name); + let repo_path = git_testtools::scripted_fixture_read_only(fixture)?.join(name); Ok(ThreadSafeRepository::open_opts(repo_path, opts)?.to_thread_local()) } @@ -44,7 +44,7 @@ pub fn repo_rw(name: &str) -> Result<(Repository, tempfile::TempDir)> { } pub fn repo_rw_opts(name: &str, opts: git_repository::open::Options) -> Result<(Repository, tempfile::TempDir)> { - let repo_path = git_testtools::scripted_fixture_repo_writable(name)?; + let repo_path = git_testtools::scripted_fixture_writable(name)?; Ok(( ThreadSafeRepository::discover_opts( repo_path.path(), diff --git a/git-revision/tests/describe/mod.rs b/git-revision/tests/describe/mod.rs index 428481e1c56..6562af77ba6 100644 --- a/git-revision/tests/describe/mod.rs +++ b/git-revision/tests/describe/mod.rs @@ -194,6 +194,6 @@ fn typical_usecases() { } fn repo() -> Repository { - let dir = git_testtools::scripted_fixture_repo_read_only("make_repo_with_branches.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_repo_with_branches.sh").unwrap(); git_repository::open(dir).unwrap() } diff --git a/git-traverse/tests/commit/mod.rs b/git-traverse/tests/commit/mod.rs index 97292b79944..0203d5452c4 100644 --- a/git-traverse/tests/commit/mod.rs +++ b/git-traverse/tests/commit/mod.rs @@ -37,7 +37,7 @@ mod ancestor { impl TraversalAssertion<'_> { fn setup(&self) -> crate::Result<(git_odb::Handle, Vec, Vec)> { - let dir = git_testtools::scripted_fixture_repo_read_only(self.init_script)?; + let dir = git_testtools::scripted_fixture_read_only(self.init_script)?; let store = git_odb::at(dir.join(".git").join("objects"))?; let tips: Vec<_> = self.tips.iter().copied().map(hex_to_id).collect(); let expected: Vec = tips @@ -237,7 +237,7 @@ mod ancestor { #[test] fn committer_date_sorted_commits_with_cutoff_is_applied_to_starting_position() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_traversal_repo_for_commits_with_dates.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_traversal_repo_for_commits_with_dates.sh")?; let store = git_odb::at(dir.join(".git").join("objects"))?; let iter = commit::Ancestors::new( Some(hex_to_id("9902e3c3e8f0c569b4ab295ddf473e6de763e1e7")), diff --git a/git-traverse/tests/tree/mod.rs b/git-traverse/tests/tree/mod.rs index b3f9fe3722f..3a4aa32c76a 100644 --- a/git-traverse/tests/tree/mod.rs +++ b/git-traverse/tests/tree/mod.rs @@ -4,7 +4,7 @@ use git_traverse::tree; use crate::hex_to_id; fn db() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_traversal_repo_for_trees.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_traversal_repo_for_trees.sh")?; let db = git_odb::at(dir.join(".git").join("objects"))?; Ok(db) } diff --git a/git-worktree/tests/worktree/fs/cache/ignore_and_attributes.rs b/git-worktree/tests/worktree/fs/cache/ignore_and_attributes.rs index 14c4ff05ef7..a03413120f1 100644 --- a/git-worktree/tests/worktree/fs/cache/ignore_and_attributes.rs +++ b/git-worktree/tests/worktree/fs/cache/ignore_and_attributes.rs @@ -35,7 +35,7 @@ impl<'a> Iterator for IgnoreExpectations<'a> { #[test] fn special_exclude_cases_we_handle_differently() { - let dir = git_testtools::scripted_fixture_repo_read_only("make_special_exclude_case.sh").unwrap(); + let dir = git_testtools::scripted_fixture_read_only("make_special_exclude_case.sh").unwrap(); let git_dir = dir.join(".git"); let mut buf = Vec::new(); @@ -85,7 +85,7 @@ fn special_exclude_cases_we_handle_differently() { #[test] fn check_against_baseline() -> crate::Result { - let dir = git_testtools::scripted_fixture_repo_read_only("make_ignore_and_attributes_setup.sh")?; + let dir = git_testtools::scripted_fixture_read_only("make_ignore_and_attributes_setup.sh")?; let worktree_dir = dir.join("repo"); let git_dir = worktree_dir.join(".git"); let mut buf = Vec::new(); diff --git a/git-worktree/tests/worktree/mod.rs b/git-worktree/tests/worktree/mod.rs index 2a9fb1aecc0..ca6aa07a841 100644 --- a/git-worktree/tests/worktree/mod.rs +++ b/git-worktree/tests/worktree/mod.rs @@ -5,7 +5,6 @@ use std::path::{Path, PathBuf}; pub type Result = std::result::Result>; pub fn fixture_path(name: &str) -> PathBuf { - let dir = - git_testtools::scripted_fixture_repo_read_only(Path::new(name).with_extension("sh")).expect("script works"); + let dir = git_testtools::scripted_fixture_read_only(Path::new(name).with_extension("sh")).expect("script works"); dir } From 15ecd841cfe7c77bbdfdfa232dd51a44c4940bbc Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 13 Dec 2022 09:18:43 +0100 Subject: [PATCH 03/15] feat: allow execution of scripts without 'bash'. (#650) This works by trying to execute the file directly, and on failure, use 'bash' as interpreter. That way we are finally able to support a wider variety of fixture generators and make the crate more useful to a wieder audience. --- tests/tools/src/lib.rs | 69 +++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 0970067a7e9..dfb0c1ed9f4 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -205,7 +205,7 @@ pub fn fixture_bytes(path: impl AsRef) -> Vec { } } -/// Run the script file `script_name` using `bash`, like `make_repo.sh` to produce a read-only directory to which +/// Run the executable at `script_name`, like `make_repo.sh` or `my_setup.py` to produce a read-only directory to which /// the path is returned. /// /// Note that it persists and the script at `script_name` will only be executed once if it ran without error. @@ -366,32 +366,17 @@ fn scripted_fixture_read_only_with_args_inner( ); } let script_absolute_path = std::env::current_dir()?.join(script_path); - let output = std::process::Command::new("bash") - .arg(script_absolute_path) - .args(args) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .current_dir(&script_result_directory) - .env_remove("GIT_DIR") - .env_remove("GIT_ASKPASS") - .env_remove("SSH_ASKPASS") - .env("GIT_TERMINAL_PROMPT", "false") - .env("GIT_AUTHOR_DATE", "2000-01-01 00:00:00 +0000") - .env("GIT_AUTHOR_EMAIL", "author@example.com") - .env("GIT_AUTHOR_NAME", "author") - .env("GIT_COMMITTER_DATE", "2000-01-02 00:00:00 +0000") - .env("GIT_COMMITTER_EMAIL", "committer@example.com") - .env("GIT_COMMITTER_NAME", "committer") - .env("GIT_CONFIG_COUNT", "4") - .env("GIT_CONFIG_KEY_0", "commit.gpgsign") - .env("GIT_CONFIG_VALUE_0", "false") - .env("GIT_CONFIG_KEY_1", "tag.gpgsign") - .env("GIT_CONFIG_VALUE_1", "false") - .env("GIT_CONFIG_KEY_2", "init.defaultBranch") - .env("GIT_CONFIG_VALUE_2", "main") - .env("GIT_CONFIG_KEY_3", "protocol.file.allow") - .env("GIT_CONFIG_VALUE_3", "always") - .output()?; + let mut cmd = std::process::Command::new(&script_absolute_path); + let output = match configure_command(&mut cmd, &args, &script_result_directory).output() { + Ok(out) => out, + Err(err) + if err.kind() == std::io::ErrorKind::PermissionDenied || err.raw_os_error() == Some(193) /* windows */ => + { + cmd = std::process::Command::new("bash"); + configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()? + } + Err(err) => return Err(err.into()), + }; if !output.status.success() { write_failure_marker(&failure_marker); } @@ -413,6 +398,36 @@ fn scripted_fixture_read_only_with_args_inner( Ok(script_result_directory) } +fn configure_command<'a>( + cmd: &'a mut std::process::Command, + args: &[String], + script_result_directory: &Path, +) -> &'a mut std::process::Command { + cmd.args(args) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .current_dir(script_result_directory) + .env_remove("GIT_DIR") + .env_remove("GIT_ASKPASS") + .env_remove("SSH_ASKPASS") + .env("GIT_TERMINAL_PROMPT", "false") + .env("GIT_AUTHOR_DATE", "2000-01-01 00:00:00 +0000") + .env("GIT_AUTHOR_EMAIL", "author@example.com") + .env("GIT_AUTHOR_NAME", "author") + .env("GIT_COMMITTER_DATE", "2000-01-02 00:00:00 +0000") + .env("GIT_COMMITTER_EMAIL", "committer@example.com") + .env("GIT_COMMITTER_NAME", "committer") + .env("GIT_CONFIG_COUNT", "4") + .env("GIT_CONFIG_KEY_0", "commit.gpgsign") + .env("GIT_CONFIG_VALUE_0", "false") + .env("GIT_CONFIG_KEY_1", "tag.gpgsign") + .env("GIT_CONFIG_VALUE_1", "false") + .env("GIT_CONFIG_KEY_2", "init.defaultBranch") + .env("GIT_CONFIG_VALUE_2", "main") + .env("GIT_CONFIG_KEY_3", "protocol.file.allow") + .env("GIT_CONFIG_VALUE_3", "always") +} + fn write_failure_marker(failure_marker: &Path) { std::fs::write(failure_marker, []).ok(); } From a8727824a4c38b27d6ad7c73ccaf7a45839c0aa9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 13 Dec 2022 21:44:24 +0100 Subject: [PATCH 04/15] fix: don't pass 'include-tag' as argument by default. This flag always has to be added by the caller based on configuration. --- git-protocol/src/command/mod.rs | 2 +- git-protocol/src/command/tests.rs | 2 +- git-protocol/src/fetch/tests.rs | 4 ---- git-protocol/src/handshake/mod.rs | 2 +- git-protocol/tests/fetch/v2.rs | 1 - 5 files changed, 3 insertions(+), 8 deletions(-) diff --git a/git-protocol/src/command/mod.rs b/git-protocol/src/command/mod.rs index 935717e5419..758309e7da2 100644 --- a/git-protocol/src/command/mod.rs +++ b/git-protocol/src/command/mod.rs @@ -94,7 +94,7 @@ mod with_io { /// Only useful for V2 pub(crate) fn initial_arguments(&self, features: &[Feature]) -> Vec { match self { - Command::Fetch => ["thin-pack", "include-tag", "ofs-delta"] + Command::Fetch => ["thin-pack", "ofs-delta"] .iter() .map(|s| s.as_bytes().as_bstr().to_owned()) .chain( diff --git a/git-protocol/src/command/tests.rs b/git-protocol/src/command/tests.rs index ac4b81494c9..5ac9401769d 100644 --- a/git-protocol/src/command/tests.rs +++ b/git-protocol/src/command/tests.rs @@ -88,7 +88,7 @@ mod v2 { git_transport::Protocol::V2, &capabilities("fetch", "shallow filter sideband-all packfile-uris") )), - ["thin-pack", "include-tag", "ofs-delta", "sideband-all"] + ["thin-pack", "ofs-delta", "sideband-all"] .iter() .map(|s| s.as_bytes().as_bstr().to_owned()) .collect::>(), diff --git a/git-protocol/src/fetch/tests.rs b/git-protocol/src/fetch/tests.rs index bc142bbfdd8..950cf849607 100644 --- a/git-protocol/src/fetch/tests.rs +++ b/git-protocol/src/fetch/tests.rs @@ -260,7 +260,6 @@ mod arguments { out.as_bstr(), b"0012command=fetch 0001000ethin-pack -0010include-tag 000eofs-delta 000ddeepen 1 0014deepen-relative @@ -294,7 +293,6 @@ mod arguments { out.as_bstr(), b"0012command=fetch 0001000ethin-pack -0010include-tag 000eofs-delta 000ddeepen 1 0017deepen-since 12345 @@ -304,7 +302,6 @@ mod arguments { 0032have 0000000000000000000000000000000000000000 00000012command=fetch 0001000ethin-pack -0010include-tag 000eofs-delta 000ddeepen 1 0017deepen-since 12345 @@ -332,7 +329,6 @@ mod arguments { out.as_bstr(), b"0012command=fetch 0001000ethin-pack -0010include-tag 000eofs-delta 001dwant-ref refs/heads/main 0009done diff --git a/git-protocol/src/handshake/mod.rs b/git-protocol/src/handshake/mod.rs index 49e1a87f827..bf293d280eb 100644 --- a/git-protocol/src/handshake/mod.rs +++ b/git-protocol/src/handshake/mod.rs @@ -16,7 +16,7 @@ pub enum Ref { }, /// A ref pointing to a commit object Direct { - /// The name at which the ref is located, like `refs/heads/main`. + /// The name at which the ref is located, like `refs/heads/main` or `refs/tags/v1.0` for lightweight tags. full_ref_name: BString, /// The hash of the object the ref points to. object: git_hash::ObjectId, diff --git a/git-protocol/tests/fetch/v2.rs b/git-protocol/tests/fetch/v2.rs index 90a751bf338..08a598557c6 100644 --- a/git-protocol/tests/fetch/v2.rs +++ b/git-protocol/tests/fetch/v2.rs @@ -185,7 +185,6 @@ async fn ref_in_want() -> crate::Result { "002fgit-upload-pack does/not/matter\0\0version=2\00012command=fetch 0014agent={} 0001000ethin-pack -0010include-tag 000eofs-delta 001dwant-ref refs/heads/main 0009done From 5a50d9599f820237866928df6d24a196a9b86fe3 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 14 Dec 2022 19:58:31 +0100 Subject: [PATCH 05/15] feat: `fetch::Arguments::use_include_tag()` allows to signal `include-tag`. One can also check for its availability using `fetch::Arguments::can_use_include_tag()`. Further there was a bugfix that assues V1 capabilities are correctly interpreted to support include-tag. --- git-protocol/src/command/mod.rs | 2 +- git-protocol/src/command/tests.rs | 3 +- git-protocol/src/fetch/arguments/mod.rs | 62 +++++++++++++++++++------ git-protocol/src/fetch/tests.rs | 48 +++++++++++++++++++ 4 files changed, 98 insertions(+), 17 deletions(-) diff --git a/git-protocol/src/command/mod.rs b/git-protocol/src/command/mod.rs index 758309e7da2..3a885d31568 100644 --- a/git-protocol/src/command/mod.rs +++ b/git-protocol/src/command/mod.rs @@ -129,7 +129,7 @@ mod with_io { .filter(|feature| match *feature { "side-band" if has_sideband_64k => false, "multi_ack" if has_multi_ack_detailed => false, - "include-tag" | "no-progress" => false, + "no-progress" => false, feature => server_capabilities.contains(feature), }) .map(|s| (s, None)) diff --git a/git-protocol/src/command/tests.rs b/git-protocol/src/command/tests.rs index 5ac9401769d..63162b41ed1 100644 --- a/git-protocol/src/command/tests.rs +++ b/git-protocol/src/command/tests.rs @@ -37,12 +37,13 @@ mod v1 { ("deepen-since", None), ("deepen-not", None), ("deepen-relative", None), + ("include-tag", None), ("allow-tip-sha1-in-want", None), ("allow-reachable-sha1-in-want", None), ("no-done", None), ("filter", None), ], - "we don't enforce include-tag or no-progress" + "we don't enforce no-progress" ); } } diff --git a/git-protocol/src/fetch/arguments/mod.rs b/git-protocol/src/fetch/arguments/mod.rs index 1fd30336044..957c271eacf 100644 --- a/git-protocol/src/fetch/arguments/mod.rs +++ b/git-protocol/src/fetch/arguments/mod.rs @@ -18,6 +18,7 @@ pub struct Arguments { deepen_not: bool, deepen_relative: bool, ref_in_want: bool, + supports_include_tag: bool, features_for_first_want: Option>, #[cfg(any(feature = "async-client", feature = "blocking-client"))] @@ -73,6 +74,10 @@ impl Arguments { pub fn can_use_ref_in_want(&self) -> bool { self.ref_in_want } + /// Return true if the 'include-tag' capability is supported. + pub fn can_use_include_tag(&self) -> bool { + self.supports_include_tag + } /// Add the given `id` pointing to a commit to the 'want' list. /// @@ -100,35 +105,56 @@ impl Arguments { } /// Add the given `id` pointing to a commit to the 'shallow' list. pub fn shallow(&mut self, id: impl AsRef) { - assert!(self.shallow, "'shallow' feature required for 'shallow '"); - self.prefixed("shallow ", id.as_ref()); + debug_assert!(self.shallow, "'shallow' feature required for 'shallow '"); + if self.shallow { + self.prefixed("shallow ", id.as_ref()); + } } /// Deepen the commit history by `depth` amount of commits. pub fn deepen(&mut self, depth: usize) { - assert!(self.shallow, "'shallow' feature required for deepen"); - self.prefixed("deepen ", depth); + debug_assert!(self.shallow, "'shallow' feature required for deepen"); + if self.shallow { + self.prefixed("deepen ", depth); + } } /// Deepen the commit history to include all commits from now to `seconds_since_unix_epoch`. pub fn deepen_since(&mut self, seconds_since_unix_epoch: usize) { - assert!(self.deepen_since, "'deepen-since' feature required"); - self.prefixed("deepen-since ", seconds_since_unix_epoch); + debug_assert!(self.deepen_since, "'deepen-since' feature required"); + if self.deepen_since { + self.prefixed("deepen-since ", seconds_since_unix_epoch); + } } /// Deepen the commit history in a relative instead of absolute fashion. pub fn deepen_relative(&mut self) { - assert!(self.deepen_relative, "'deepen-relative' feature required"); - self.args.push("deepen-relative".into()); + debug_assert!(self.deepen_relative, "'deepen-relative' feature required"); + if self.deepen_relative { + self.args.push("deepen-relative".into()); + } } /// Do not include commits reachable by the given `ref_path` when deepening the history. pub fn deepen_not(&mut self, ref_path: &BStr) { - assert!(self.deepen_not, "'deepen-not' feature required"); - let mut line = BString::from("deepen-not "); - line.extend_from_slice(ref_path); - self.args.push(line); + debug_assert!(self.deepen_not, "'deepen-not' feature required"); + if self.deepen_not { + let mut line = BString::from("deepen-not "); + line.extend_from_slice(ref_path); + self.args.push(line); + } } /// Set the given filter `spec` when listing references. pub fn filter(&mut self, spec: &str) { - assert!(self.filter, "'filter' feature required"); - self.prefixed("filter ", spec); + debug_assert!(self.filter, "'filter' feature required"); + if self.filter { + self.prefixed("filter ", spec); + } + } + /// Permanently allow the server to include tags that point to commits or objects it would return. + /// + /// Needs to only be called once. + pub fn use_include_tag(&mut self) { + debug_assert!(self.supports_include_tag, "'include-tag' feature required"); + if self.supports_include_tag { + self.args.push("include-tag".into()); + } } fn prefixed(&mut self, prefix: &str, value: impl fmt::Display) { self.args.push(format!("{}{}", prefix, value).into()); @@ -145,11 +171,13 @@ impl Arguments { let mut deepen_since = shallow; let mut deepen_not = shallow; let mut deepen_relative = shallow; + let supports_include_tag; let (initial_arguments, features_for_first_want) = match version { git_transport::Protocol::V1 => { deepen_since = has("deepen-since"); deepen_not = has("deepen-not"); deepen_relative = has("deepen-relative"); + supports_include_tag = has("include-tag"); let baked_features = features .iter() .map(|(n, v)| match v { @@ -159,7 +187,10 @@ impl Arguments { .collect::>(); (Vec::new(), Some(baked_features)) } - git_transport::Protocol::V2 => (Command::Fetch.initial_arguments(&features), None), + git_transport::Protocol::V2 => { + supports_include_tag = true; + (Command::Fetch.initial_arguments(&features), None) + } }; Arguments { @@ -169,6 +200,7 @@ impl Arguments { haves: Vec::new(), filter, shallow, + supports_include_tag, deepen_not, deepen_relative, ref_in_want, diff --git a/git-protocol/src/fetch/tests.rs b/git-protocol/src/fetch/tests.rs index 950cf849607..b2ad063bb0d 100644 --- a/git-protocol/src/fetch/tests.rs +++ b/git-protocol/src/fetch/tests.rs @@ -159,11 +159,35 @@ mod arguments { use crate::fetch::tests::arguments::{arguments_v1, id, transport}; + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn include_tag() { + let mut out = Vec::new(); + let mut t = transport(&mut out, true); + let mut arguments = arguments_v1(["include-tag", "feature-b"].iter().cloned()); + assert!(arguments.can_use_include_tag()); + + arguments.use_include_tag(); + arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0048want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff include-tag feature-b +0010include-tag +00000009done +" + .as_bstr() + ); + } + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] async fn haves_and_wants_for_clone() { let mut out = Vec::new(); let mut t = transport(&mut out, true); let mut arguments = arguments_v1(["feature-a", "feature-b"].iter().cloned()); + assert!( + !arguments.can_use_include_tag(), + "needs to be enabled by features in V1" + ); arguments.want(id("7b333369de1221f9bfbbe03a3a13e9a09bc1c907")); arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); @@ -245,6 +269,30 @@ mod arguments { use crate::fetch::tests::arguments::{arguments_v2, id, transport}; + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] + async fn include_tag() { + let mut out = Vec::new(); + let mut t = transport(&mut out, true); + let mut arguments = arguments_v2(["does not matter for us here"].iter().copied()); + assert!(arguments.can_use_include_tag(), "always on in V2"); + arguments.use_include_tag(); + + arguments.want(id("ff333369de1221f9bfbbe03a3a13e9a09bc1ffff")); + arguments.send(&mut t, true).await.expect("sending to buffer to work"); + assert_eq!( + out.as_bstr(), + b"0012command=fetch +0001000ethin-pack +000eofs-delta +0010include-tag +0032want ff333369de1221f9bfbbe03a3a13e9a09bc1ffff +0009done +0000" + .as_bstr(), + "we filter features/capabilities without value as these apparently shouldn't be listed (remote dies otherwise)" + ); + } + #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] async fn haves_and_wants_for_clone_stateful() { let mut out = Vec::new(); From 90ef6fc36b440cc4baf3fde4a30060f1b4a0c8cf Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 12 Dec 2022 10:57:39 +0100 Subject: [PATCH 06/15] feat: `Remote` knows about its `tagOpt` configuration. That way it's clear if it should or shouldn't fetch included/reachable tags automatically. The default setting for this is to include tags, similar to `git`. The `fetch_tags()` accessor allows to query this information, and the `with_fetch_tags()` builder method allows to set the value comfortably right after creating the `Remote` instance. The `tagOpt` key will also be written as part of the remote's git configuration. Clone operations can set the `Tags` setting when configuring the remote in a callback. This also comes with a fix to assure that ref-updates aren't skipped just because there was no pack to receive. That way, locally missing refs or tags will automatically be put back. --- git-repository/src/clone/fetch/mod.rs | 55 ++++-- git-repository/src/clone/mod.rs | 3 +- git-repository/src/remote/access.rs | 5 + git-repository/src/remote/build.rs | 6 + .../src/remote/connection/fetch/mod.rs | 9 +- .../src/remote/connection/fetch/negotiate.rs | 17 +- .../remote/connection/fetch/receive_pack.rs | 23 ++- .../connection/fetch/update_refs/mod.rs | 28 ++- .../connection/fetch/update_refs/tests.rs | 29 ++- .../connection/fetch/update_refs/update.rs | 9 +- .../src/remote/connection/ref_map.rs | 36 +++- git-repository/src/remote/errors.rs | 4 + git-repository/src/remote/fetch.rs | 86 ++++++++- git-repository/src/remote/init.rs | 4 + git-repository/src/remote/mod.rs | 1 - git-repository/src/remote/save.rs | 28 ++- git-repository/src/repository/remote.rs | 25 ++- git-repository/src/types.rs | 2 + git-repository/tests/clone/mod.rs | 83 +++++++-- .../tests/fixtures/make_remote_repos.sh | 21 +++ git-repository/tests/remote/fetch.rs | 165 +++++++++++------- git-repository/tests/remote/mod.rs | 3 +- git-repository/tests/remote/ref_map.rs | 60 +++++-- git-repository/tests/remote/save.rs | 7 +- git-repository/tests/repository/remote.rs | 29 ++- 25 files changed, 586 insertions(+), 152 deletions(-) diff --git a/git-repository/src/clone/fetch/mod.rs b/git-repository/src/clone/fetch/mod.rs index 8961cb92bcd..4b1296f93b7 100644 --- a/git-repository/src/clone/fetch/mod.rs +++ b/git-repository/src/clone/fetch/mod.rs @@ -12,7 +12,9 @@ pub enum Error { #[error(transparent)] Fetch(#[from] crate::remote::fetch::Error), #[error(transparent)] - RemoteConfiguration(#[from] crate::remote::init::Error), + RemoteInit(#[from] crate::remote::init::Error), + #[error("Custom configuration of remote to clone from failed")] + RemoteConfiguration(#[source] Box), #[error("Default remote configured at `clone.defaultRemoteName` is invalid")] RemoteName(#[from] crate::remote::name::Error), #[error("Failed to load repo-local git configuration before writing")] @@ -24,7 +26,7 @@ pub enum Error { #[error("The remote HEAD points to a reference named {head_ref_name:?} which is invalid.")] InvalidHeadRef { source: git_validate::refname::Error, - head_ref_name: crate::bstr::BString, + head_ref_name: BString, }, #[error("Failed to update HEAD with values from remote")] HeadUpdate(#[from] crate::reference::edit::Error), @@ -51,6 +53,7 @@ impl PrepareFetch { P: crate::Progress, P::SubProgress: 'static, { + use crate::remote; use crate::{bstr::ByteVec, remote::fetch::RefLogMessage}; let repo = self @@ -64,7 +67,7 @@ impl PrepareFetch { .config .resolved .string_by_key("clone.defaultRemoteName") - .map(|n| crate::remote::name::validated(n.into_owned())) + .map(|n| remote::name::validated(n.into_owned())) .unwrap_or_else(|| Ok("origin".into()))?, }; @@ -72,28 +75,39 @@ impl PrepareFetch { .remote_at(self.url.clone())? .with_refspecs( Some(format!("+refs/heads/*:refs/remotes/{remote_name}/*").as_str()), - crate::remote::Direction::Fetch, + remote::Direction::Fetch, ) .expect("valid static spec"); + let mut clone_fetch_tags = None; if let Some(f) = self.configure_remote.as_mut() { - remote = f(remote)?; + remote = f(remote).map_err(|err| Error::RemoteConfiguration(err))?; + } else { + clone_fetch_tags = remote::fetch::Tags::All.into(); } let config = util::write_remote_to_local_config_file(&mut remote, remote_name.clone())?; + // Now we are free to apply remote configuration we don't want to be written to disk. + if let Some(fetch_tags) = clone_fetch_tags { + remote = remote.with_fetch_tags(fetch_tags); + } + // Add HEAD after the remote was written to config, we need it to know what to checkout later, and assure // the ref that HEAD points to is present no matter what. - remote.fetch_specs.push( - git_refspec::parse( - format!("HEAD:refs/remotes/{remote_name}/HEAD").as_str().into(), - git_refspec::parse::Operation::Fetch, - ) - .expect("valid") - .to_owned(), - ); - let pending_pack: crate::remote::fetch::Prepare<'_, '_, _, _> = remote - .connect(crate::remote::Direction::Fetch, progress)? - .prepare_fetch(self.fetch_options.clone())?; + let head_refspec = git_refspec::parse( + format!("HEAD:refs/remotes/{remote_name}/HEAD").as_str().into(), + git_refspec::parse::Operation::Fetch, + ) + .expect("valid") + .to_owned(); + let pending_pack: remote::fetch::Prepare<'_, '_, _, _> = + remote.connect(remote::Direction::Fetch, progress)?.prepare_fetch({ + let mut opts = self.fetch_options.clone(); + if !opts.extra_refspecs.contains(&head_refspec) { + opts.extra_refspecs.push(head_refspec) + } + opts + })?; if pending_pack.ref_map().object_hash != repo.object_hash() { unimplemented!("configure repository to expect a different object hash as advertised by the server") } @@ -148,10 +162,15 @@ impl PrepareFetch { /// /// The passed in `remote` will be un-named and pre-configured to be a default remote as we know it from git-clone. /// It is not yet present in the configuration of the repository, - /// but each change it will eventually be written to the configuration prior to performing a the fetch operation. + /// but each change it will eventually be written to the configuration prior to performing a the fetch operation, + /// _all changes done in `f()` will be persisted_. + /// + /// It can also be used to configure additional options, like those for fetching tags. Note that + /// [with_fetch_tags()][crate::Remote::with_fetch_tags()] should be called here to configure the clone as desired. + /// Otherwise a clone is configured to be complete and fetches all tags, not only those reachable from all branches. pub fn configure_remote( mut self, - f: impl FnMut(crate::Remote<'_>) -> Result, crate::remote::init::Error> + 'static, + f: impl FnMut(crate::Remote<'_>) -> Result, Box> + 'static, ) -> Self { self.configure_remote = Some(Box::new(f)); self diff --git a/git-repository/src/clone/mod.rs b/git-repository/src/clone/mod.rs index b32495eba15..2bd6e7d480e 100644 --- a/git-repository/src/clone/mod.rs +++ b/git-repository/src/clone/mod.rs @@ -2,7 +2,8 @@ use std::convert::TryInto; use crate::bstr::BString; -type ConfigureRemoteFn = Box) -> Result, crate::remote::init::Error>>; +type ConfigureRemoteFn = + Box) -> Result, Box>>; /// A utility to collect configuration on how to fetch from a remote and initiate a fetch operation. It will delete the newly /// created repository on when dropped without successfully finishing a fetch. diff --git a/git-repository/src/remote/access.rs b/git-repository/src/remote/access.rs index c18f7ea4a84..665ec3d65e7 100644 --- a/git-repository/src/remote/access.rs +++ b/git-repository/src/remote/access.rs @@ -22,6 +22,11 @@ impl<'repo> Remote<'repo> { } } + /// Return how we handle tags when fetching the remote. + pub fn fetch_tags(&self) -> remote::fetch::Tags { + self.fetch_tags + } + /// Return the url used for the given `direction` with rewrites from `url..insteadOf|pushInsteadOf`, unless the instance /// was created with one of the `_without_url_rewrite()` methods. /// For pushing, this is the `remote..pushUrl` or the `remote..url` used for fetching, and for fetching it's diff --git a/git-repository/src/remote/build.rs b/git-repository/src/remote/build.rs index be10b8cf5eb..15ac8dcdfb9 100644 --- a/git-repository/src/remote/build.rs +++ b/git-repository/src/remote/build.rs @@ -23,6 +23,12 @@ impl Remote<'_> { self.push_url_inner(url, false) } + /// Configure how tags should be handled when fetching from the remote. + pub fn with_fetch_tags(mut self, tags: remote::fetch::Tags) -> Self { + self.fetch_tags = tags; + self + } + fn push_url_inner(mut self, push_url: Url, should_rewrite_urls: bool) -> Result where Url: TryInto, diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index c30bc1356f0..85f5f75b321 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -41,8 +41,13 @@ impl RefLogMessage { /// The status of the repository after the fetch operation #[derive(Debug, Clone)] pub enum Status { - /// Nothing changed as the remote didn't have anything new compared to our tracking branches. - NoChange, + /// Nothing changed as the remote didn't have anything new compared to our tracking branches, thus no pack was received + /// and no new object was added. + NoPackReceived { + /// However, depending on the refspecs, references might have been updated nonetheless to point to objects as + /// reported by the remote. + update_refs: refs::update::Outcome, + }, /// There was at least one tip with a new object which we received. Change { /// Information collected while writing the pack and its index. diff --git a/git-repository/src/remote/connection/fetch/negotiate.rs b/git-repository/src/remote/connection/fetch/negotiate.rs index e7132c2f8db..898400a2fcb 100644 --- a/git-repository/src/remote/connection/fetch/negotiate.rs +++ b/git-repository/src/remote/connection/fetch/negotiate.rs @@ -21,14 +21,27 @@ pub(crate) fn one_round( round: usize, repo: &crate::Repository, ref_map: &crate::remote::fetch::RefMap, + fetch_tags: crate::remote::fetch::Tags, arguments: &mut git_protocol::fetch::Arguments, _previous_response: Option<&git_protocol::fetch::Response>, ) -> Result { + let tag_refspec_to_ignore = fetch_tags + .to_refspec() + .filter(|_| matches!(fetch_tags, crate::remote::fetch::Tags::Included)); match algo { Algorithm::Naive => { assert_eq!(round, 1, "Naive always finishes after the first round, and claims."); let mut has_missing_tracking_branch = false; for mapping in &ref_map.mappings { + if tag_refspec_to_ignore.map_or(false, |tag_spec| { + mapping + .spec_index + .implicit_index() + .and_then(|idx| ref_map.extra_refspecs.get(idx)) + .map_or(false, |spec| spec.to_ref() == tag_spec) + }) { + continue; + } let have_id = mapping.local.as_ref().and_then(|name| { repo.find_reference(name) .ok() @@ -44,8 +57,8 @@ pub(crate) fn one_round( } } None => { - if let Some(have_id) = mapping.remote.as_id() { - arguments.want(have_id); + if let Some(want_id) = mapping.remote.as_id() { + arguments.want(want_id); has_missing_tracking_branch = true; } } diff --git a/git-repository/src/remote/connection/fetch/receive_pack.rs b/git-repository/src/remote/connection/fetch/receive_pack.rs index f13681db16a..09e44d0cd28 100644 --- a/git-repository/src/remote/connection/fetch/receive_pack.rs +++ b/git-repository/src/remote/connection/fetch/receive_pack.rs @@ -80,6 +80,12 @@ where git_protocol::fetch::Response::check_required_features(protocol_version, &fetch_features)?; let sideband_all = fetch_features.iter().any(|(n, _)| *n == "sideband-all"); let mut arguments = git_protocol::fetch::Arguments::new(protocol_version, fetch_features); + if matches!(con.remote.fetch_tags, crate::remote::fetch::Tags::Included) { + if !arguments.can_use_include_tag() { + unimplemented!("we expect servers to support 'include-tag', otherwise we have to implement another pass to fetch attached tags separately"); + } + arguments.use_include_tag(); + } let mut previous_response = None::; let mut round = 1; @@ -99,14 +105,27 @@ where round, repo, &self.ref_map, + con.remote.fetch_tags, &mut arguments, previous_response.as_ref(), ) { Ok(_) if arguments.is_empty() => { git_protocol::indicate_end_of_interaction(&mut con.transport).await.ok(); + let update_refs = refs::update( + repo, + self.reflog_message + .take() + .unwrap_or_else(|| RefLogMessage::Prefixed { action: "fetch".into() }), + &self.ref_map.mappings, + con.remote.refspecs(remote::Direction::Fetch), + &self.ref_map.extra_refspecs, + con.remote.fetch_tags, + self.dry_run, + self.write_packed_refs, + )?; return Ok(Outcome { ref_map: std::mem::take(&mut self.ref_map), - status: Status::NoChange, + status: Status::NoPackReceived { update_refs }, }); } Ok(is_done) => is_done, @@ -175,6 +194,8 @@ where .unwrap_or_else(|| RefLogMessage::Prefixed { action: "fetch".into() }), &self.ref_map.mappings, con.remote.refspecs(remote::Direction::Fetch), + &self.ref_map.extra_refspecs, + con.remote.fetch_tags, self.dry_run, self.write_packed_refs, )?; diff --git a/git-repository/src/remote/connection/fetch/update_refs/mod.rs b/git-repository/src/remote/connection/fetch/update_refs/mod.rs index 8e8eeb5d2ce..176d4c4181c 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -33,7 +33,7 @@ impl From for Update { } } -/// Update all refs as derived from `mappings` and produce an `Outcome` informing about all applied changes in detail, with each +/// Update all refs as derived from `refmap.mappings` and produce an `Outcome` informing about all applied changes in detail, with each /// [`update`][Update] corresponding to the [`fetch::Mapping`] of at the same index. /// If `dry_run` is true, ref transactions won't actually be applied, but are assumed to work without error so the underlying /// `repo` is not actually changed. Also it won't perform an 'object exists' check as these are likely not to exist as the pack @@ -41,30 +41,50 @@ impl From for Update { /// `action` is the prefix used for reflog entries, and is typically "fetch". /// /// It can be used to produce typical information that one is used to from `git fetch`. +#[allow(clippy::too_many_arguments)] pub(crate) fn update( repo: &Repository, message: RefLogMessage, mappings: &[fetch::Mapping], refspecs: &[git_refspec::RefSpec], + extra_refspecs: &[git_refspec::RefSpec], + fetch_tags: fetch::Tags, dry_run: fetch::DryRun, write_packed_refs: fetch::WritePackedRefs, ) -> Result { let mut edits = Vec::new(); let mut updates = Vec::new(); - for (remote, local, spec) in mappings.iter().filter_map( + let implicit_tag_refspec = fetch_tags + .to_refspec() + .filter(|_| matches!(fetch_tags, crate::remote::fetch::Tags::Included)); + for (remote, local, spec, is_implicit_tag) in mappings.iter().filter_map( |fetch::Mapping { remote, local, spec_index, - }| refspecs.get(*spec_index).map(|spec| (remote, local, spec)), + }| { + spec_index.get(refspecs, extra_refspecs).map(|spec| { + ( + remote, + local, + spec, + implicit_tag_refspec.map_or(false, |tag_spec| spec.to_ref() == tag_spec), + ) + }) + }, ) { let remote_id = match remote.as_id() { Some(id) => id, None => continue, }; if dry_run == fetch::DryRun::No && !repo.objects.contains(remote_id) { - updates.push(update::Mode::RejectedSourceObjectNotFound { id: remote_id.into() }.into()); + let update = if is_implicit_tag { + update::Mode::ImplicitTagNotSentByRemote.into() + } else { + update::Mode::RejectedSourceObjectNotFound { id: remote_id.into() }.into() + }; + updates.push(update); continue; } let checked_out_branches = worktree_branches(repo)?; diff --git a/git-repository/src/remote/connection/fetch/update_refs/tests.rs b/git-repository/src/remote/connection/fetch/update_refs/tests.rs index a148b0cdc8b..bc32397ef17 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/tests.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/tests.rs @@ -33,6 +33,7 @@ mod update { } use git_ref::{transaction::Change, TargetRef}; + use crate::remote::fetch::SpecIndex; use crate::{ bstr::BString, remote::{ @@ -128,6 +129,8 @@ mod update { prefixed("action"), &mapping, &specs, + &[], + fetch::Tags::None, reflog_message.map(|_| fetch::DryRun::Yes).unwrap_or(fetch::DryRun::No), fetch::WritePackedRefs::Never, ) @@ -190,6 +193,8 @@ mod update { prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, )?; @@ -219,6 +224,8 @@ mod update { prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -246,13 +253,15 @@ mod update { object: hex_to_id("f99771fe6a1b535783af3163eba95a927aae21d5"), }), local: Some("refs/heads/symbolic".into()), - spec_index: 0, + spec_index: SpecIndex::ExplicitInRemote(0), }); let out = fetch::refs::update( &repo, prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -294,6 +303,8 @@ mod update { prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -318,6 +329,8 @@ mod update { prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -359,13 +372,15 @@ mod update { object: hex_to_id("f99771fe6a1b535783af3163eba95a927aae21d5"), }), local: Some("refs/remotes/origin/main".into()), - spec_index: 0, + spec_index: SpecIndex::ExplicitInRemote(0), }); let out = fetch::refs::update( &repo, prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -412,6 +427,8 @@ mod update { }, &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::Yes, fetch::WritePackedRefs::Never, ) @@ -444,6 +461,8 @@ mod update { prefixed("action"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::No, fetch::WritePackedRefs::Never, ) @@ -464,6 +483,8 @@ mod update { prefixed("prefix"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::No, fetch::WritePackedRefs::Never, ) @@ -495,6 +516,8 @@ mod update { prefixed("prefix"), &mappings, &specs, + &[], + fetch::Tags::None, fetch::DryRun::No, fetch::WritePackedRefs::Never, ) @@ -536,7 +559,7 @@ mod update { _ => unreachable!("not a ref, must be id: {:?}", m), }), local: m.rhs.map(|r| r.into_owned()), - spec_index: m.spec_index, + spec_index: SpecIndex::ExplicitInRemote(m.spec_index), }) .collect(); (mappings, vec![spec.to_owned()]) diff --git a/git-repository/src/remote/connection/fetch/update_refs/update.rs b/git-repository/src/remote/connection/fetch/update_refs/update.rs index ddebc733b80..6e0cb436883 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/update.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/update.rs @@ -47,6 +47,11 @@ pub enum Mode { Forced, /// A new ref has been created as there was none before. New, + /// The reference belongs to a tag that was listed by the server but whose target didn't get sent as it doesn't point + /// to the commit-graph we were fetching explicitly. + /// + /// This is kind of update is only happening if `remote..tagOpt` is not set explicitly to either `--tags` or `--no-tags`. + ImplicitTagNotSentByRemote, /// The object id to set the target reference to could not be found. RejectedSourceObjectNotFound { /// The id of the object that didn't exist in the object database, even though it should since it should be part of the pack. @@ -75,6 +80,7 @@ impl std::fmt::Display for Mode { Mode::FastForward => "fast-forward", Mode::Forced => "forced-update", Mode::New => "new", + Mode::ImplicitTagNotSentByRemote => "unrelated tag on remote", Mode::RejectedSourceObjectNotFound { id } => return write!(f, "rejected ({} not found)", id), Mode::RejectedTagUpdate => "rejected (would overwrite existing tag)", Mode::RejectedNonFastForward => "rejected (non-fast-forward)", @@ -101,6 +107,7 @@ impl Outcome { &self, mappings: &'a [fetch::Mapping], refspecs: &'b [git_refspec::RefSpec], + extra_refspecs: &'b [git_refspec::RefSpec], ) -> impl Iterator< Item = ( &super::Update, @@ -113,7 +120,7 @@ impl Outcome { ( update, mapping, - refspecs.get(mapping.spec_index), + mapping.spec_index.get(refspecs, extra_refspecs), update.edit_index.and_then(|idx| self.edits.get(idx)), ) }) diff --git a/git-repository/src/remote/connection/ref_map.rs b/git-repository/src/remote/connection/ref_map.rs index 1e8782df3ce..219f3040faa 100644 --- a/git-repository/src/remote/connection/ref_map.rs +++ b/git-repository/src/remote/connection/ref_map.rs @@ -3,6 +3,7 @@ use std::collections::HashSet; use git_features::progress::Progress; use git_protocol::transport::client::Transport; +use crate::remote::fetch::SpecIndex; use crate::{ bstr, bstr::{BString, ByteVec}, @@ -55,13 +56,18 @@ pub struct Options { /// /// This is useful in case of custom servers. pub handshake_parameters: Vec<(String, Option)>, + /// A list of refspecs to use as implicit refspecs which won't be saved or otherwise be part of the remote in question. + /// + /// This is useful for handling `remote..tagOpt` for example. + pub extra_refspecs: Vec, } impl Default for Options { fn default() -> Self { Options { prefix_from_spec_as_filter_on_remote: true, - handshake_parameters: Default::default(), + handshake_parameters: Vec::new(), + extra_refspecs: Vec::new(), } } } @@ -104,13 +110,26 @@ where Options { prefix_from_spec_as_filter_on_remote, handshake_parameters, + mut extra_refspecs, }: Options, ) -> Result { let null = git_hash::ObjectId::null(git_hash::Kind::Sha1); // OK to hardcode Sha1, it's not supposed to match, ever. + + if let Some(tag_spec) = self.remote.fetch_tags.to_refspec().map(|spec| spec.to_owned()) { + if !extra_refspecs.contains(&tag_spec) { + extra_refspecs.push(tag_spec); + } + }; + let specs = { + let mut s = self.remote.fetch_specs.clone(); + s.extend(extra_refspecs.clone()); + s + }; let remote = self - .fetch_refs(prefix_from_spec_as_filter_on_remote, handshake_parameters) + .fetch_refs(prefix_from_spec_as_filter_on_remote, handshake_parameters, &specs) .await?; - let group = git_refspec::MatchGroup::from_fetch_specs(self.remote.fetch_specs.iter().map(|s| s.to_ref())); + let num_explicit_specs = self.remote.fetch_specs.len(); + let group = git_refspec::MatchGroup::from_fetch_specs(specs.iter().map(|s| s.to_ref())); let (res, fixes) = group .match_remotes(remote.refs.iter().map(|r| { let (full_ref_name, target, object) = r.unpack(); @@ -135,13 +154,18 @@ where }) }), local: m.rhs.map(|c| c.into_owned()), - spec_index: m.spec_index, + spec_index: if m.spec_index < num_explicit_specs { + SpecIndex::ExplicitInRemote(m.spec_index) + } else { + SpecIndex::Implicit(m.spec_index - num_explicit_specs) + }, }) .collect(); let object_hash = extract_object_format(self.remote.repo, &remote.outcome)?; Ok(fetch::RefMap { mappings, + extra_refspecs, fixes, remote_refs: remote.refs, handshake: remote.outcome, @@ -155,6 +179,7 @@ where &mut self, filter_by_prefix: bool, extra_parameters: Vec<(String, Option)>, + refspecs: &[git_refspec::RefSpec], ) -> Result { let mut credentials_storage; let url = self.transport.to_url(); @@ -190,7 +215,6 @@ where let refs = match outcome.refs.take() { Some(refs) => refs, None => { - let specs = &self.remote.fetch_specs; let agent_feature = self.remote.repo.config.user_agent_tuple(); git_protocol::ls_refs( &mut self.transport, @@ -199,7 +223,7 @@ where features.push(agent_feature); if filter_by_prefix { let mut seen = HashSet::new(); - for spec in specs { + for spec in refspecs { let spec = spec.to_ref(); if seen.insert(spec.instruction()) { let mut prefixes = Vec::with_capacity(1); diff --git a/git-repository/src/remote/errors.rs b/git-repository/src/remote/errors.rs index b73e12d0300..a27e4d2b5f7 100644 --- a/git-repository/src/remote/errors.rs +++ b/git-repository/src/remote/errors.rs @@ -6,6 +6,10 @@ pub mod find { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { + #[error( + "The value for 'remote..tagOpt` is invalid and must either be '--tags' or '--no-tags': \"{value}\"" + )] + TagOpt { value: BString }, #[error("{spec:?} {kind} ref-spec failed to parse")] RefSpec { spec: BString, diff --git a/git-repository/src/remote/fetch.rs b/git-repository/src/remote/fetch.rs index d82a70b82e7..1a2f95da78b 100644 --- a/git-repository/src/remote/fetch.rs +++ b/git-repository/src/remote/fetch.rs @@ -1,5 +1,3 @@ -use crate::bstr::{BStr, BString}; - /// If `Yes`, don't really make changes but do as much as possible to get an idea of what would be done. #[derive(Debug, Copy, Clone, PartialEq, Eq)] #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] @@ -20,11 +18,51 @@ pub(crate) enum WritePackedRefs { Only, } +/// Describe how to handle tags when fetching +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Tags { + /// Fetch all tags from the remote, even if these are not reachable from objects referred to by our refspecs. + All, + /// Fetch only the tags that point to the objects being sent. + /// That way, annotated tags that point to an object we receive are automatically transmitted and their refs are created. + /// The same goes for lightweight tags. + Included, + /// Do not fetch any tags. + None, +} + +impl Default for Tags { + fn default() -> Self { + Tags::Included + } +} + +impl Tags { + /// Obtain a refspec that determines whether or not to fetch all tags, depending on this variant. + /// + /// The returned refspec is the default refspec for tags, but won't overwrite local tags ever. + pub fn to_refspec(&self) -> Option> { + match self { + Tags::All | Tags::Included => Some( + git_refspec::parse("refs/tags/*:refs/tags/*".into(), git_refspec::parse::Operation::Fetch) + .expect("valid"), + ), + Tags::None => None, + } + } +} + /// Information about the relationship between our refspecs, and remote references with their local counterparts. #[derive(Default, Debug, Clone)] +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] pub struct RefMap { /// A mapping between a remote reference and a local tracking branch. pub mappings: Vec, + /// Refspecs which have been added implicitly due to settings of the `remote`, possibly pre-initialized from + /// [`extra_refspecs` in RefMap options][crate::remote::ref_map::Options::extra_refspecs]. + /// + /// They are never persisted nor are they typically presented to the user. + pub extra_refspecs: Vec, /// Information about the fixes applied to the `mapping` due to validation and sanitization. pub fixes: Vec, /// All refs advertised by the remote. @@ -41,6 +79,7 @@ pub struct RefMap { /// Either an object id that the remote has or the matched remote ref itself. #[derive(Debug, Clone)] +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] pub enum Source { /// An object id, as the matched ref-spec was an object id itself. ObjectId(git_hash::ObjectId), @@ -48,6 +87,7 @@ pub enum Source { Ref(git_protocol::handshake::Ref), } +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] impl Source { /// Return either the direct object id we refer to or the direct target that a reference refers to. /// The latter may be a direct or a symbolic reference, and we degenerate this to the peeled object id. @@ -60,7 +100,7 @@ impl Source { } /// Return ourselves as the full name of the reference we represent, or `None` if this source isn't a reference but an object. - pub fn as_name(&self) -> Option<&BStr> { + pub fn as_name(&self) -> Option<&crate::bstr::BStr> { match self { Source::ObjectId(_) => None, Source::Ref(r) => match r { @@ -73,15 +113,51 @@ impl Source { } } +/// An index into various lists of refspecs that have been used in a [Mapping] of remote references to local ones. +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub enum SpecIndex { + /// An index into the _refspecs of the remote_ that triggered a fetch operation. + /// These refspecs are explicit and visible to the user. + ExplicitInRemote(usize), + /// An index into the list of [extra refspecs][crate::remote::fetch::RefMap::extra_refspecs] that are implicit + /// to a particular fetch operation. + Implicit(usize), +} + +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] +impl SpecIndex { + /// Depending on our index variant, get the index either from `refspecs` or from `extra_refspecs` for `Implicit` variants. + pub fn get<'a>( + self, + refspecs: &'a [git_refspec::RefSpec], + extra_refspecs: &'a [git_refspec::RefSpec], + ) -> Option<&'a git_refspec::RefSpec> { + match self { + SpecIndex::ExplicitInRemote(idx) => refspecs.get(idx), + SpecIndex::Implicit(idx) => extra_refspecs.get(idx), + } + } + + /// If this is an `Implicit` variant, return its index. + pub fn implicit_index(self) -> Option { + match self { + SpecIndex::Implicit(idx) => Some(idx), + SpecIndex::ExplicitInRemote(_) => None, + } + } +} + /// A mapping between a single remote reference and its advertised objects to a local destination which may or may not exist. #[derive(Debug, Clone)] +#[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] pub struct Mapping { /// The reference on the remote side, along with information about the objects they point to as advertised by the server. pub remote: Source, /// The local tracking reference to update after fetching the object visible via `remote`. - pub local: Option, + pub local: Option, /// The index into the fetch ref-specs used to produce the mapping, allowing it to be recovered. - pub spec_index: usize, + pub spec_index: SpecIndex, } #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] diff --git a/git-repository/src/remote/init.rs b/git-repository/src/remote/init.rs index eef0a950e55..c997e91283d 100644 --- a/git-repository/src/remote/init.rs +++ b/git-repository/src/remote/init.rs @@ -27,6 +27,7 @@ use crate::bstr::BString; /// Initialization impl<'repo> Remote<'repo> { + #[allow(clippy::too_many_arguments)] pub(crate) fn from_preparsed_config( name_or_url: Option, url: Option, @@ -34,6 +35,7 @@ impl<'repo> Remote<'repo> { fetch_specs: Vec, push_specs: Vec, should_rewrite_urls: bool, + fetch_tags: remote::fetch::Tags, repo: &'repo Repository, ) -> Result { debug_assert!( @@ -51,6 +53,7 @@ impl<'repo> Remote<'repo> { push_url_alias, fetch_specs, push_specs, + fetch_tags, repo, }) } @@ -76,6 +79,7 @@ impl<'repo> Remote<'repo> { push_url_alias: None, fetch_specs: Vec::new(), push_specs: Vec::new(), + fetch_tags: Default::default(), repo, }) } diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index 4206e962526..d3c9be4e3ef 100644 --- a/git-repository/src/remote/mod.rs +++ b/git-repository/src/remote/mod.rs @@ -43,7 +43,6 @@ pub use errors::find; pub mod init; /// -#[cfg(any(feature = "async-network-client", feature = "blocking-network-client"))] pub mod fetch; /// diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index d2755c4bfd1..895bc4a4d87 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -1,6 +1,7 @@ use std::convert::TryInto; -use crate::{bstr::BString, Remote}; +use crate::bstr::BStr; +use crate::{bstr::BString, remote, Remote}; /// The error returned by [`Remote::save_to()`]. #[derive(Debug, thiserror::Error)] @@ -30,6 +31,9 @@ impl Remote<'_> { /// and the last `remote ""` section will be containing all relevant values so that reloading the remote /// from `config` would yield the same in-memory state. pub fn save_to(&self, config: &mut git_config::File<'static>) -> Result<(), Error> { + fn as_key(name: &str) -> git_config::parse::section::Key<'_> { + name.try_into().expect("valid") + } let name = self.name().ok_or_else(|| Error::NameMissing { url: self .url @@ -43,7 +47,7 @@ impl Remote<'_> { .collect::>() }) { let mut sections_to_remove = Vec::new(); - const KEYS_TO_REMOVE: &[&str] = &["url", "pushurl", "fetch", "push"]; + const KEYS_TO_REMOVE: &[&str] = &["url", "pushurl", "fetch", "push", "tagOpt"]; for id in section_ids { let mut section = config.section_mut_by_id(id).expect("just queried"); let was_empty = section.num_values() == 0; @@ -65,10 +69,21 @@ impl Remote<'_> { .section_mut_or_create_new("remote", Some(name.as_ref())) .expect("section name is validated and 'remote' is acceptable"); if let Some(url) = self.url.as_ref() { - section.push("url".try_into().expect("valid"), Some(url.to_bstring().as_ref())) + section.push(as_key("url"), Some(url.to_bstring().as_ref())) } if let Some(url) = self.push_url.as_ref() { - section.push("pushurl".try_into().expect("valid"), Some(url.to_bstring().as_ref())) + section.push(as_key("pushurl"), Some(url.to_bstring().as_ref())) + } + if self.fetch_tags != Default::default() { + section.push( + as_key("tagOpt"), + BStr::new(match self.fetch_tags { + remote::fetch::Tags::All => "--tags", + remote::fetch::Tags::None => "--no-tags", + remote::fetch::Tags::Included => unreachable!("BUG: the default shouldn't be written and we try"), + }) + .into(), + ) } for (key, spec) in self .fetch_specs @@ -76,10 +91,7 @@ impl Remote<'_> { .map(|spec| ("fetch", spec)) .chain(self.push_specs.iter().map(|spec| ("push", spec))) { - section.push( - key.try_into().expect("valid"), - Some(spec.to_ref().to_bstring().as_ref()), - ) + section.push(as_key(key), Some(spec.to_ref().to_bstring().as_ref())) } Ok(()) } diff --git a/git-repository/src/repository/remote.rs b/git-repository/src/repository/remote.rs index 315cc180be7..a6722c1930c 100644 --- a/git-repository/src/repository/remote.rs +++ b/git-repository/src/repository/remote.rs @@ -4,6 +4,9 @@ use crate::{bstr::BStr, remote, remote::find, Remote}; impl crate::Repository { /// Create a new remote available at the given `url`. + /// + /// It's configured to fetch included tags by default, similar to git. + /// See [`with_fetch_tags(…)`][Remote::with_fetch_tags()] for a way to change it. pub fn remote_at(&self, url: Url) -> Result, remote::init::Error> where Url: TryInto, @@ -12,7 +15,8 @@ impl crate::Repository { Remote::from_fetch_url(url, true, self) } - /// Create a new remote available at the given `url`, but don't rewrite the url according to rewrite rules. + /// Create a new remote available at the given `url` similarly to [`remote_at()`][crate::Repository::remote_at()], + /// but don't rewrite the url according to rewrite rules. /// This eliminates a failure mode in case the rewritten URL is faulty, allowing to selectively [apply rewrite /// rules][Remote::rewrite_urls()] later and do so non-destructively. pub fn remote_at_without_url_rewrite(&self, url: Url) -> Result, remote::init::Error> @@ -91,14 +95,14 @@ impl crate::Repository { }; let url = config_url("url", "fetch"); let push_url = config_url("pushUrl", "push"); + let config = &self.config.resolved; let mut config_spec = |op: git_refspec::parse::Operation| { let kind = match op { git_refspec::parse::Operation::Fetch => "fetch", git_refspec::parse::Operation::Push => "push", }; - self.config - .resolved + config .strings_filter("remote", Some(name_or_url), kind, &mut filter) .map(|specs| { specs @@ -122,6 +126,20 @@ impl crate::Repository { }; let fetch_specs = config_spec(git_refspec::parse::Operation::Fetch); let push_specs = config_spec(git_refspec::parse::Operation::Push); + let fetch_tags = config + .string_filter("remote", Some(name_or_url), "tagOpt", &mut filter) + .map(|tag| { + Ok(match tag.as_ref().as_ref() { + b"--tags" => remote::fetch::Tags::All, + b"--no-tags" => remote::fetch::Tags::None, + unknown => return Err(find::Error::TagOpt { value: unknown.into() }), + }) + }); + let fetch_tags = match fetch_tags { + Some(Ok(v)) => v, + Some(Err(err)) => return Some(Err(err)), + None => Default::default(), + }; match (url, fetch_specs, push_url, push_specs) { (None, None, None, None) => None, @@ -156,6 +174,7 @@ impl crate::Repository { fetch_specs, push_specs, rewrite_urls, + fetch_tags, self, ) .map_err(Into::into), diff --git a/git-repository/src/types.rs b/git-repository/src/types.rs index 16b658659ce..b684a5ec251 100644 --- a/git-repository/src/types.rs +++ b/git-repository/src/types.rs @@ -195,6 +195,8 @@ pub struct Remote<'repo> { pub(crate) fetch_specs: Vec, /// Refspecs for use when pushing. pub(crate) push_specs: Vec, + /// Tell us what to do with tags when fetched. + pub(crate) fetch_tags: remote::fetch::Tags, // /// Delete local tracking branches that don't exist on the remote anymore. // pub(crate) prune: bool, // /// Delete tags that don't exist on the remote anymore, equivalent to pruning the refspec `refs/tags/*:refs/tags/*`. diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 0d286d194e1..fe0cabc4042 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -7,6 +7,7 @@ mod blocking_io { use git_object::bstr::ByteSlice; use git_ref::TargetRef; use git_repository as git; + use git_repository::remote::fetch::SpecIndex; use crate::remote; @@ -15,6 +16,7 @@ mod blocking_io { let tmp = git_testtools::tempfile::TempDir::new()?; let called_configure_remote = std::sync::Arc::new(std::sync::atomic::AtomicBool::default()); let remote_name = "special"; + let desired_fetch_tags = git::remote::fetch::Tags::Included; let mut prepare = git::clone::PrepareFetch::new( remote::repo("base").path(), tmp.path(), @@ -30,10 +32,10 @@ mod blocking_io { let called_configure_remote = called_configure_remote.clone(); move |r| { called_configure_remote.store(true, std::sync::atomic::Ordering::Relaxed); - Ok( - r.with_refspecs(Some("+refs/tags/*:refs/tags/*"), git::remote::Direction::Fetch) - .expect("valid static spec"), - ) + let r = r + .with_refspecs(Some("+refs/tags/b-tag:refs/tags/b-tag"), git::remote::Direction::Fetch)? + .with_fetch_tags(desired_fetch_tags); + Ok(r) } }); let (repo, out) = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; @@ -45,10 +47,15 @@ mod blocking_io { ); assert_eq!(repo.remote_names().len(), 1, "only ever one remote"); let remote = repo.find_remote(remote_name)?; + let num_refspecs = remote.refspecs(git::remote::Direction::Fetch).len(); assert_eq!( - remote.refspecs(git::remote::Direction::Fetch).len(), - 2, - "our added spec was stored as well" + num_refspecs, 2, + "our added spec was stored as well, but no implied specs due to the `Tags::All` setting" + ); + assert_eq!( + remote.fetch_tags(), + desired_fetch_tags, + "fetch-tags are persisted via the 'tagOpt` key" ); assert!( git::path::from_bstr( @@ -62,21 +69,39 @@ mod blocking_io { "file urls can't be relative paths" ); - assert_eq!(out.ref_map.mappings.len(), 14); + let (explicit_max_idx, implicit_max_index) = + out.ref_map + .mappings + .iter() + .map(|m| m.spec_index) + .fold((0, 0), |(a, b), i| match i { + SpecIndex::ExplicitInRemote(idx) => (idx.max(a), b), + SpecIndex::Implicit(idx) => (a, idx.max(b)), + }); + assert_eq!( + explicit_max_idx, + num_refspecs - 1, + "mappings don't refer to non-existing explicit refspecs" + ); + assert_eq!( + implicit_max_index, + &out.ref_map.extra_refspecs.len() - 1, + "mappings don't refer to non-existing implicit refspecs" + ); let packed_refs = repo .refs .cached_packed_buffer()? .expect("packed refs should be present"); - assert_eq!( - packed_refs.iter()?.count(), - 14, - "all non-symbolic refs should be stored" - ); assert_eq!( repo.refs.loose_iter()?.count(), 2, "HEAD and an actual symbolic ref we received" ); + assert_eq!( + packed_refs.iter()?.count(), + 14, + "all non-symbolic refs should be stored, if reachable from our refs" + ); match out.status { git_repository::remote::fetch::Status::Change { update_refs, .. } => { @@ -107,6 +132,38 @@ mod blocking_io { assert_reflog(logs.all()); } } + let mut out_of_graph_tags = Vec::new(); + for mapping in update_refs + .updates + .iter() + .enumerate() + .filter_map(|(idx, update)| { + matches!( + update.mode, + git::remote::fetch::refs::update::Mode::ImplicitTagNotSentByRemote + ) + .then(|| idx) + }) + .map(|idx| &out.ref_map.mappings[idx]) + { + out_of_graph_tags.push( + mapping + .remote + .as_name() + .expect("tag always has a path") + .to_str() + .expect("valid UTF8"), + ); + } + assert_eq!( + out_of_graph_tags, + &[ + "refs/tags/annotated-detached-tag", + "refs/tags/annotated-future-tag", + "refs/tags/detached-tag", + "refs/tags/future-tag" + ] + ); } _ => unreachable!("clones are always causing changes and dry-runs aren't possible"), } diff --git a/git-repository/tests/fixtures/make_remote_repos.sh b/git-repository/tests/fixtures/make_remote_repos.sh index e3d22bd544a..e32f45e1a28 100644 --- a/git-repository/tests/fixtures/make_remote_repos.sh +++ b/git-repository/tests/fixtures/make_remote_repos.sh @@ -90,6 +90,21 @@ git init base { echo g && echo h && echo i && echo j && echo d && echo e && echo f && echo b && echo c && echo a; } > file git add file && git commit -m A git branch a + + git checkout --orphan detached + git rm -rf . + git commit --allow-empty -am "detached" + git tag detached-tag + git tag -m on-detached-graph annotated-detached-tag + + git checkout -b tmp main + git branch -D detached + git commit --allow-empty -m "something new" + git tag future-tag + git tag -m on-future-graph annotated-future-tag + + git checkout main + git branch -D tmp ) git clone --shared base clone @@ -97,6 +112,12 @@ git clone --shared base clone git remote add myself . ) +git clone --no-tags --shared base clone-no-tags +(cd clone-no-tags + git remote add --no-tags myself-no-tags . + git remote add --tags myself-with-tags . +) + git clone --shared base push-default (cd push-default diff --git a/git-repository/tests/remote/fetch.rs b/git-repository/tests/remote/fetch.rs index 8ef0acc3bd5..04e4675182a 100644 --- a/git-repository/tests/remote/fetch.rs +++ b/git-repository/tests/remote/fetch.rs @@ -43,33 +43,42 @@ mod blocking_and_async_io { async(feature = "async-network-client-async-std", async_std::test) )] async fn fetch_empty_pack() -> crate::Result { - let (repo, _tmp) = repo_rw("two-origins"); let daemon = spawn_git_daemon_if_async(repo_path("base"))?; - let mut remote = into_daemon_remote_if_async( - repo.head()?.into_remote(Fetch).expect("present")?, - daemon.as_ref(), - None, - ); - remote.replace_refspecs(Some("HEAD:refs/remotes/origin/does-not-yet-exist"), Fetch)?; + for (fetch_tags, expected_ref_count) in [ + (git::remote::fetch::Tags::None, 1), + (git::remote::fetch::Tags::Included, 7), + (git::remote::fetch::Tags::All, 7), + ] { + let (repo, _tmp) = repo_rw("two-origins"); + let mut remote = into_daemon_remote_if_async( + repo.head()? + .into_remote(Fetch) + .expect("present")? + .with_fetch_tags(fetch_tags), + daemon.as_ref(), + None, + ); + remote.replace_refspecs(Some("HEAD:refs/remotes/origin/does-not-yet-exist"), Fetch)?; - let res = remote - .connect(Fetch, git::progress::Discard) - .await? - .prepare_fetch(Default::default()) - .await? - .receive(&AtomicBool::default()) - .await?; + let res = remote + .connect(Fetch, git::progress::Discard) + .await? + .prepare_fetch(Default::default()) + .await? + .receive(&AtomicBool::default()) + .await?; - match res.status { - git::remote::fetch::Status::Change {write_pack_bundle, update_refs} => { - assert_eq!(write_pack_bundle.index.data_hash, hex_to_id("029d08823bd8a8eab510ad6ac75c823cfd3ed31e")); - assert_eq!(write_pack_bundle.index.num_objects, 0, "empty pack"); - assert!(write_pack_bundle.data_path.as_deref().map_or(false, |p| p.is_file())); - assert!(write_pack_bundle.index_path.as_deref().map_or(false, |p| p.is_file())); - assert_eq!(update_refs.edits.len(), 1); - assert!(!write_pack_bundle.keep_path.as_deref().map_or(false, |p| p.is_file()), ".keep files are deleted if at least one ref-edit was made or the pack is empty"); - }, - _ => unreachable!("Naive negotiation sends the same have and wants, resulting in an empty pack (technically no change, but we don't detect it) - empty packs are fine") + match res.status { + git::remote::fetch::Status::Change { write_pack_bundle, update_refs } => { + assert_eq!(write_pack_bundle.index.data_hash, hex_to_id("029d08823bd8a8eab510ad6ac75c823cfd3ed31e")); + assert_eq!(write_pack_bundle.index.num_objects, 0, "empty pack"); + assert!(write_pack_bundle.data_path.as_deref().map_or(false, |p| p.is_file())); + assert!(write_pack_bundle.index_path.as_deref().map_or(false, |p| p.is_file())); + assert_eq!(update_refs.edits.len(), expected_ref_count); + assert!(!write_pack_bundle.keep_path.as_deref().map_or(false, |p| p.is_file()), ".keep files are deleted if at least one ref-edit was made or the pack is empty"); + }, + _ => unreachable!("Naive negotiation sends the same have and wants, resulting in an empty pack (technically no change, but we don't detect it) - empty packs are fine") + } } Ok(()) } @@ -79,30 +88,55 @@ mod blocking_and_async_io { async(feature = "async-network-client-async-std", async_std::test) )] async fn fetch_pack_without_local_destination() -> crate::Result { - let (repo, _tmp) = repo_rw("two-origins"); let daemon = spawn_git_daemon_if_async(repo_path("clone-as-base-with-changes"))?; - let mut remote = - into_daemon_remote_if_async(repo.find_remote("changes-on-top-of-origin")?, daemon.as_ref(), None); - remote.replace_refspecs(Some("HEAD"), Fetch)?; + for (fetch_tags, expected_data_hash, num_objects_offset, expected_ref_edits) in [ + ( + git::remote::fetch::Tags::None, + "de303ef102bd5705a40a0c42ae2972eb1a668455", + 0, + 0, + ), + ( + git::remote::fetch::Tags::Included, + "edc8cc8a25e64e73aacea469fc765564dd2c3f65", + 1, + 7, + ), + ( + git::remote::fetch::Tags::All, + "edc8cc8a25e64e73aacea469fc765564dd2c3f65", + 1, + 7, + ), + ] { + let (repo, _tmp) = repo_rw("two-origins"); + let mut remote = into_daemon_remote_if_async( + repo.find_remote("changes-on-top-of-origin")? + .with_fetch_tags(fetch_tags), + daemon.as_ref(), + None, + ); + remote.replace_refspecs(Some("HEAD"), Fetch)?; - let res: git::remote::fetch::Outcome = remote - .connect(Fetch, git::progress::Discard) - .await? - .prepare_fetch(Default::default()) - .await? - .receive(&AtomicBool::default()) - .await?; + let res: git::remote::fetch::Outcome = remote + .connect(Fetch, git::progress::Discard) + .await? + .prepare_fetch(Default::default()) + .await? + .receive(&AtomicBool::default()) + .await?; - match res.status { - git::remote::fetch::Status::Change {write_pack_bundle, update_refs} => { - assert_eq!(write_pack_bundle.index.data_hash, hex_to_id("edc8cc8a25e64e73aacea469fc765564dd2c3f65")); - assert_eq!(write_pack_bundle.index.num_objects, 4); - assert!(write_pack_bundle.data_path.as_deref().map_or(false, |p| p.is_file())); - assert!(write_pack_bundle.index_path.as_deref().map_or(false, |p| p.is_file())); - assert_eq!(update_refs.edits.len(), 0); - assert!(write_pack_bundle.keep_path.as_deref().map_or(false, |p| p.is_file()), ".keep are kept if there was no edit to bind the packs objects to our commit graph"); - }, - _ => unreachable!("Naive negotiation sends the same have and wants, resulting in an empty pack (technically no change, but we don't detect it) - empty packs are fine") + match res.status { + git::remote::fetch::Status::Change { write_pack_bundle, update_refs } => { + assert_eq!(write_pack_bundle.index.data_hash, hex_to_id(expected_data_hash), ); + assert_eq!(write_pack_bundle.index.num_objects, 3 + num_objects_offset, "{fetch_tags:?}"); + assert!(write_pack_bundle.data_path.as_deref().map_or(false, |p| p.is_file())); + assert!(write_pack_bundle.index_path.as_deref().map_or(false, |p| p.is_file())); + assert_eq!(update_refs.edits.len(), expected_ref_edits, "{fetch_tags:?}"); + assert_eq!(write_pack_bundle.keep_path.as_deref().map_or(false, |p| p.is_file()), update_refs.edits.is_empty(),".keep are kept if there was no edit to prevent `git gc` from clearing out the pack as it's not referred to necessarily"); + }, + _ => unreachable!("Naive negotiation sends the same have and wants, resulting in an empty pack (technically no change, but we don't detect it) - empty packs are fine") + } } Ok(()) } @@ -117,18 +151,10 @@ mod blocking_and_async_io { p.pop(); p })?; - for (version, expected_objects, expected_hash) in [ - (None, 4, "d07c527cf14e524a8494ce6d5d08e28079f5c6ea"), - ( - Some(git::protocol::transport::Protocol::V2), - 4, - "d07c527cf14e524a8494ce6d5d08e28079f5c6ea", - ), - ( - Some(git::protocol::transport::Protocol::V1), - 4, - "d07c527cf14e524a8494ce6d5d08e28079f5c6ea", // TODO: if these are the same, remove them - ), + for version in [ + None, + Some(git::protocol::transport::Protocol::V2), + Some(git::protocol::transport::Protocol::V1), ] { let (mut repo, _tmp) = repo_rw("two-origins"); if let Some(version) = version { @@ -158,13 +184,17 @@ mod blocking_and_async_io { .await? .receive(&AtomicBool::default()) .await?; - assert!(matches!(outcome.status, git::remote::fetch::Status::NoChange)); + assert!(matches!( + outcome.status, + git::remote::fetch::Status::NoPackReceived { .. } + )); } // Some updates to be fetched for dry_run in [true, false] { let remote = into_daemon_remote_if_async( - repo.find_remote("changes-on-top-of-origin")?, + repo.find_remote("changes-on-top-of-origin")? + .with_fetch_tags(git::remote::fetch::Tags::None), daemon.as_ref(), "clone-as-base-with-changes", ); @@ -183,12 +213,15 @@ mod blocking_and_async_io { } => { assert_eq!(write_pack_bundle.pack_version, git::odb::pack::data::Version::V2); assert_eq!(write_pack_bundle.object_hash, repo.object_hash()); - assert_eq!(write_pack_bundle.index.num_objects, expected_objects, "{dry_run}: this value is 4 when git does it with 'consecutive' negotiation style, but could be 33 if completely naive."); + assert_eq!(write_pack_bundle.index.num_objects, 4, "{dry_run}: this value is 4 when git does it with 'consecutive' negotiation style, but could be 33 if completely naive."); assert_eq!( write_pack_bundle.index.index_version, git::odb::pack::index::Version::V2 ); - assert_eq!(write_pack_bundle.index.index_hash, hex_to_id(expected_hash)); + assert_eq!( + write_pack_bundle.index.index_hash, + hex_to_id("d07c527cf14e524a8494ce6d5d08e28079f5c6ea") + ); assert!(write_pack_bundle.data_path.map_or(false, |f| f.is_file())); assert!(write_pack_bundle.index_path.map_or(false, |f| f.is_file())); assert_eq!(update_refs.edits.len(), 2); @@ -214,7 +247,7 @@ mod blocking_and_async_io { update_refs } fetch::Status::DryRun { update_refs } => update_refs, - fetch::Status::NoChange => unreachable!("we firmly expect changes here"), + fetch::Status::NoPackReceived { .. } => unreachable!("we firmly expect changes here"), }; assert_eq!( @@ -230,9 +263,11 @@ mod blocking_and_async_io { } ] ); - for (_update, mapping, _spec, edit) in - refs.iter_mapping_updates(&outcome.ref_map.mappings, remote.refspecs(Fetch)) - { + for (_update, mapping, _spec, edit) in refs.iter_mapping_updates( + &outcome.ref_map.mappings, + remote.refspecs(Fetch), + &outcome.ref_map.extra_refspecs, + ) { let edit = edit.expect("refedit present even if it's a no-op"); if dry_run { assert_eq!( diff --git a/git-repository/tests/remote/mod.rs b/git-repository/tests/remote/mod.rs index d832bc32c05..85355725deb 100644 --- a/git-repository/tests/remote/mod.rs +++ b/git-repository/tests/remote/mod.rs @@ -51,7 +51,8 @@ pub(crate) fn into_daemon_remote_if_async<'repo, 'a>( _daemon.expect("daemon is available in async mode").url, _repo_name.into().unwrap_or_default() )) - .expect("valid url to create remote at"); + .expect("valid url to create remote at") + .with_fetch_tags(remote.fetch_tags()); for direction in [git::remote::Direction::Fetch, git::remote::Direction::Push] { new_remote .replace_refspecs( diff --git a/git-repository/tests/remote/ref_map.rs b/git-repository/tests/remote/ref_map.rs index 87d9ad4d168..1e770b9dba0 100644 --- a/git-repository/tests/remote/ref_map.rs +++ b/git-repository/tests/remote/ref_map.rs @@ -16,10 +16,44 @@ mod blocking_and_async_io { )] async fn all() -> crate::Result { let daemon = spawn_git_daemon_if_async(remote::repo_path("base"))?; - for (version, expected_remote_refs) in [ - (None, 11), - (Some(git::protocol::transport::Protocol::V2), 11), - (Some(git::protocol::transport::Protocol::V1), 14), // V1 doesn't support prefiltering. + for (fetch_tags, version, expected_remote_refs, expected_mappings) in [ + (git::remote::fetch::Tags::None, None, 11, 11), + ( + git::remote::fetch::Tags::None, + Some(git::protocol::transport::Protocol::V2), + 11, + 11, + ), + ( + git::remote::fetch::Tags::Included, + Some(git::protocol::transport::Protocol::V2), + 17, + 17, + ), + ( + git::remote::fetch::Tags::All, + Some(git::protocol::transport::Protocol::V2), + 17, + 17, + ), + ( + git::remote::fetch::Tags::None, + Some(git::protocol::transport::Protocol::V1), + 18, + 11, + ), + ( + git::remote::fetch::Tags::Included, + Some(git::protocol::transport::Protocol::V1), + 18, + 17, + ), + ( + git::remote::fetch::Tags::All, + Some(git::protocol::transport::Protocol::V1), + 18, + 17, + ), ] { let mut repo = remote::repo("clone"); if let Some(version) = version { @@ -31,23 +65,27 @@ mod blocking_and_async_io { )?; } - let remote = into_daemon_remote_if_async(repo.find_remote("origin")?, daemon.as_ref(), None); + let remote = into_daemon_remote_if_async( + repo.find_remote("origin")?.with_fetch_tags(fetch_tags), + daemon.as_ref(), + None, + ); let map = remote .connect(Fetch, progress::Discard) .await? .ref_map(Default::default()) .await?; assert_eq!( - map.remote_refs.len(), - expected_remote_refs, - "{:?}: it gets all remote refs, independently of the refspec. But we use a prefix so pre-filter them.", - version - ); + map.remote_refs.len(), + expected_remote_refs , + "{:?} fetch-tags={fetch_tags:?}: it gets all remote refs, independently of the refspec. But we use a prefix so pre-filter them.", + version + ); assert_eq!(map.fixes.len(), 0); assert_eq!( map.mappings.len(), - 11, + expected_mappings, "mappings are only a sub-set of all remotes due to refspec matching, tags are filtered out." ); } diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 1778a14d25c..11c06da0f23 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -50,7 +50,7 @@ mod save_as_to { use crate::{basic_repo, remote::save::uniformize}; #[test] - fn anonymous_remotes_cannot_be_save_lacking_a_name() -> crate::Result { + fn anonymous_remotes_cannot_be_saved_lacking_a_name() -> crate::Result { let repo = basic_repo()?; let remote = repo.remote_at("https://example.com/path")?; assert!(matches!( @@ -66,6 +66,7 @@ mod save_as_to { let mut remote = repo .remote_at("https://example.com/path")? .push_url("https://ein.hub/path")? + .with_fetch_tags(git::remote::fetch::Tags::All) .with_refspecs( [ "+refs/heads/*:refs/remotes/any/*", @@ -88,7 +89,7 @@ mod save_as_to { assert_eq!(remote.name(), None); let mut config = git::config::File::default(); remote.save_as_to(remote_name, &mut config)?; - let expected = "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n"; + let expected = "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\ttagOpt = --tags\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n"; assert_eq!(uniformize(config.to_string()), expected); remote.save_as_to(remote_name, &mut config)?; @@ -114,7 +115,7 @@ mod save_as_to { remote.save_as_to(remote_name, &mut config)?; assert_eq!( uniformize(config.to_string()), - "[remote \"origin\"]\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", + "[remote \"origin\"]\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\ttagOpt = --tags\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", "unrelated keys are kept, and so are keys in the sections we edit" ); Ok(()) diff --git a/git-repository/tests/repository/remote.rs b/git-repository/tests/repository/remote.rs index e2fd5fc3b33..941db10c758 100644 --- a/git-repository/tests/repository/remote.rs +++ b/git-repository/tests/repository/remote.rs @@ -110,7 +110,21 @@ mod find_remote { use crate::remote; #[test] - fn typical() { + fn tags_option() -> crate::Result { + let repo = remote::repo("clone-no-tags"); + for (remote_name, expected) in [ + ("origin", git::remote::fetch::Tags::None), + ("myself-no-tags", git::remote::fetch::Tags::None), + ("myself-with-tags", git::remote::fetch::Tags::All), + ] { + let remote = repo.find_remote(remote_name)?; + assert_eq!(remote.fetch_tags(), expected, "specifically set in this repo"); + } + Ok(()) + } + + #[test] + fn typical() -> crate::Result { let repo = remote::repo("clone"); let mut count = 0; let base_dir = base_dir(&repo); @@ -120,11 +134,17 @@ mod find_remote { ]; for (name, (url, refspec)) in repo.remote_names().into_iter().zip(expected) { count += 1; - let remote = repo.find_remote(name).expect("no error"); + let remote = repo.find_remote(name)?; assert_eq!(remote.name().expect("set").as_bstr(), name); - let url = git::url::parse(url.into()).expect("valid"); - assert_eq!(remote.url(Direction::Fetch).unwrap(), &url); + assert_eq!( + remote.fetch_tags(), + git::remote::fetch::Tags::Included, + "the default value as it's not specified" + ); + + let url = git::url::parse(url.into())?; + assert_eq!(remote.url(Direction::Fetch).expect("present"), &url); assert_eq!( remote.refspecs(Direction::Fetch), @@ -142,6 +162,7 @@ mod find_remote { repo.find_remote("unknown").unwrap_err(), git::remote::find::existing::Error::NotFound { .. } )); + Ok(()) } #[test] From 0cefadbe92d170241632c69b4215a795ab172301 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 13 Dec 2022 22:14:28 +0100 Subject: [PATCH 07/15] adapt to changes in `git-repository` --- gitoxide-core/src/repository/clone.rs | 2 +- gitoxide-core/src/repository/fetch.rs | 16 ++++------------ gitoxide-core/src/repository/remote.rs | 9 ++++++--- .../pack/receive/file-v-any-no-output-single-ref | 4 ++-- 4 files changed, 13 insertions(+), 18 deletions(-) diff --git a/gitoxide-core/src/repository/clone.rs b/gitoxide-core/src/repository/clone.rs index 7a0560f91f3..0351fd3aba4 100644 --- a/gitoxide-core/src/repository/clone.rs +++ b/gitoxide-core/src/repository/clone.rs @@ -67,7 +67,7 @@ pub(crate) mod function { } match fetch_outcome.status { - Status::NoChange => { + Status::NoPackReceived { .. } => { unreachable!("clone always has changes") } Status::DryRun { .. } => unreachable!("dry-run unsupported"), diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index d76bdadd73e..bfbc12e8a36 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -60,16 +60,8 @@ pub(crate) mod function { let ref_specs = remote.refspecs(git::remote::Direction::Fetch); match res.status { - Status::NoChange => { - let show_unmapped = false; - crate::repository::remote::refs::print_refmap( - &repo, - ref_specs, - res.ref_map, - show_unmapped, - &mut out, - err, - ) + Status::NoPackReceived { update_refs } => { + print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err) } Status::DryRun { update_refs } => print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err), Status::Change { @@ -100,9 +92,9 @@ pub(crate) mod function { mut out: impl std::io::Write, mut err: impl std::io::Write, ) -> anyhow::Result<()> { - let mut last_spec_index = usize::MAX; + let mut last_spec_index = git::remote::fetch::SpecIndex::ExplicitInRemote(usize::MAX); let mut updates = update_refs - .iter_mapping_updates(&map.mappings, refspecs) + .iter_mapping_updates(&map.mappings, refspecs, &map.extra_refspecs) .filter_map(|(update, mapping, spec, edit)| spec.map(|spec| (update, mapping, spec, edit))) .collect::>(); updates.sort_by_key(|t| t.2); diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 0d7f03c6a70..99a8c951052 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -32,7 +32,7 @@ mod refs_impl { pub handshake_info: bool, } - pub(crate) use super::{print, print_ref, print_refmap}; + pub(crate) use super::{print, print_ref}; } #[git::protocol::maybe_async::maybe_async] @@ -116,12 +116,15 @@ mod refs_impl { mut out: impl std::io::Write, mut err: impl std::io::Write, ) -> anyhow::Result<()> { - let mut last_spec_index = usize::MAX; + let mut last_spec_index = git::remote::fetch::SpecIndex::ExplicitInRemote(usize::MAX); map.mappings.sort_by_key(|m| m.spec_index); for mapping in &map.mappings { if mapping.spec_index != last_spec_index { last_spec_index = mapping.spec_index; - let spec = &refspecs[mapping.spec_index]; + let spec = mapping + .spec_index + .get(refspecs, &map.extra_refspecs) + .expect("refspecs here are the ones used for mapping"); spec.to_ref().write_to(&mut out)?; writeln!(out)?; } diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-no-output-single-ref b/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-no-output-single-ref index 84260bc4021..f712606d55f 100644 --- a/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-no-output-single-ref +++ b/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-no-output-single-ref @@ -1,2 +1,2 @@ -index: c787de2aafb897417ca8167baeb146eabd18bc5f -pack: 346574b7331dc3a1724da218d622c6e1b6c66a57 \ No newline at end of file +index: 3ff97f80d63a1261147ace4cb06191a2fd686ff6 +pack: de6c8d1e0ca3ee9331a3f92da74add15abd03049 \ No newline at end of file From 3ebb3405f265fbd78a89c01bd30c3ad34036ef84 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 12 Dec 2022 16:06:21 +0100 Subject: [PATCH 08/15] update `gix progress` to inform about `remote..tagOpt` --- src/plumbing/progress.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index 7d94461f482..29d8af93cac 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -729,6 +729,13 @@ static GIT_CONFIG: &[Record] = &[ note: None } }, + Record { + config: "remote..tagOpt", + usage: InModule { + name: "repository::remote", + deviation: None + } + }, Record { config: "gitoxide.userAgent", usage: InModule { From 9dd5659c386e97900a9c2d8c28ac70cc64ed0a52 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 14 Dec 2022 16:28:39 +0100 Subject: [PATCH 09/15] feat: `gix remote ref-map` makes clear which specs are implicit. Normally refspecs are coming from the configuration only, but now implied specs due to the implicit include-tag feature also matter. --- gitoxide-core/src/repository/fetch.rs | 37 +++++++++++++++++++++++++- gitoxide-core/src/repository/remote.rs | 15 ++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index bfbc12e8a36..ba55c95b56b 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -46,6 +46,7 @@ pub(crate) mod function { let mut remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref())?; if !ref_specs.is_empty() { remote.replace_refspecs(ref_specs.iter(), git::remote::Direction::Fetch)?; + remote = remote.with_fetch_tags(git::remote::fetch::Tags::None); } let res: git::remote::fetch::Outcome = remote .connect(git::remote::Direction::Fetch, progress)? @@ -98,13 +99,46 @@ pub(crate) mod function { .filter_map(|(update, mapping, spec, edit)| spec.map(|spec| (update, mapping, spec, edit))) .collect::>(); updates.sort_by_key(|t| t.2); + let mut skipped_due_to_implicit_tag = None; + fn consume_skipped_tags(skipped: &mut Option, out: &mut impl std::io::Write) -> std::io::Result<()> { + if let Some(skipped) = skipped.take() { + if skipped != 0 { + writeln!( + out, + "\tskipped {skipped} tags known to the remote without bearing on this commit-graph. Use `gix remote ref-map` to list them." + )?; + } + } + Ok(()) + } for (update, mapping, spec, edit) in updates { if mapping.spec_index != last_spec_index { last_spec_index = mapping.spec_index; + consume_skipped_tags(&mut skipped_due_to_implicit_tag, &mut out)?; spec.to_ref().write_to(&mut out)?; + let is_implicit = mapping.spec_index.implicit_index().is_some(); + if is_implicit { + write!(&mut out, " (implicit")?; + if spec.to_ref() + == git::remote::fetch::Tags::Included + .to_refspec() + .expect("always yields refspec") + { + skipped_due_to_implicit_tag = Some(0); + write!(&mut out, ", due to auto-tag")?; + } + write!(&mut out, ")")?; + } writeln!(out)?; } + if let Some(num_skipped) = skipped_due_to_implicit_tag.as_mut() { + if matches!(update.mode, git::remote::fetch::refs::update::Mode::NoChangeNeeded) { + *num_skipped += 1; + continue; + } + } + write!(out, "\t")?; match &mapping.remote { git::remote::fetch::Source::ObjectId(id) => { @@ -118,9 +152,10 @@ pub(crate) mod function { Some(edit) => { writeln!(out, " -> {} [{}]", edit.name, update.mode) } - None => writeln!(out, " (fetch only)"), + None => writeln!(out, " [{}]", update.mode), }?; } + consume_skipped_tags(&mut skipped_due_to_implicit_tag, &mut out)?; if !map.fixes.is_empty() { writeln!( err, diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 99a8c951052..15aff7c1a46 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -60,6 +60,7 @@ mod refs_impl { } if !ref_specs.is_empty() { remote.replace_refspecs(ref_specs.iter(), git::remote::Direction::Fetch)?; + remote = remote.with_fetch_tags(git::remote::fetch::Tags::None); } *show_unmapped_remote_refs } else { @@ -126,6 +127,18 @@ mod refs_impl { .get(refspecs, &map.extra_refspecs) .expect("refspecs here are the ones used for mapping"); spec.to_ref().write_to(&mut out)?; + let is_implicit = mapping.spec_index.implicit_index().is_some(); + if is_implicit { + write!(&mut out, " (implicit")?; + if spec.to_ref() + == git::remote::fetch::Tags::Included + .to_refspec() + .expect("always yields refspec") + { + write!(&mut out, ", due to auto-tag")?; + } + write!(&mut out, ")")?; + } writeln!(out)?; } @@ -203,7 +216,7 @@ mod refs_impl { } } if refspecs.is_empty() { - bail!("Without ref-specs there is nothing to show here. Add ref-specs as arguments or configure them in git-config.") + bail!("Without refspecs there is nothing to show here. Add refspecs as arguments or configure them in git-config.") } Ok(()) } From 4ba22358026bd4b2f4c129e6504af208166daf0d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 12:47:34 +0100 Subject: [PATCH 10/15] upgade jwalk for good measure --- Cargo.lock | 14 ++++++++++++-- gitoxide-core/Cargo.toml | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca5e49ac079..f2acec5ca9a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1386,7 +1386,7 @@ dependencies = [ "document-features", "flate2", "git-hash", - "jwalk", + "jwalk 0.6.0", "libc", "num_cpus", "once_cell", @@ -1967,7 +1967,7 @@ dependencies = [ "git-transport", "git-url", "itertools", - "jwalk", + "jwalk 0.8.0", "mime_guess", "num_cpus", "serde", @@ -2336,6 +2336,16 @@ dependencies = [ "rayon", ] +[[package]] +name = "jwalk" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37cb840f933b9fa56c78fde73acc9f2c883a2594fc6c791992a133468106965d" +dependencies = [ + "crossbeam", + "rayon", +] + [[package]] name = "kv-log-macro" version = "1.0.7" diff --git a/gitoxide-core/Cargo.toml b/gitoxide-core/Cargo.toml index 78e3fc3bf66..b7b25f58402 100644 --- a/gitoxide-core/Cargo.toml +++ b/gitoxide-core/Cargo.toml @@ -57,7 +57,7 @@ blocking = { version = "1.0.2", optional = true } # for 'organize' functionality git-url = { version = "^0.11.0", path = "../git-url", optional = true } -jwalk = { version = "0.6.0", optional = true } +jwalk = { version = "0.8.0", optional = true } # for 'hours' itertools = { version = "0.10.1", optional = true } From 0f27c67c92fc0bc23a6712b5c4c730ad6a0156bf Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 13:02:34 +0100 Subject: [PATCH 11/15] feat!: add support for explicit non-parallel iteration. That way we can allow the implementation to choose whether they need greatest speed at some cost or not. This also allows us to create a new thread-pool on each iteration as those who expect high cost or many files will likely chose to do that instead of single-threaded iteration, which nicely contains the threads needed and avoids keeping them alive as part of some global pool. --- Cargo.lock | 18 +++--------- Makefile | 2 +- git-features/Cargo.toml | 5 ++-- git-features/src/fs.rs | 62 +++++++++++++++++++++++++++++++---------- 4 files changed, 55 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2acec5ca9a..02513543c03 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1386,7 +1386,7 @@ dependencies = [ "document-features", "flate2", "git-hash", - "jwalk 0.6.0", + "jwalk", "libc", "num_cpus", "once_cell", @@ -1967,7 +1967,7 @@ dependencies = [ "git-transport", "git-url", "itertools", - "jwalk 0.8.0", + "jwalk", "mime_guess", "num_cpus", "serde", @@ -2328,19 +2328,9 @@ dependencies = [ [[package]] name = "jwalk" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "172752e853a067cbce46427de8470ddf308af7fd8ceaf9b682ef31a5021b6bb9" -dependencies = [ - "crossbeam", - "rayon", -] - -[[package]] -name = "jwalk" -version = "0.8.0" +version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "37cb840f933b9fa56c78fde73acc9f2c883a2594fc6c791992a133468106965d" +checksum = "2735847566356cd2179a2a38264839308f7079fa96e6bd5a42d740460e003c56" dependencies = [ "crossbeam", "rayon", diff --git a/Makefile b/Makefile index 69a240b0758..635698d5ac5 100644 --- a/Makefile +++ b/Makefile @@ -95,7 +95,7 @@ check: ## Build all code in suitable configurations && cargo check cd git-features && cargo check --all-features \ && cargo check --features parallel \ - && cargo check --features parallel,fs-walkdir-parallel \ + && cargo check --features fs-walkdir-parallel \ && cargo check --features rustsha1 \ && cargo check --features fast-sha1 \ && cargo check --features progress \ diff --git a/git-features/Cargo.toml b/git-features/Cargo.toml index 2252ec1c6ce..cce1753796d 100644 --- a/git-features/Cargo.toml +++ b/git-features/Cargo.toml @@ -19,8 +19,7 @@ default = [] progress = ["prodash"] ## If set, walkdir iterators will be multi-threaded. -## This feature has [certain side-effects](https://github.com/starship/starship/issues/4251) of rayon threadpool configuration with `jwalk`. -fs-walkdir-parallel = ["parallel", "jwalk" ] +fs-walkdir-parallel = [ "num_cpus", "jwalk" ] ## Use scoped threads and channels to parallelize common workloads on multiple objects. If enabled, it is used everywhere ## where it makes sense. @@ -107,7 +106,7 @@ crossbeam-channel = { version = "0.5.0", optional = true } num_cpus = { version = "1.13.0", optional = true } parking_lot = { version = "0.12.0", default-features = false, optional = true } -jwalk = { version = "0.6.0", optional = true } +jwalk = { version = "0.8.1", optional = true } ## Makes facilities of the `walkdir` crate partially available. ## In conjunction with the **parallel** feature, directory walking will be parallel instead behind a compatible interface. walkdir = { version = "2.3.2", optional = true } # used when parallel is off diff --git a/git-features/src/fs.rs b/git-features/src/fs.rs index 33f69b099de..c20f38b973b 100644 --- a/git-features/src/fs.rs +++ b/git-features/src/fs.rs @@ -6,29 +6,62 @@ //! For information on how to use the [`WalkDir`] type, have a look at //! * [`jwalk::WalkDir`](https://docs.rs/jwalk/0.5.1/jwalk/type.WalkDir.html) if `parallel` feature is enabled //! * [walkdir::WalkDir](https://docs.rs/walkdir/2.3.1/walkdir/struct.WalkDir.html) otherwise -#[cfg(feature = "fs-walkdir-parallel")] + +#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))] +mod shared { + /// The desired level of parallelism. + pub enum Parallelism { + /// Do not parallelize at all by making a serial traversal on the current thread. + Serial, + /// Create a new thread pool for each traversal with up to 16 threads or the amount of logical cores of the machine. + ThreadPoolPerTraversal { + /// The base name of the threads we create as part of the thread-pool. + thread_name: &'static str, + }, + } +} + /// +#[cfg(feature = "fs-walkdir-parallel")] pub mod walkdir { - use std::path::Path; - + pub use super::shared::Parallelism; pub use jwalk::{DirEntry as DirEntryGeneric, DirEntryIter as DirEntryIterGeneric, Error, WalkDir}; + use std::path::Path; /// An alias for an uncustomized directory entry to match the one of the non-parallel version offered by `walkdir`. pub type DirEntry = DirEntryGeneric<((), ())>; - /// Instantiate a new directory iterator which will not skip hidden files. - pub fn walkdir_new(root: impl AsRef) -> WalkDir { - WalkDir::new(root) - .skip_hidden(false) - .parallelism(jwalk::Parallelism::Serial) + impl From for jwalk::Parallelism { + fn from(v: Parallelism) -> Self { + match v { + Parallelism::Serial => jwalk::Parallelism::Serial, + Parallelism::ThreadPoolPerTraversal { thread_name } => { + let pool = jwalk::rayon::ThreadPoolBuilder::new() + .num_threads(num_cpus::get().min(16)) + .stack_size(128 * 1024) + .thread_name(move |idx| format!("{thread_name} {idx}")) + .build() + .expect("we only set options that can't cause a build failure"); + jwalk::Parallelism::RayonExistingPool { + pool: pool.into(), + busy_timeout: None, + } + } + } + } + } + + /// Instantiate a new directory iterator which will not skip hidden files, with the given level of `parallelism`. + pub fn walkdir_new(root: impl AsRef, parallelism: Parallelism) -> WalkDir { + WalkDir::new(root).skip_hidden(false).parallelism(parallelism.into()) } /// Instantiate a new directory iterator which will not skip hidden files and is sorted - pub fn walkdir_sorted_new(root: impl AsRef) -> WalkDir { + pub fn walkdir_sorted_new(root: impl AsRef, parallelism: Parallelism) -> WalkDir { WalkDir::new(root) .skip_hidden(false) .sort(true) - .parallelism(jwalk::Parallelism::Serial) + .parallelism(parallelism.into()) } /// The Iterator yielding directory items @@ -38,17 +71,18 @@ pub mod walkdir { #[cfg(all(feature = "walkdir", not(feature = "fs-walkdir-parallel")))] /// pub mod walkdir { + pub use super::shared::Parallelism; use std::path::Path; pub use walkdir::{DirEntry, Error, WalkDir}; - /// Instantiate a new directory iterator which will not skip hidden files. - pub fn walkdir_new(root: impl AsRef) -> WalkDir { + /// Instantiate a new directory iterator which will not skip hidden files, with the given level of `parallelism`. + pub fn walkdir_new(root: impl AsRef, _: Parallelism) -> WalkDir { WalkDir::new(root) } - /// Instantiate a new directory iterator which will not skip hidden files and is sorted - pub fn walkdir_sorted_new(root: impl AsRef) -> WalkDir { + /// Instantiate a new directory iterator which will not skip hidden files and is sorted, with the given level of `parallelism`. + pub fn walkdir_sorted_new(root: impl AsRef, _: Parallelism) -> WalkDir { WalkDir::new(root).sort_by_file_name() } From 35f7d5960210738d88d35aef9c1ed3480681c481 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 13:51:05 +0100 Subject: [PATCH 12/15] adapt to changes in `git-features::fs`. We only use parallel traversal where it matters most, namely loose object traversal which can be many objects in up to 256 folders. Being able to parallelize among them will be a noticable speedup well worth the cost (presumably). Ref traversals are single-threaded as we expect most refs in packed-refs anyway. Furthermore we can now consider parallel traversal safe as we don't use the global rayon pool anymore, something that can break any parallel iteration (and just a version of `jwalk` earlier it would deadlock). --- git-odb/src/store_impls/loose/find.rs | 13 ++++++++----- git-odb/src/store_impls/loose/iter.rs | 15 ++++++++++----- git-ref/src/store/file/loose/iter.rs | 7 ++++--- git-repository/Cargo.toml | 5 +++-- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/git-odb/src/store_impls/loose/find.rs b/git-odb/src/store_impls/loose/find.rs index 989140cf84d..e7b1912bd4c 100644 --- a/git-odb/src/store_impls/loose/find.rs +++ b/git-odb/src/store_impls/loose/find.rs @@ -49,11 +49,14 @@ impl Store { mut candidates: Option<&mut HashSet>, ) -> Result, crate::loose::iter::Error> { let single_directory_iter = crate::loose::Iter { - inner: git_features::fs::walkdir_new(&self.path.join(prefix.as_oid().to_hex_with_len(2).to_string())) - .min_depth(1) - .max_depth(1) - .follow_links(false) - .into_iter(), + inner: git_features::fs::walkdir_new( + &self.path.join(prefix.as_oid().to_hex_with_len(2).to_string()), + git_features::fs::walkdir::Parallelism::Serial, + ) + .min_depth(1) + .max_depth(1) + .follow_links(false) + .into_iter(), hash_hex_len: prefix.as_oid().kind().len_in_hex(), }; let mut candidate = None; diff --git a/git-odb/src/store_impls/loose/iter.rs b/git-odb/src/store_impls/loose/iter.rs index 6da87c0a9a1..0ec33bd8429 100644 --- a/git-odb/src/store_impls/loose/iter.rs +++ b/git-odb/src/store_impls/loose/iter.rs @@ -65,11 +65,16 @@ impl loose::Store { /// needed if iterators need to be implemented by hand in the absence of generators. pub fn iter(&self) -> loose::Iter { loose::Iter { - inner: fs::walkdir_new(&self.path) - .min_depth(2) - .max_depth(3) - .follow_links(false) - .into_iter(), + inner: fs::walkdir_new( + &self.path, + fs::walkdir::Parallelism::ThreadPoolPerTraversal { + thread_name: "git_odb::loose::Store::iter: fs-walk", + }, + ) + .min_depth(2) + .max_depth(3) + .follow_links(false) + .into_iter(), hash_hex_len: self.object_hash.len_in_hex(), } } diff --git a/git-ref/src/store/file/loose/iter.rs b/git-ref/src/store/file/loose/iter.rs index 33a9b9804d2..1accedb717b 100644 --- a/git-ref/src/store/file/loose/iter.rs +++ b/git-ref/src/store/file/loose/iter.rs @@ -18,9 +18,10 @@ impl SortedLoosePaths { SortedLoosePaths { base: base.into(), filename_prefix, - file_walk: path - .is_dir() - .then(|| git_features::fs::walkdir_sorted_new(path).into_iter()), + file_walk: path.is_dir().then(|| { + // serial iteration as we expect most refs in packed-refs anyway. + git_features::fs::walkdir_sorted_new(path, git_features::fs::walkdir::Parallelism::Serial).into_iter() + }), } } } diff --git a/git-repository/Cargo.toml b/git-repository/Cargo.toml index 3a55e367792..77775f3da73 100644 --- a/git-repository/Cargo.toml +++ b/git-repository/Cargo.toml @@ -64,7 +64,7 @@ serde1 = [ "serde", ## Activate other features that maximize performance, like usage of threads, `zlib-ng` and access to caching in object databases. ## Note that some platforms might suffer from compile failures, which is when `max-performance-safe` should be used. -max-performance = [ "git-features/zlib-ng-compat", "fast-sha1", "max-performance-safe", "git-features/fs-walkdir-parallel" ] +max-performance = [ "git-features/zlib-ng-compat", "fast-sha1", "max-performance-safe" ] ## If enabled, use assembly versions of sha1 on supported platforms. ## This might cause compile failures as well which is why it can be turned off separately. @@ -75,7 +75,8 @@ fast-sha1 = [ "git-features/fast-sha1" ] max-performance-safe = [ "git-features/parallel", "git-pack/pack-cache-lru-static", - "git-pack/pack-cache-lru-dynamic" + "git-pack/pack-cache-lru-dynamic", + "git-features/fs-walkdir-parallel" ] ## Print debugging information about usage of object database caches, useful for tuning cache sizes. cache-efficiency-debug = ["git-features/cache-efficiency-debug"] From e2b8c5dce7185b5fa194b90f32e642e5c9d1227f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 17:29:14 +0100 Subject: [PATCH 13/15] feat: `gix clone --no-tags` support. This is the same as `git clone --no-tags`. --- gitoxide-core/src/repository/clone.rs | 5 +++++ src/plumbing/main.rs | 2 ++ src/plumbing/options/mod.rs | 6 +++++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/gitoxide-core/src/repository/clone.rs b/gitoxide-core/src/repository/clone.rs index 0351fd3aba4..c2ceeaa7253 100644 --- a/gitoxide-core/src/repository/clone.rs +++ b/gitoxide-core/src/repository/clone.rs @@ -4,6 +4,7 @@ pub struct Options { pub format: OutputFormat, pub bare: bool, pub handshake_info: bool, + pub no_tags: bool, } pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=3; @@ -29,6 +30,7 @@ pub(crate) mod function { format, handshake_info, bare, + no_tags, }: Options, ) -> anyhow::Result<()> where @@ -51,6 +53,9 @@ pub(crate) mod function { opts }, )?; + if no_tags { + prepare = prepare.configure_remote(|r| Ok(r.with_fetch_tags(git::remote::fetch::Tags::None))); + } let (mut checkout, fetch_outcome) = prepare.fetch_then_checkout(&mut progress, &git::interrupt::IS_INTERRUPTED)?; diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index ebed5aba977..aa73c2b5d0c 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -124,6 +124,7 @@ pub fn main() -> Result<()> { Subcommands::Clone(crate::plumbing::options::clone::Platform { handshake_info, bare, + no_tags, remote, directory, }) => { @@ -131,6 +132,7 @@ pub fn main() -> Result<()> { format, bare, handshake_info, + no_tags, }; prepare_and_run( "clone", diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 2415205b160..0df898b3479 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -170,10 +170,14 @@ pub mod clone { #[clap(long, short = 'H')] pub handshake_info: bool, - /// If set, the clone will be bare and a working tree checkout won't be available. + /// The clone will be bare and a working tree checkout won't be available. #[clap(long)] pub bare: bool, + /// Do not clone any tags. Useful to reduce the size of the clone if only branches are needed. + #[clap(long)] + pub no_tags: bool, + /// The url of the remote to connect to, like `https://github.com/byron/gitoxide`. pub remote: OsString, From f1160fb42acf59b37cbeda546a7079af3c9bc050 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 20:54:02 +0100 Subject: [PATCH 14/15] thanks clippy --- git-discover/tests/is_git/mod.rs | 2 +- git-discover/tests/upwards/ceiling_dirs.rs | 12 ++++++------ git-discover/tests/upwards/mod.rs | 12 ++++++------ git-features/src/parallel/mod.rs | 4 +--- git-features/src/progress.rs | 2 +- git-lock/tests/lock/marker.rs | 2 +- git-odb/src/store_impls/dynamic/handle.rs | 2 +- git-odb/src/store_impls/dynamic/load_index.rs | 4 ++-- git-odb/src/store_impls/loose/find.rs | 2 +- git-odb/tests/odb/alternate/mod.rs | 2 +- git-pack/src/cache/delta/traverse/resolve.rs | 2 +- git-pack/src/data/file/decode_entry.rs | 2 +- git-pack/src/data/input/entries_to_bytes.rs | 4 +--- git-pack/src/data/output/bytes.rs | 2 +- git-pack/src/index/traverse/reduce.rs | 4 ++-- git-packetline/src/encode/mod.rs | 2 +- git-packetline/src/write/blocking_io.rs | 2 +- git-path/tests/realpath/mod.rs | 11 ++++------- .../file/loose/reflog/create_or_update/tests.rs | 2 +- git-repository/src/create.rs | 2 +- git-repository/src/repository/init.rs | 2 +- git-repository/src/repository/worktree.rs | 2 +- tests/tools/src/lib.rs | 4 ++-- 23 files changed, 39 insertions(+), 46 deletions(-) diff --git a/git-discover/tests/is_git/mod.rs b/git-discover/tests/is_git/mod.rs index 412f6cce622..d49f5e3d385 100644 --- a/git-discover/tests/is_git/mod.rs +++ b/git-discover/tests/is_git/mod.rs @@ -54,7 +54,7 @@ fn missing_configuration_file_is_not_a_dealbreaker_in_bare_repo() -> crate::Resu fn missing_configuration_file_is_not_a_dealbreaker_in_nonbare_repo() -> crate::Result { for name in ["worktree-no-config-after-init/.git", "worktree-no-config/.git"] { let repo = repo_path()?.join(name); - let kind = git_discover::is_git(&repo)?; + let kind = git_discover::is_git(repo)?; assert_eq!(kind, git_discover::repository::Kind::WorkTree { linked_git_dir: None }); } Ok(()) diff --git a/git-discover/tests/upwards/ceiling_dirs.rs b/git-discover/tests/upwards/ceiling_dirs.rs index 955ad0ffbfd..819038140fe 100644 --- a/git-discover/tests/upwards/ceiling_dirs.rs +++ b/git-discover/tests/upwards/ceiling_dirs.rs @@ -19,7 +19,7 @@ fn git_dir_candidate_within_ceiling_allows_discovery() -> crate::Result { let work_dir = repo_path()?; let dir = work_dir.join("some/very/deeply/nested/subdir"); let (repo_path, _trust) = git_discover::upwards_opts( - &dir, + dir, Options { ceiling_dirs: vec![work_dir.clone()], ..Default::default() @@ -37,7 +37,7 @@ fn ceiling_dir_limits_are_respected_and_prevent_discovery() -> crate::Result { let dir = work_dir.join("some/very/deeply/nested/subdir"); let err = git_discover::upwards_opts( - &dir, + dir, Options { ceiling_dirs: vec![work_dir.join("some/../some")], ..Default::default() @@ -57,7 +57,7 @@ fn no_matching_ceiling_dir_error_can_be_suppressed() -> crate::Result { let work_dir = repo_path()?; let dir = work_dir.join("some/very/deeply/nested/subdir"); let (repo_path, _trust) = git_discover::upwards_opts( - &dir, + dir, Options { match_ceiling_dir_or_error: false, ceiling_dirs: vec![ @@ -80,7 +80,7 @@ fn more_restrictive_ceiling_dirs_overrule_less_restrictive_ones() -> crate::Resu let work_dir = repo_path()?; let dir = work_dir.join("some/very/deeply/nested/subdir"); let err = git_discover::upwards_opts( - &dir, + dir, Options { ceiling_dirs: vec![work_dir.clone(), work_dir.join("some")], ..Default::default() @@ -100,7 +100,7 @@ fn ceiling_dirs_are_not_processed_differently_than_the_git_dir_candidate() -> cr let work_dir = repo_path()?; let dir = work_dir.join("some/very/deeply/nested/subdir/../../../../../.."); let (repo_path, _trust) = git_discover::upwards_opts( - &dir, + dir, Options { match_ceiling_dir_or_error: false, ceiling_dirs: vec![Path::new("./some").into()], @@ -123,7 +123,7 @@ fn no_matching_ceiling_dirs_errors_by_default() -> crate::Result { let relative_work_dir = repo_path()?; let dir = relative_work_dir.join("some"); let res = git_discover::upwards_opts( - &dir, + dir, Options { ceiling_dirs: vec!["/something/somewhere".into()], ..Default::default() diff --git a/git-discover/tests/upwards/mod.rs b/git-discover/tests/upwards/mod.rs index 37addfb118c..8ac8411ae28 100644 --- a/git-discover/tests/upwards/mod.rs +++ b/git-discover/tests/upwards/mod.rs @@ -38,7 +38,7 @@ fn from_bare_git_dir_without_config_file() -> crate::Result { fn from_inside_bare_git_dir() -> crate::Result { let git_dir = repo_path()?.join("bare.git"); let dir = git_dir.join("objects"); - let (path, trust) = git_discover::upwards(&dir)?; + let (path, trust) = git_discover::upwards(dir)?; assert_eq!( path.as_ref(), git_dir, @@ -89,7 +89,7 @@ fn from_working_dir_no_config() -> crate::Result { fn from_nested_dir() -> crate::Result { let working_dir = repo_path()?; let dir = working_dir.join("some/very/deeply/nested/subdir"); - let (path, trust) = git_discover::upwards(&dir)?; + let (path, trust) = git_discover::upwards(dir)?; assert_eq!(path.kind(), Kind::WorkTree { linked_git_dir: None }); assert_eq!(path.as_ref(), working_dir, "a working tree dir yields the git dir"); assert_eq!(trust, expected_trust()); @@ -105,7 +105,7 @@ fn from_dir_with_dot_dot() -> crate::Result { // exploring ancestors.) let working_dir = repo_path()?; let dir = working_dir.join("some/very/deeply/nested/subdir/../../../../../.."); - let (path, trust) = git_discover::upwards(&dir)?; + let (path, trust) = git_discover::upwards(dir)?; assert_ne!( path.as_ref().canonicalize()?, working_dir.canonicalize()?, @@ -140,7 +140,7 @@ fn from_dir_with_dot_dot() -> crate::Result { fn from_nested_dir_inside_a_git_dir() -> crate::Result { let working_dir = repo_path()?; let dir = working_dir.join(".git").join("objects"); - let (path, trust) = git_discover::upwards(&dir)?; + let (path, trust) = git_discover::upwards(dir)?; assert_eq!(path.kind(), Kind::WorkTree { linked_git_dir: None }); assert_eq!(path.as_ref(), working_dir, "we find .git directories on the way"); assert_eq!(trust, expected_trust()); @@ -269,7 +269,7 @@ fn cross_fs() -> crate::Result { )); let (repo_path, _trust) = git_discover::upwards_opts( - &top_level_repo.path().join("remote"), + top_level_repo.path().join("remote"), Options { cross_fs: true, ..Default::default() @@ -322,7 +322,7 @@ mod submodules { submodule_m1_gitdir ); - let (path, _trust) = git_discover::upwards(&submodule_m1_workdir.join("subdir"))?; + let (path, _trust) = git_discover::upwards(submodule_m1_workdir.join("subdir"))?; assert!( matches!(path, git_discover::repository::Path::LinkedWorkTree{ref work_dir, ref git_dir} if work_dir == &submodule_m1_workdir && git_dir == &submodule_m1_gitdir), "{:?} should match {:?} {:?}", diff --git a/git-features/src/parallel/mod.rs b/git-features/src/parallel/mod.rs index e16ba93f2a0..64ff050b4f2 100644 --- a/git-features/src/parallel/mod.rs +++ b/git-features/src/parallel/mod.rs @@ -88,9 +88,7 @@ pub fn optimize_chunk_size_and_thread_limit( .map(|num_items| { let desired_chunks_per_thread_at_least = 2; let items = num_items; - let chunk_size = (items / (available_threads * desired_chunks_per_thread_at_least)) - .max(1) - .min(upper); + let chunk_size = (items / (available_threads * desired_chunks_per_thread_at_least)).clamp(1, upper); let num_chunks = items / chunk_size; let thread_limit = if num_chunks <= available_threads { (num_chunks / desired_chunks_per_thread_at_least).max(1) diff --git a/git-features/src/progress.rs b/git-features/src/progress.rs index b5fb6f164e6..18200e49185 100644 --- a/git-features/src/progress.rs +++ b/git-features/src/progress.rs @@ -50,7 +50,7 @@ where { fn read(&mut self, buf: &mut [u8]) -> io::Result { let bytes_read = self.inner.read(buf)?; - self.progress.inc_by(bytes_read as usize); + self.progress.inc_by(bytes_read); Ok(bytes_read) } } diff --git a/git-lock/tests/lock/marker.rs b/git-lock/tests/lock/marker.rs index e46cfc658ee..db6d8c9cf08 100644 --- a/git-lock/tests/lock/marker.rs +++ b/git-lock/tests/lock/marker.rs @@ -71,7 +71,7 @@ mod commit { fn fails_for_ordinary_marker_that_was_never_writable() -> crate::Result { let dir = tempfile::tempdir()?; let resource = dir.path().join("the-resource"); - let mark = git_lock::Marker::acquire_to_hold_resource(&resource, Fail::Immediately, None)?; + let mark = git_lock::Marker::acquire_to_hold_resource(resource, Fail::Immediately, None)?; let err = mark.commit().expect_err("should always fail"); assert_eq!(err.error.kind(), std::io::ErrorKind::Other); assert_eq!( diff --git a/git-odb/src/store_impls/dynamic/handle.rs b/git-odb/src/store_impls/dynamic/handle.rs index c4d3e16c33f..18c33ee4c58 100644 --- a/git-odb/src/store_impls/dynamic/handle.rs +++ b/git-odb/src/store_impls/dynamic/handle.rs @@ -94,7 +94,7 @@ pub(crate) mod index_lookup { "BUG: multi-pack index must be set if this is a multi-pack, pack-indices seem unstable", ); Box::new(index.iter().filter_map(move |e| { - (e.pack_index == pack_index as u32).then(|| git_pack::index::Entry { + (e.pack_index == pack_index).then(|| git_pack::index::Entry { oid: e.oid, pack_offset: e.pack_offset, crc32: None, diff --git a/git-odb/src/store_impls/dynamic/load_index.rs b/git-odb/src/store_impls/dynamic/load_index.rs index 034c341fe75..854b0f57839 100644 --- a/git-odb/src/store_impls/dynamic/load_index.rs +++ b/git-odb/src/store_impls/dynamic/load_index.rs @@ -279,7 +279,7 @@ impl super::Store { .unwrap_or(0); let mut num_indices_checked = 0; let mut needs_generation_change = false; - let mut slot_indices_to_remove: Vec<_> = idx_by_index_path.into_iter().map(|(_, v)| v).collect(); + let mut slot_indices_to_remove: Vec<_> = idx_by_index_path.into_values().collect(); while let Some((mut index_info, mtime, move_from_slot_idx)) = index_paths_to_add.pop_front() { 'increment_slot_index: loop { if num_indices_checked == self.files.len() { @@ -419,7 +419,7 @@ impl super::Store { let mut indices_by_modification_time = Vec::with_capacity(initial_capacity.unwrap_or_default()); for db_path in db_paths { let packs = db_path.join("pack"); - let entries = match std::fs::read_dir(&packs) { + let entries = match std::fs::read_dir(packs) { Ok(e) => e, Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, Err(err) => return Err(err.into()), diff --git a/git-odb/src/store_impls/loose/find.rs b/git-odb/src/store_impls/loose/find.rs index e7b1912bd4c..3c3baa9ce5f 100644 --- a/git-odb/src/store_impls/loose/find.rs +++ b/git-odb/src/store_impls/loose/find.rs @@ -50,7 +50,7 @@ impl Store { ) -> Result, crate::loose::iter::Error> { let single_directory_iter = crate::loose::Iter { inner: git_features::fs::walkdir_new( - &self.path.join(prefix.as_oid().to_hex_with_len(2).to_string()), + self.path.join(prefix.as_oid().to_hex_with_len(2).to_string()), git_features::fs::walkdir::Parallelism::Serial, ) .min_depth(1) diff --git a/git-odb/tests/odb/alternate/mod.rs b/git-odb/tests/odb/alternate/mod.rs index 13fef133821..28680ca6402 100644 --- a/git-odb/tests/odb/alternate/mod.rs +++ b/git-odb/tests/odb/alternate/mod.rs @@ -67,7 +67,7 @@ fn circular_alternates_are_detected_with_relative_paths() -> crate::Result { None, )?; - match alternate::resolve(&from, std::env::current_dir()?) { + match alternate::resolve(from, std::env::current_dir()?) { Err(alternate::Error::Cycle(chain)) => { assert_eq!( chain diff --git a/git-pack/src/cache/delta/traverse/resolve.rs b/git-pack/src/cache/delta/traverse/resolve.rs index c641a5f5693..83a13eef1d9 100644 --- a/git-pack/src/cache/delta/traverse/resolve.rs +++ b/git-pack/src/cache/delta/traverse/resolve.rs @@ -46,7 +46,7 @@ where pack_offset: slice.start, })?; let entry = crate::data::Entry::from_bytes(&bytes_buf, slice.start, hash_len); - let compressed = &bytes_buf[entry.header_size() as usize..]; + let compressed = &bytes_buf[entry.header_size()..]; let decompressed_len = entry.decompressed_size as usize; Ok((entry, slice.end, decompress_all_at_once(compressed, decompressed_len)?)) }; diff --git a/git-pack/src/data/file/decode_entry.rs b/git-pack/src/data/file/decode_entry.rs index b3a7920deb0..9bd7bfc9b84 100644 --- a/git-pack/src/data/file/decode_entry.rs +++ b/git-pack/src/data/file/decode_entry.rs @@ -381,7 +381,7 @@ impl File { // have been cut short by a cache hit. The caller must deactivate the cache to get // actual results num_deltas: chain_len as u32, - decompressed_size: first_entry.decompressed_size as u64, + decompressed_size: first_entry.decompressed_size, compressed_size: consumed_input, object_size: last_result_size as u64, }) diff --git a/git-pack/src/data/input/entries_to_bytes.rs b/git-pack/src/data/input/entries_to_bytes.rs index f5e6174cb18..c5c0fcc0bd1 100644 --- a/git-pack/src/data/input/entries_to_bytes.rs +++ b/git-pack/src/data/input/entries_to_bytes.rs @@ -72,9 +72,7 @@ where self.output.write_all(&header_bytes[..])?; } self.num_entries += 1; - entry - .header - .write_to(entry.decompressed_size as u64, &mut self.output)?; + entry.header.write_to(entry.decompressed_size, &mut self.output)?; std::io::copy( &mut entry .compressed diff --git a/git-pack/src/data/output/bytes.rs b/git-pack/src/data/output/bytes.rs index ba854d3713d..99878e52392 100644 --- a/git-pack/src/data/output/bytes.rs +++ b/git-pack/src/data/output/bytes.rs @@ -116,7 +116,7 @@ where self.written - base_offset }); self.written += header.write_to(entry.decompressed_size as u64, &mut self.output)? as u64; - self.written += std::io::copy(&mut &*entry.compressed_data, &mut self.output)? as u64; + self.written += std::io::copy(&mut &*entry.compressed_data, &mut self.output)?; } } None => { diff --git a/git-pack/src/index/traverse/reduce.rs b/git-pack/src/index/traverse/reduce.rs index 5611063a099..d38b961025f 100644 --- a/git-pack/src/index/traverse/reduce.rs +++ b/git-pack/src/index/traverse/reduce.rs @@ -89,7 +89,7 @@ where *self.stats.objects_per_chain_length.entry(stats.num_deltas).or_insert(0) += 1; self.stats.total_decompressed_entries_size += stats.decompressed_size; self.stats.total_compressed_entries_size += stats.compressed_size as u64; - self.stats.total_object_size += stats.object_size as u64; + self.stats.total_object_size += stats.object_size; use git_object::Kind::*; match stats.kind { Commit => self.stats.num_commits += 1, @@ -112,7 +112,7 @@ where } fn finalize(mut self) -> Result { - div_decode_result(&mut self.stats.average, self.entries_seen as usize); + div_decode_result(&mut self.stats.average, self.entries_seen); let elapsed_s = self.then.elapsed().as_secs_f32(); let objects_per_second = (self.entries_seen as f32 / elapsed_s) as u32; diff --git a/git-packetline/src/encode/mod.rs b/git-packetline/src/encode/mod.rs index b5a277c8c8f..cd82f12e680 100644 --- a/git-packetline/src/encode/mod.rs +++ b/git-packetline/src/encode/mod.rs @@ -22,6 +22,6 @@ pub use blocking_io::*; pub(crate) fn u16_to_hex(value: u16) -> [u8; 4] { let mut buf = [0u8; 4]; - hex::encode_to_slice((value as u16).to_be_bytes(), &mut buf).expect("two bytes to 4 hex chars never fails"); + hex::encode_to_slice(value.to_be_bytes(), &mut buf).expect("two bytes to 4 hex chars never fails"); buf } diff --git a/git-packetline/src/write/blocking_io.rs b/git-packetline/src/write/blocking_io.rs index 9adc9244465..939d8174038 100644 --- a/git-packetline/src/write/blocking_io.rs +++ b/git-packetline/src/write/blocking_io.rs @@ -59,7 +59,7 @@ impl io::Write for Writer { crate::encode::text_to_write(data, &mut self.inner) }?; // subtract header (and trailng NL) because write-all can't handle writing more than it passes in - written -= U16_HEX_BYTES + if self.binary { 0 } else { 1 }; + written -= U16_HEX_BYTES + usize::from(!self.binary); buf = rest; } Ok(written) diff --git a/git-path/tests/realpath/mod.rs b/git-path/tests/realpath/mod.rs index d04bdc406c1..6021850e6bd 100644 --- a/git-path/tests/realpath/mod.rs +++ b/git-path/tests/realpath/mod.rs @@ -66,7 +66,7 @@ fn link_cycle_is_detected() -> crate::Result { let link_name = "link"; let link_destination = dir.join(link_name); let link_path = dir.join(link_name); - create_symlink(&link_path, &link_destination)?; + create_symlink(&link_path, link_destination)?; let max_symlinks = 8; assert!( @@ -100,7 +100,7 @@ fn symlink_to_relative_path_gets_expanded_into_absolute_path() -> crate::Result let cwd = canonicalized_tempdir()?; let dir = cwd.path(); let link_name = "pq_link"; - create_symlink(&dir.join("r").join(link_name), &Path::new("p").join("q"))?; + create_symlink(dir.join("r").join(link_name), Path::new("p").join("q"))?; assert_eq!( realpath_opts(Path::new(link_name).join(".git"), dir.join("r"), 8)?, dir.join("r").join("p").join("q").join(".git"), @@ -113,13 +113,10 @@ fn symlink_to_relative_path_gets_expanded_into_absolute_path() -> crate::Result fn symlink_processing_is_disabled_if_the_value_is_zero() -> crate::Result { let cwd = canonicalized_tempdir()?; let link_name = "x_link"; - create_symlink( - &cwd.path().join(link_name), - Path::new("link destination does not exist"), - )?; + create_symlink(cwd.path().join(link_name), Path::new("link destination does not exist"))?; assert!( matches!( - realpath_opts(&Path::new(link_name).join(".git"), &cwd, 0), + realpath_opts(Path::new(link_name).join(".git"), &cwd, 0), Err(Error::MaxSymlinksExceeded { max_symlinks: 0 }) ), "symlink processing is disabled if the value is zero" diff --git a/git-ref/src/store/file/loose/reflog/create_or_update/tests.rs b/git-ref/src/store/file/loose/reflog/create_or_update/tests.rs index baff1d98d61..de2d43757a0 100644 --- a/git-ref/src/store/file/loose/reflog/create_or_update/tests.rs +++ b/git-ref/src/store/file/loose/reflog/create_or_update/tests.rs @@ -111,7 +111,7 @@ fn missing_reflog_creates_it_even_if_similarly_named_empty_dir_exists_and_append let full_name: &FullNameRef = full_name_str.try_into()?; let reflog_path = store.reflog_path(full_name_str.try_into().expect("valid")); let directory_in_place_of_reflog = reflog_path.join("empty-a").join("empty-b"); - std::fs::create_dir_all(&directory_in_place_of_reflog)?; + std::fs::create_dir_all(directory_in_place_of_reflog)?; store.reflog_create_or_append( full_name, diff --git a/git-repository/src/create.rs b/git-repository/src/create.rs index 4f997fd3443..10f3d4bcec9 100644 --- a/git-repository/src/create.rs +++ b/git-repository/src/create.rs @@ -221,7 +221,7 @@ pub fn into( } let mut cursor = PathCursor(&mut dot_git); let config_path = cursor.at("config"); - std::fs::write(config_path, &config.to_bstring()).map_err(|err| Error::IoWrite { + std::fs::write(config_path, config.to_bstring()).map_err(|err| Error::IoWrite { source: err, path: config_path.to_owned(), })?; diff --git a/git-repository/src/repository/init.rs b/git-repository/src/repository/init.rs index aa52496103b..939c3f36558 100644 --- a/git-repository/src/repository/init.rs +++ b/git-repository/src/repository/init.rs @@ -34,7 +34,7 @@ fn setup_objects(mut objects: crate::OdbHandle, config: &crate::config::Cache) - #[cfg(feature = "max-performance-safe")] { match config.pack_cache_bytes { - None => objects.set_pack_cache(|| Box::new(git_pack::cache::lru::StaticLinkedList::<64>::default())), + None => objects.set_pack_cache(|| Box::>::default()), Some(0) => objects.unset_pack_cache(), Some(bytes) => objects.set_pack_cache(move || -> Box { Box::new(git_pack::cache::lru::MemoryCappedHashmap::new(bytes)) diff --git a/git-repository/src/repository/worktree.rs b/git-repository/src/repository/worktree.rs index 797e5b72442..b80cdf22c72 100644 --- a/git-repository/src/repository/worktree.rs +++ b/git-repository/src/repository/worktree.rs @@ -64,7 +64,7 @@ impl crate::Repository { .resolved .boolean("index", None, "threads") .map(|res| { - res.map(|value| if value { 0usize } else { 1 }).or_else(|err| { + res.map(|value| usize::from(!value)).or_else(|err| { git_config::Integer::try_from(err.input.as_ref()) .map_err(|err| worktree::open_index::Error::ConfigIndexThreads { value: err.input.clone(), diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index dfb0c1ed9f4..110df8016a4 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -250,7 +250,7 @@ pub fn scripted_fixture_writable_with_args( Ok(match mode { Creation::CopyFromReadOnly => { let ro_dir = scripted_fixture_read_only_with_args_inner(script_name, args, None)?; - copy_recursively_into_existing_dir(&ro_dir, dst.path())?; + copy_recursively_into_existing_dir(ro_dir, dst.path())?; dst } Creation::ExecuteScript => { @@ -518,7 +518,7 @@ fn populate_meta_dir(destination_dir: &Path, script_identity: u32) -> std::io::R )?; std::fs::write( meta_dir.join(META_GIT_VERSION), - &std::process::Command::new("git").arg("--version").output()?.stdout, + std::process::Command::new("git").arg("--version").output()?.stdout, )?; Ok(meta_dir) } From 680c1436d0a8cb1d2f602369120c5bc0e36815b1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 15 Dec 2022 21:09:18 +0100 Subject: [PATCH 15/15] try to fix clippy on CI as well. I don't know if the toolchain file works like that either, set a local override for now via `rustup override set 1.65`. --- .github/workflows/ci.yml | 2 +- rust-toolchain.tml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a543f827193..36ac16f6d2e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -120,7 +120,7 @@ jobs: - uses: actions/checkout@v3 - uses: dtolnay/rust-toolchain@master with: - toolchain: stable + toolchain: 1.65 # clippy is broken for us in 1.66 components: clippy,rustfmt - name: Run cargo clippy run: cargo clippy --all --tests diff --git a/rust-toolchain.tml b/rust-toolchain.tml index 292fe499e3b..d22c6829c69 100644 --- a/rust-toolchain.tml +++ b/rust-toolchain.tml @@ -1,2 +1,2 @@ [toolchain] -channel = "stable" +channel = "stable-1.65"