From 0699c7e81152f68d21c882e251d2ae1aec581b61 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 15:30:22 +0800 Subject: [PATCH 01/34] first test to validate new sort-by-date with cutoff sorting mode (breaking) Breaking indication in commit message has to happen later once things statbilize a bit more. --- git-traverse/src/commit.rs | 18 ++++++++++++++++-- git-traverse/tests/commit/mod.rs | 14 ++++++++++++++ ...raversal_repo_for_commits_with_dates.tar.xz | 4 ++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/git-traverse/src/commit.rs b/git-traverse/src/commit.rs index 796595937aa..47a4d8fd951 100644 --- a/git-traverse/src/commit.rs +++ b/git-traverse/src/commit.rs @@ -36,6 +36,14 @@ pub enum Sorting { /// This mode benefits greatly from having an object_cache in `find()` /// to avoid having to lookup each commit twice. ByCommitTimeNewestFirst, + /// This sorting is similar to `ByCommitTimeNewestFirst`, but adds a cutoff to not return commits older than + /// a given time, stopping the iteration once no younger commits is queued to be traversed. + /// + /// As the query is usually repeated with different cutoff dates, this search mode benefits greatly from an object cache. + ByCommitTimeNewestFirstCutoffOlderThan { + /// The amount of seconds since unix epoch, the same value obtained by any `git_date::Time` structure and the way git counts time. + time_in_seconds_since_epoch: u32, + }, } impl Default for Sorting { @@ -217,7 +225,10 @@ pub mod ancestors { } else { match self.sorting { Sorting::Topological => self.next_by_topology(), - Sorting::ByCommitTimeNewestFirst => self.next_by_commit_date(), + Sorting::ByCommitTimeNewestFirst => self.next_by_commit_date(None), + Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch, + } => self.next_by_commit_date(time_in_seconds_since_epoch.into()), } } } @@ -231,7 +242,10 @@ pub mod ancestors { StateMut: BorrowMut, E: std::error::Error + Send + Sync + 'static, { - fn next_by_commit_date(&mut self) -> Option> { + fn next_by_commit_date( + &mut self, + _cutoff_older_than: Option, + ) -> Option> { let state = self.state.borrow_mut(); let (oid, _commit_time) = state.next.pop_front()?; diff --git a/git-traverse/tests/commit/mod.rs b/git-traverse/tests/commit/mod.rs index 39b41e5df1b..b39665d40f8 100644 --- a/git-traverse/tests/commit/mod.rs +++ b/git-traverse/tests/commit/mod.rs @@ -222,6 +222,20 @@ mod ancestor { .check() } + #[test] + #[ignore] + fn committer_date_sorted_commits_with_cutoff() -> crate::Result { + TraversalAssertion::new( + "make_traversal_repo_for_commits_with_dates.sh", + &["288e509293165cb5630d08f4185bdf2445bf6170"], + &["bcb05040a6925f2ff5e10d3ae1f9264f2e8c43ac"], + ) + .with_sorting(commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch: 978393600, // =2001-01-02 00:00:00 +0000 + }) + .check() + } + #[test] fn committer_date_sorted_commits_parents_only() -> crate::Result { TraversalAssertion::new( diff --git a/git-traverse/tests/fixtures/generated-archives/make_traversal_repo_for_commits_with_dates.tar.xz b/git-traverse/tests/fixtures/generated-archives/make_traversal_repo_for_commits_with_dates.tar.xz index 5e9a4e24677..36ed2e1aaad 100644 --- a/git-traverse/tests/fixtures/generated-archives/make_traversal_repo_for_commits_with_dates.tar.xz +++ b/git-traverse/tests/fixtures/generated-archives/make_traversal_repo_for_commits_with_dates.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:91b1dd658c06390db23c8aa1fbf4942c83ed31e05eab07e0f7596c94a8a8fc07 -size 10496 +oid sha256:3f19abf0d05a12c2934f5b479a565afbf348cdaf9c05fb99d7b5c7d225eeb125 +size 10512 From 86a99a90eb992322099a870fbba10dddbcb80e83 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 18:39:04 +0800 Subject: [PATCH 02/34] feat: add `commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan(time)`.(#450) It allows to stop traversals early if all commmits to be traversed are older than a given time. --- git-traverse/src/commit.rs | 46 +++++++++++++++++++++++++------- git-traverse/tests/commit/mod.rs | 21 ++++++++++++++- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/git-traverse/src/commit.rs b/git-traverse/src/commit.rs index 47a4d8fd951..7b7c8b6f168 100644 --- a/git-traverse/src/commit.rs +++ b/git-traverse/src/commit.rs @@ -114,15 +114,26 @@ pub mod ancestors { pub fn sorting(mut self, sorting: Sorting) -> Result { self.sorting = sorting; if !matches!(self.sorting, Sorting::Topological) { + let mut cutoff_time_storage = self.sorting.cutoff_time().map(|cot| (cot, Vec::new())); let state = self.state.borrow_mut(); for (commit_id, commit_time) in state.next.iter_mut() { let commit_iter = (self.find)(commit_id, &mut state.buf).map_err(|err| Error::FindExisting { oid: *commit_id, source: err.into(), })?; - *commit_time = commit_iter.committer()?.time.seconds_since_unix_epoch; + let time = commit_iter.committer()?.time.seconds_since_unix_epoch; + match &mut cutoff_time_storage { + Some((cutoff_time, storage)) if time >= *cutoff_time => { + storage.push((*commit_id, time)); + } + Some(_) => {} + None => *commit_time = time, + } } - let mut v = Vec::from_iter(std::mem::take(&mut state.next).into_iter()); + let mut v = match cutoff_time_storage { + Some((_, storage)) => storage, + None => Vec::from_iter(std::mem::take(&mut state.next).into_iter()), + }; v.sort_by(|a, b| a.1.cmp(&b.1).reverse()); state.next = v.into(); } @@ -234,6 +245,18 @@ pub mod ancestors { } } + impl Sorting { + /// If not topo sort, provide the cutoff date if present. + fn cutoff_time(&self) -> Option { + match self { + Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch, + } => Some(*time_in_seconds_since_epoch), + _ => None, + } + } + } + /// Utilities impl Ancestors where @@ -242,10 +265,7 @@ pub mod ancestors { StateMut: BorrowMut, E: std::error::Error + Send + Sync + 'static, { - fn next_by_commit_date( - &mut self, - _cutoff_older_than: Option, - ) -> Option> { + fn next_by_commit_date(&mut self, cutoff_older_than: Option) -> Option> { let state = self.state.borrow_mut(); let (oid, _commit_time) = state.next.pop_front()?; @@ -277,9 +297,17 @@ pub mod ancestors { }) .unwrap_or_default(); - match state.next.binary_search_by(|c| c.1.cmp(&parent_commit_time).reverse()) { - Ok(_) => state.next.push_back((id, parent_commit_time)), // collision => topo-sort - Err(pos) => state.next.insert(pos, (id, parent_commit_time)), // otherwise insert by commit-time + let pos = match state.next.binary_search_by(|c| c.1.cmp(&parent_commit_time).reverse()) + { + Ok(_) => None, + Err(pos) => Some(pos), + }; + match cutoff_older_than { + Some(cutoff_older_than) if parent_commit_time < cutoff_older_than => continue, + Some(_) | None => match pos { + Some(pos) => state.next.insert(pos, (id, parent_commit_time)), + None => state.next.push_back((id, parent_commit_time)), + }, } if is_first && matches!(self.parents, Parents::First) { diff --git a/git-traverse/tests/commit/mod.rs b/git-traverse/tests/commit/mod.rs index b39665d40f8..97292b79944 100644 --- a/git-traverse/tests/commit/mod.rs +++ b/git-traverse/tests/commit/mod.rs @@ -223,7 +223,6 @@ mod ancestor { } #[test] - #[ignore] fn committer_date_sorted_commits_with_cutoff() -> crate::Result { TraversalAssertion::new( "make_traversal_repo_for_commits_with_dates.sh", @@ -236,6 +235,26 @@ mod ancestor { .check() } + #[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 store = git_odb::at(dir.join(".git").join("objects"))?; + let iter = commit::Ancestors::new( + Some(hex_to_id("9902e3c3e8f0c569b4ab295ddf473e6de763e1e7")), + commit::ancestors::State::default(), + move |oid, buf| store.find_commit_iter(oid, buf).map(|t| t.0), + ) + .sorting(commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch: 978393600, // =2001-01-02 00:00:00 +0000 + })?; + assert_eq!( + iter.count(), + 0, + "initial tips that don't pass cutoff value are not returned either" + ); + Ok(()) + } + #[test] fn committer_date_sorted_commits_parents_only() -> crate::Result { TraversalAssertion::new( From 2d0d782a59a90021e388b2c55354e9b3472d9cd3 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 19:44:37 +0800 Subject: [PATCH 03/34] first non-fastforward tests in dry-run mode (#450) The implementation for non-dry-run exists as well, but the test doesn't quite work yet. --- .../connection/fetch/update_refs/mod.rs | 35 ++++++++++++++- .../connection/fetch/update_refs/tests.rs | 43 +++++++++++++++---- .../connection/fetch/update_refs/update.rs | 2 + 3 files changed, 71 insertions(+), 9 deletions(-) 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 9cfbd71aaae..4dc02ec7150 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -1,6 +1,8 @@ +use crate::ext::ObjectIdExt; use crate::remote::fetch; use crate::remote::fetch::refs::update::Mode; use crate::Repository; +use git_odb::FindExt; use git_pack::Find; use git_ref::transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog}; use git_ref::{Target, TargetRef}; @@ -85,7 +87,38 @@ pub(crate) fn update( updates.push(update::Mode::RejectedTagUpdate.into()); continue; } else { - todo!("check for fast-forward (is local an ancestor of remote?)") + match dry_run { + fetch::DryRun::No => match repo + .find_object(local_id)? + .try_into_commit() + .map_err(|_| ()) + .and_then(|c| { + c.committer().map(|a| a.time.seconds_since_unix_epoch).map_err(|_| ()) + }).and_then(|local_commit_time| + remote_id + .to_owned() + .ancestors(|id, buf| repo.objects.find_commit_iter(id, buf)) + .sorting( + git_traverse::commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch: local_commit_time + }, + ) + .map_err(|_| ()) + ) { + Ok(mut ancestors) => { + if ancestors.any(|cid| cid.map_or(false, |cid| cid == local_id)) { + (update::Mode::FastForward, "fast-forward") + } else { + updates.push(update::Mode::RejectedNonFastForward.into()); + continue; + } + } + Err(_) => (update::Mode::Forced, "forced-update"), + }, + fetch::DryRun::Yes => { + (update::Mode::FastForward, "fast-forward (guessed in dry-run)") + } + } }; (mode, reflog_message, existing.name().to_owned()) } 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 ddbba5f11f9..10e0f83e239 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/tests.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/tests.rs @@ -18,6 +18,16 @@ mod update { .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( + "make_fetch_repos.sh", + [base_repo_path()], + git_testtools::Creation::ExecuteScript, + ) + .unwrap(); + let repo = git::open_opts(dir.path().join(name), git::open::Options::isolated()).unwrap(); + (repo, dir) + } use crate::remote::fetch; use git_ref::transaction::Change; use git_ref::TargetRef; @@ -97,12 +107,12 @@ mod update { None, "checked out branches cannot be written, as it requires a merge of sorts which isn't done here", ), - // ( // TODO: make fast-forwards work - // "refs/remotes/origin/g:refs/heads/not-currently-checked-out", - // fetch::refs::update::Mode::FastForward, - // true, - // "a fast-forward only fast-forward situation, all good", - // ), + ( + "refs/remotes/origin/g:refs/heads/not-currently-checked-out", + fetch::refs::update::Mode::FastForward, + Some("fast-forward (guessed in dry-run)"), + "a fast-forward only fast-forward situation, all good", + ), ] { let (mapping, specs) = mapping_from_spec(spec, &repo); let out = fetch::refs::update( @@ -193,12 +203,29 @@ mod update { } #[test] - #[should_panic] - fn fast_forward_is_not_implemented_yet_but_should_be_denied() { + fn non_fast_forward_is_rejected_but_appears_to_be_fast_forward_in_dryrun_mode() { let repo = repo("two-origins"); let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/remotes/origin/g", &repo); let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::Yes).unwrap(); + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::FastForward, + edit_index: Some(0), + }], + "The caller has to be aware and note that dry-runs can't know about fast-forwards as they don't have remote objects" + ); + assert_eq!(out.edits.len(), 1); + } + + #[test] + #[ignore] + fn non_fast_forward_is_rejected_if_dry_run_is_disabled() { + let (repo, _tmp) = repo_rw("two-origins"); + let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/remotes/origin/g", &repo); + let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::No).unwrap(); + assert_eq!( out.updates, vec![fetch::refs::Update { 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 a0658f934be..67a4b39d628 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/update.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/update.rs @@ -16,6 +16,8 @@ mod error { WorktreeListing(#[from] std::io::Error), #[error("Could not open worktree repository")] OpenWorktreeRepo(#[from] crate::open::Error), + #[error("Could not find local commit for fast-forward ancestor check")] + FindCommit(#[from] crate::object::find::existing::Error), } } From e3b937ef35839402498988af322f943e4745178f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 19:55:57 +0800 Subject: [PATCH 04/34] add test to validate actual fast-forwards, without dry-run (#450) --- .../connection/fetch/update_refs/mod.rs | 15 ++++++--- .../connection/fetch/update_refs/tests.rs | 33 ++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) 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 4dc02ec7150..3a7b519e098 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -60,7 +60,7 @@ pub(crate) fn update( let checked_out_branches = worktree_branches(repo)?; let (mode, edit_index) = match local { Some(name) => { - let (mode, reflog_message, name) = match repo.try_find_reference(name)? { + let (mode, reflog_message, name, previous_value) = match repo.try_find_reference(name)? { Some(existing) => { if let Some(wt_dir) = checked_out_branches.get(existing.name()) { let mode = update::Mode::RejectedCurrentlyCheckedOut { @@ -75,6 +75,8 @@ pub(crate) fn update( continue; } TargetRef::Peeled(local_id) => { + let previous_value = + PreviousValue::MustExistAndMatch(Target::Peeled(local_id.to_owned())); let (mode, reflog_message) = if local_id == remote_id { (update::Mode::NoChangeNeeded, "no update will be performed") } else if refspecs[*spec_index].allow_non_fast_forward() { @@ -120,7 +122,7 @@ pub(crate) fn update( } } }; - (mode, reflog_message, existing.name().to_owned()) + (mode, reflog_message, existing.name().to_owned(), previous_value) } } } @@ -131,7 +133,12 @@ pub(crate) fn update( Some(git_ref::Category::LocalBranch) => "storing head", _ => "storing ref", }; - (update::Mode::New, reflog_msg, name) + ( + update::Mode::New, + reflog_msg, + name, + PreviousValue::ExistingMustMatch(Target::Peeled(remote_id.to_owned())), + ) } }; let edit = RefEdit { @@ -141,7 +148,7 @@ pub(crate) fn update( force_create_reflog: false, message: format!("{}: {}", action, reflog_message).into(), }, - expected: PreviousValue::ExistingMustMatch(Target::Peeled(remote_id.into())), + expected: previous_value, new: Target::Peeled(remote_id.into()), }, name, 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 10e0f83e239..0d8d1c8e63a 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/tests.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/tests.rs @@ -35,7 +35,6 @@ mod update { #[test] fn various_valid_updates() { let repo = repo("two-origins"); - // TODO: test reflog message (various cases if it's new) for (spec, expected_mode, reflog_message, detail) in [ ( "refs/heads/main:refs/remotes/origin/main", @@ -136,13 +135,21 @@ mod update { if let Some(reflog_message) = reflog_message { let edit = &out.edits[0]; match &edit.change { - Change::Update { log, .. } => { + Change::Update { log, new, .. } => { assert_eq!( log.message, format!("action: {}", reflog_message), "{}: reflog messages are specific and we emulate git word for word", spec ); + let remote_ref = repo + .find_reference(specs[0].to_ref().source().expect("always present")) + .unwrap(); + assert_eq!( + new.id(), + remote_ref.target().id(), + "remote ref provides the id to set in the local reference" + ) } _ => unreachable!("only updates"), } @@ -220,20 +227,38 @@ mod update { } #[test] - #[ignore] fn non_fast_forward_is_rejected_if_dry_run_is_disabled() { let (repo, _tmp) = repo_rw("two-origins"); - let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/remotes/origin/g", &repo); + let (mappings, specs) = mapping_from_spec("refs/remotes/origin/g:refs/heads/not-currently-checked-out", &repo); let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::No).unwrap(); assert_eq!( out.updates, vec![fetch::refs::Update { mode: fetch::refs::update::Mode::RejectedNonFastForward, + edit_index: None, + }] + ); + assert_eq!(out.edits.len(), 0); + + let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/remotes/origin/g", &repo); + let out = fetch::refs::update(&repo, "prefix", &mappings, &specs, fetch::DryRun::No).unwrap(); + + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::FastForward, edit_index: Some(0), }] ); assert_eq!(out.edits.len(), 1); + let edit = &out.edits[0]; + match &edit.change { + Change::Update { log, .. } => { + assert_eq!(log.message, format!("prefix: {}", "fast-forward")); + } + _ => unreachable!("only updates"), + } } fn mapping_from_spec(spec: &str, repo: &git::Repository) -> (Vec, Vec) { From 8334148c63bfdb10c7388e4ce6480070e39cfa9f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 20:39:07 +0800 Subject: [PATCH 05/34] refactor (#450) --- .../connection/fetch/update_refs/mod.rs | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) 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 3a7b519e098..e9f986ff81b 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -90,33 +90,36 @@ pub(crate) fn update( continue; } else { match dry_run { - fetch::DryRun::No => match repo - .find_object(local_id)? - .try_into_commit() - .map_err(|_| ()) - .and_then(|c| { - c.committer().map(|a| a.time.seconds_since_unix_epoch).map_err(|_| ()) - }).and_then(|local_commit_time| - remote_id - .to_owned() - .ancestors(|id, buf| repo.objects.find_commit_iter(id, buf)) - .sorting( - git_traverse::commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { - time_in_seconds_since_epoch: local_commit_time - }, - ) - .map_err(|_| ()) - ) { - Ok(mut ancestors) => { - if ancestors.any(|cid| cid.map_or(false, |cid| cid == local_id)) { - (update::Mode::FastForward, "fast-forward") - } else { - updates.push(update::Mode::RejectedNonFastForward.into()); - continue; + fetch::DryRun::No => { + let ancestors = repo + .find_object(local_id)? + .try_into_commit() + .map_err(|_| ()) + .and_then(|c| { + c.committer().map(|a| a.time.seconds_since_unix_epoch).map_err(|_| ()) + }).and_then(|local_commit_time| + remote_id + .to_owned() + .ancestors(|id, buf| repo.objects.find_commit_iter(id, buf)) + .sorting( + git_traverse::commit::Sorting::ByCommitTimeNewestFirstCutoffOlderThan { + time_in_seconds_since_epoch: local_commit_time + }, + ) + .map_err(|_| ()) + ); + match ancestors { + Ok(mut ancestors) => { + if ancestors.any(|cid| cid.map_or(false, |cid| cid == local_id)) { + (update::Mode::FastForward, "fast-forward") + } else { + updates.push(update::Mode::RejectedNonFastForward.into()); + continue; + } } + Err(_) => (update::Mode::Forced, "forced-update"), } - Err(_) => (update::Mode::Forced, "forced-update"), - }, + } fetch::DryRun::Yes => { (update::Mode::FastForward, "fast-forward (guessed in dry-run)") } From c0d3ced850a1a369e1c562e172266d78b0bc6698 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 20:52:27 +0800 Subject: [PATCH 06/34] refactor (#450) prepare for making fast-forward calculations even if force is specified. --- .../connection/fetch/update_refs/mod.rs | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) 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 e9f986ff81b..e0117b7b1ec 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -89,7 +89,8 @@ pub(crate) fn update( updates.push(update::Mode::RejectedTagUpdate.into()); continue; } else { - match dry_run { + let mut force = refspecs[*spec_index].allow_non_fast_forward(); + let is_fast_forward = match dry_run { fetch::DryRun::No => { let ancestors = repo .find_object(local_id)? @@ -110,18 +111,29 @@ pub(crate) fn update( ); match ancestors { Ok(mut ancestors) => { - if ancestors.any(|cid| cid.map_or(false, |cid| cid == local_id)) { - (update::Mode::FastForward, "fast-forward") - } else { - updates.push(update::Mode::RejectedNonFastForward.into()); - continue; - } + ancestors.any(|cid| cid.map_or(false, |cid| cid == local_id)) + } + Err(_) => { + force = true; + false } - Err(_) => (update::Mode::Forced, "forced-update"), } } - fetch::DryRun::Yes => { - (update::Mode::FastForward, "fast-forward (guessed in dry-run)") + fetch::DryRun::Yes => true, + }; + if is_fast_forward { + ( + update::Mode::FastForward, + matches!(dry_run, fetch::DryRun::Yes) + .then(|| "fast-forward (guessed in dry-run)") + .unwrap_or("fast-forward"), + ) + } else { + if force { + (update::Mode::Forced, "forced-update") + } else { + updates.push(update::Mode::RejectedNonFastForward.into()); + continue; } } }; From a34370c1aea028cd9d6b839f136a16a11b32b804 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 20:58:47 +0800 Subject: [PATCH 07/34] always perform fast-forward checks even if force is specified. (#450) That way we emulate gits behaviour, which does the same unless it's turned off. The latter we don't allow yet as it should really be fast enough due to the date-cutoff during traversal. --- .../connection/fetch/update_refs/mod.rs | 14 ++++----- .../connection/fetch/update_refs/tests.rs | 29 +++++++++++++++++-- 2 files changed, 32 insertions(+), 11 deletions(-) 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 e0117b7b1ec..6553ae1b1c4 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -79,15 +79,13 @@ pub(crate) fn update( PreviousValue::MustExistAndMatch(Target::Peeled(local_id.to_owned())); let (mode, reflog_message) = if local_id == remote_id { (update::Mode::NoChangeNeeded, "no update will be performed") - } else if refspecs[*spec_index].allow_non_fast_forward() { - let reflog_msg = match existing.name().category() { - Some(git_ref::Category::Tag) => "updating tag", - _ => "forced-update", - }; - (update::Mode::Forced, reflog_msg) } else if let Some(git_ref::Category::Tag) = existing.name().category() { - updates.push(update::Mode::RejectedTagUpdate.into()); - continue; + if refspecs[*spec_index].allow_non_fast_forward() { + (update::Mode::Forced, "updating tag") + } else { + updates.push(update::Mode::RejectedTagUpdate.into()); + continue; + } } else { let mut force = refspecs[*spec_index].allow_non_fast_forward(); let is_fast_forward = match dry_run { 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 0d8d1c8e63a..8ef68b5f448 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/tests.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/tests.rs @@ -74,9 +74,9 @@ mod update { ), ( "+refs/heads/main:refs/remotes/origin/g", - fetch::refs::update::Mode::Forced, - Some("forced-update"), - "a forced non-fastforward (main goes backwards)", + fetch::refs::update::Mode::FastForward, + Some("fast-forward (guessed in dry-run)"), + "a forced non-fastforward (main goes backwards), but dry-run calls it fast-forward", ), ( "+refs/heads/main:refs/tags/b-tag", @@ -261,6 +261,29 @@ mod update { } } + #[test] + fn fast_forwards_are_called_out_even_if_force_is_given() { + let (repo, _tmp) = repo_rw("two-origins"); + let (mappings, specs) = mapping_from_spec("+refs/heads/main:refs/remotes/origin/g", &repo); + let out = fetch::refs::update(&repo, "prefix", &mappings, &specs, fetch::DryRun::No).unwrap(); + + assert_eq!( + out.updates, + vec![fetch::refs::Update { + mode: fetch::refs::update::Mode::FastForward, + edit_index: Some(0), + }] + ); + assert_eq!(out.edits.len(), 1); + let edit = &out.edits[0]; + match &edit.change { + Change::Update { log, .. } => { + assert_eq!(log.message, format!("prefix: {}", "fast-forward")); + } + _ => unreachable!("only updates"), + } + } + fn mapping_from_spec(spec: &str, repo: &git::Repository) -> (Vec, Vec) { let spec = git_refspec::parse(spec.into(), git_refspec::parse::Operation::Fetch).unwrap(); let group = git_refspec::MatchGroup::from_fetch_specs(Some(spec)); From 83f21565bbbc43442410be3b63030ea2537e6d2c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 1 Oct 2022 21:01:06 +0800 Subject: [PATCH 08/34] thanks clippy --- .../src/remote/connection/fetch/update_refs/mod.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 6553ae1b1c4..72c2ea96cb7 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/mod.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/mod.rs @@ -126,13 +126,11 @@ pub(crate) fn update( .then(|| "fast-forward (guessed in dry-run)") .unwrap_or("fast-forward"), ) + } else if force { + (update::Mode::Forced, "forced-update") } else { - if force { - (update::Mode::Forced, "forced-update") - } else { - updates.push(update::Mode::RejectedNonFastForward.into()); - continue; - } + updates.push(update::Mode::RejectedNonFastForward.into()); + continue; } }; (mode, reflog_message, existing.name().to_owned(), previous_value) From 5b72d2708889dc388facd9cbc61e5bfa5403e003 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 14:55:51 +0800 Subject: [PATCH 09/34] Frame for `gix fetch` (#450) --- gitoxide-core/src/repository/fetch.rs | 34 +++++++++++++++++++++++++++ gitoxide-core/src/repository/mod.rs | 2 ++ src/plumbing/main.rs | 24 +++++++++++++++++++ src/plumbing/options/mod.rs | 25 ++++++++++++++++++++ 4 files changed, 85 insertions(+) create mode 100644 gitoxide-core/src/repository/fetch.rs diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs new file mode 100644 index 00000000000..163b1df6a2e --- /dev/null +++ b/gitoxide-core/src/repository/fetch.rs @@ -0,0 +1,34 @@ +use crate::OutputFormat; +use git::bstr::BString; +use git_repository as git; + +pub struct Options { + pub format: OutputFormat, + pub dry_run: bool, + pub remote: Option, + /// If non-empty, override all ref-specs otherwise configured in the remote + pub ref_specs: Vec, +} + +pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=2; + +pub(crate) mod function { + #![allow(unused_variables, unused_mut)] + use super::Options; + use git_repository as git; + + pub fn fetch( + repo: git::Repository, + mut progress: impl git::Progress, + mut out: impl std::io::Write, + err: impl std::io::Write, + Options { + format, + dry_run, + remote, + ref_specs, + }: Options, + ) -> anyhow::Result<()> { + todo!() + } +} diff --git a/gitoxide-core/src/repository/mod.rs b/gitoxide-core/src/repository/mod.rs index 8e170f7f066..ef57ce26ab2 100644 --- a/gitoxide-core/src/repository/mod.rs +++ b/gitoxide-core/src/repository/mod.rs @@ -19,6 +19,8 @@ pub mod config; mod credential; pub use credential::function as credential; pub mod exclude; +pub mod fetch; +pub use fetch::function::fetch; pub mod mailmap; pub mod odb; pub mod remote; diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index cdebe387dab..9759f27e27d 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -13,6 +13,7 @@ use git_repository::bstr::io::BufReadExt; use gitoxide_core as core; use gitoxide_core::pack::verify; +use crate::plumbing::options::fetch; use crate::{ plumbing::{ options::{commit, config, credential, exclude, free, mailmap, odb, remote, revision, tree, Args, Subcommands}, @@ -112,6 +113,29 @@ pub fn main() -> Result<()> { })?; match cmd { + #[cfg(feature = "gitoxide-core-blocking-client")] + Subcommands::Fetch(fetch::Platform { + dry_run, + remote, + ref_spec, + }) => { + let opts = core::repository::fetch::Options { + format, + dry_run, + remote, + ref_specs: ref_spec, + }; + prepare_and_run( + "fetch", + verbose, + progress, + progress_keep_open, + core::repository::fetch::PROGRESS_RANGE, + move |progress, out, err| { + core::repository::fetch(repository(Mode::LenientWithGitInstallConfig)?, progress, out, err, opts) + }, + ) + } Subcommands::Progress => show_progress(), Subcommands::Credential(cmd) => core::repository::credential( repository(Mode::StrictWithGitInstallConfig)?, diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index b7c3ea58db8..5f3e995938c 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -81,6 +81,9 @@ pub enum Subcommands { /// A program just like `git credential`. #[clap(subcommand)] Credential(credential::Subcommands), + /// Fetch data from remotes and store it in the repository + #[cfg(feature = "gitoxide-core-blocking-client")] + Fetch(fetch::Platform), /// Interact with the mailmap. #[clap(subcommand)] Mailmap(mailmap::Subcommands), @@ -110,6 +113,28 @@ pub mod config { } } +#[cfg(feature = "gitoxide-core-blocking-client")] +pub mod fetch { + use git_repository as git; + + #[derive(Debug, clap::Parser)] + pub struct Platform { + /// Don't change the local repository, but otherwise try to be as accurate as possible. + #[clap(long, short = 'n')] + pub dry_run: bool, + + /// The name of the remote to connect to. + /// + /// If unset, the current branch will determine the remote. + #[clap(long, short = 'r')] + pub remote: Option, + + /// Override the built-in and configured ref-specs with one or more of the given ones. + #[clap(parse(try_from_os_str = git::env::os_str_to_bstring))] + pub ref_spec: Vec, + } +} + pub mod remote { use git_repository as git; From 8c7351c4c50517b3ccc3479f2a7a020bc607bf24 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 15:05:39 +0800 Subject: [PATCH 10/34] support for `--url` for arbitrary urls when fetching (#450) Also known as 'anonymous remotes'. --- README.md | 1 + gitoxide-core/src/repository/fetch.rs | 3 +++ gitoxide-core/src/repository/remote.rs | 33 +++++++++++++++++--------- src/plumbing/main.rs | 2 ++ src/plumbing/options/mod.rs | 4 ++++ 5 files changed, 32 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6b6b6ac72d0..0e66e28b1f4 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,7 @@ Please see _'Development Status'_ for a listing of all crates and their capabili * **remote** * [x] **refs** - list all references available on the remote based on the current remote configuration. * [x] **ref-map** - show how remote references relate to their local tracking branches as mapped by refspecs. + * **fetch** - fetch the current remote or the given one, optionally just as dry-run. * **credential** * [x] **fill/approve/reject** - The same as `git credential`, but implemented in Rust, calling helpers only when from trusted configuration. * **free** - no git repository necessary diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 163b1df6a2e..8a74c53002f 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -6,6 +6,7 @@ pub struct Options { pub format: OutputFormat, pub dry_run: bool, pub remote: Option, + pub url: Option, /// If non-empty, override all ref-specs otherwise configured in the remote pub ref_specs: Vec, } @@ -26,9 +27,11 @@ pub(crate) mod function { format, dry_run, remote, + url, ref_specs, }: Options, ) -> anyhow::Result<()> { + let _remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref(), url)?; todo!() } } diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 39ee2ee0abc..e8f716472d1 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -1,5 +1,7 @@ #[cfg(any(feature = "blocking-client", feature = "async-client"))] mod refs_impl { + use super::by_name_or_url; + use crate::OutputFormat; use anyhow::bail; use git_repository as git; use git_repository::{ @@ -7,8 +9,6 @@ mod refs_impl { refspec::{match_group::validate::Fix, RefSpec}, }; - use crate::OutputFormat; - pub mod refs { use git_repository::bstr::BString; @@ -46,15 +46,7 @@ mod refs_impl { }: refs::Context, ) -> anyhow::Result<()> { use anyhow::Context; - let mut remote = match (name, url) { - (Some(name), None) => repo.find_remote(&name)?, - (None, None) => repo - .head()? - .into_remote(git::remote::Direction::Fetch) - .context("Cannot find a remote for unborn branch")??, - (None, Some(url)) => repo.remote_at(url)?, - (Some(_), Some(_)) => bail!("Must not set both the remote name and the url - they are mutually exclusive"), - }; + let mut remote = by_name_or_url(&repo, name.as_deref(), url)?; if let refs::Kind::Tracking { ref_specs, .. } = &kind { if format != OutputFormat::Human { bail!("JSON output isn't yet supported for listing ref-mappings."); @@ -258,3 +250,22 @@ mod refs_impl { } #[cfg(any(feature = "blocking-client", feature = "async-client"))] pub use refs_impl::{refs, refs_fn as refs, JsonRef}; + +use git_repository as git; + +pub(crate) fn by_name_or_url<'repo>( + repo: &'repo git::Repository, + name: Option<&str>, + url: Option, +) -> anyhow::Result> { + use anyhow::{bail, Context}; + Ok(match (name, url) { + (Some(name), None) => repo.find_remote(&name)?, + (None, None) => repo + .head()? + .into_remote(git::remote::Direction::Fetch) + .context("Cannot find a remote for unborn branch")??, + (None, Some(url)) => repo.remote_at(url)?, + (Some(_), Some(_)) => bail!("Must not set both the remote name and the url - they are mutually exclusive"), + }) +} diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 9759f27e27d..0f8302fd7c6 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -118,12 +118,14 @@ pub fn main() -> Result<()> { dry_run, remote, ref_spec, + url, }) => { let opts = core::repository::fetch::Options { format, dry_run, remote, ref_specs: ref_spec, + url, }; prepare_and_run( "fetch", diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 5f3e995938c..40b605d3f61 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -129,6 +129,10 @@ pub mod fetch { #[clap(long, short = 'r')] pub remote: Option, + /// Connect directly to the given URL, forgoing any configuration from the repository. + #[clap(long, short = 'u', conflicts_with("remote"), parse(try_from_os_str = std::convert::TryFrom::try_from))] + pub url: Option, + /// Override the built-in and configured ref-specs with one or more of the given ones. #[clap(parse(try_from_os_str = git::env::os_str_to_bstring))] pub ref_spec: Vec, From 92bbe335688e4c8e96061663e71a599022f7b96f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 15:29:54 +0800 Subject: [PATCH 11/34] feat!: remove `gix remote --url` in favor of determining the intention similar to `git fetch` (#450) --- gitoxide-core/src/repository/fetch.rs | 4 +--- gitoxide-core/src/repository/remote.rs | 33 +++++++++++++------------- src/plumbing/main.rs | 8 ++----- src/plumbing/options/mod.rs | 12 ++-------- 4 files changed, 22 insertions(+), 35 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 8a74c53002f..7eed45e950e 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -6,7 +6,6 @@ pub struct Options { pub format: OutputFormat, pub dry_run: bool, pub remote: Option, - pub url: Option, /// If non-empty, override all ref-specs otherwise configured in the remote pub ref_specs: Vec, } @@ -27,11 +26,10 @@ pub(crate) mod function { format, dry_run, remote, - url, ref_specs, }: Options, ) -> anyhow::Result<()> { - let _remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref(), url)?; + let _remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref())?; todo!() } } diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index e8f716472d1..f895cecc707 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -21,10 +21,9 @@ mod refs_impl { Tracking { ref_specs: Vec }, } - pub struct Context { + pub struct Options { pub format: OutputFormat, - pub name: Option, - pub url: Option, + pub name_or_url: Option, pub handshake_info: bool, } @@ -38,15 +37,14 @@ mod refs_impl { mut progress: impl git::Progress, mut out: impl std::io::Write, err: impl std::io::Write, - refs::Context { + refs::Options { format, - name, - url, + name_or_url, handshake_info, - }: refs::Context, + }: refs::Options, ) -> anyhow::Result<()> { use anyhow::Context; - let mut remote = by_name_or_url(&repo, name.as_deref(), url)?; + let mut remote = by_name_or_url(&repo, name_or_url.as_deref())?; if let refs::Kind::Tracking { ref_specs, .. } = &kind { if format != OutputFormat::Human { bail!("JSON output isn't yet supported for listing ref-mappings."); @@ -255,17 +253,20 @@ use git_repository as git; pub(crate) fn by_name_or_url<'repo>( repo: &'repo git::Repository, - name: Option<&str>, - url: Option, + name_or_url: Option<&str>, ) -> anyhow::Result> { - use anyhow::{bail, Context}; - Ok(match (name, url) { - (Some(name), None) => repo.find_remote(&name)?, - (None, None) => repo + use anyhow::Context; + Ok(match name_or_url { + Some(name) => { + if name.contains('/') { + repo.remote_at(git::url::parse(name.into())?)? + } else { + repo.find_remote(&name)? + } + } + None => repo .head()? .into_remote(git::remote::Direction::Fetch) .context("Cannot find a remote for unborn branch")??, - (None, Some(url)) => repo.remote_at(url)?, - (Some(_), Some(_)) => bail!("Must not set both the remote name and the url - they are mutually exclusive"), }) } diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 0f8302fd7c6..6c8404cc2bb 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -118,14 +118,12 @@ pub fn main() -> Result<()> { dry_run, remote, ref_spec, - url, }) => { let opts = core::repository::fetch::Options { format, dry_run, remote, ref_specs: ref_spec, - url, }; prepare_and_run( "fetch", @@ -150,7 +148,6 @@ pub fn main() -> Result<()> { #[cfg_attr(feature = "small", allow(unused_variables))] Subcommands::Remote(remote::Platform { name, - url, cmd, handshake_info, }) => match cmd { @@ -162,9 +159,8 @@ pub fn main() -> Result<()> { core::repository::remote::refs::Kind::Tracking { ref_specs: ref_spec } } }; - let context = core::repository::remote::refs::Context { - name, - url, + let context = core::repository::remote::refs::Options { + name_or_url: name, format, handshake_info, }; diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 40b605d3f61..7207af6f2dd 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -123,16 +123,12 @@ pub mod fetch { #[clap(long, short = 'n')] pub dry_run: bool, - /// The name of the remote to connect to. + /// The name of the remote to connect to, or the url of the remote to connect to directly. /// /// If unset, the current branch will determine the remote. #[clap(long, short = 'r')] pub remote: Option, - /// Connect directly to the given URL, forgoing any configuration from the repository. - #[clap(long, short = 'u', conflicts_with("remote"), parse(try_from_os_str = std::convert::TryFrom::try_from))] - pub url: Option, - /// Override the built-in and configured ref-specs with one or more of the given ones. #[clap(parse(try_from_os_str = git::env::os_str_to_bstring))] pub ref_spec: Vec, @@ -144,16 +140,12 @@ pub mod remote { #[derive(Debug, clap::Parser)] pub struct Platform { - /// The name of the remote to connect to. + /// The name of the remote to connect to, or the URL of the remote to connect to directly. /// /// If unset, the current branch will determine the remote. #[clap(long, short = 'n')] pub name: Option, - /// Connect directly to the given URL, forgoing any configuration from the repository. - #[clap(long, short = 'u', conflicts_with("name"), parse(try_from_os_str = std::convert::TryFrom::try_from))] - pub url: Option, - /// Output additional typically information provided by the server as part of the connection handshake. #[clap(long)] pub handshake_info: bool, From ac17e11ce95ee6ef8fa8d89fd57534be5a8aeda0 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:06:27 +0800 Subject: [PATCH 12/34] a sketch of how fetching could work, but it fails with the line-reader for some reason (#450) --- gitoxide-core/src/repository/fetch.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 7eed45e950e..fe31a078463 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -14,7 +14,10 @@ pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=2; pub(crate) mod function { #![allow(unused_variables, unused_mut)] + use super::Options; + use crate::OutputFormat; + use anyhow::bail; use git_repository as git; pub fn fetch( @@ -29,7 +32,21 @@ pub(crate) mod function { ref_specs, }: Options, ) -> anyhow::Result<()> { - let _remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref())?; - todo!() + if format != OutputFormat::Human { + bail!("JSON output isn't yet supported for fetching."); + } + 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)?; + } + + let res = remote + .connect(git::remote::Direction::Fetch, progress)? + .prepare_fetch(Default::default())? + .with_dry_run(dry_run) + .receive(&git::interrupt::IS_INTERRUPTED)?; + + dbg!(res); + Ok(()) } } From 266395e18d7cf754f51355098c84fd1c969fc972 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:19:53 +0800 Subject: [PATCH 13/34] fix: Link up Io error as source for error chaining; add `Debug` to `client::fetch::Response` (#450) --- git-protocol/src/fetch/response/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-protocol/src/fetch/response/mod.rs b/git-protocol/src/fetch/response/mod.rs index c9afce53470..0b7011ab112 100644 --- a/git-protocol/src/fetch/response/mod.rs +++ b/git-protocol/src/fetch/response/mod.rs @@ -8,7 +8,7 @@ use crate::fetch::command::Feature; #[allow(missing_docs)] pub enum Error { #[error("Failed to read from line reader")] - Io(std::io::Error), + Io(#[source] std::io::Error), #[error(transparent)] UploadPack(#[from] git_transport::packetline::read::Error), #[error(transparent)] @@ -139,6 +139,7 @@ impl WantedRef { } /// A representation of a complete fetch response +#[derive(Debug)] pub struct Response { acks: Vec, shallows: Vec, From df36ede64501fab7d6161ac1961da9d4db9e5ecc Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:46:08 +0800 Subject: [PATCH 14/34] properly implement `fetch::Arguments::is_empty()` (#450) Unfortunately its test is in `git-repository`, but better than nothing. --- git-protocol/src/fetch/arguments/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-protocol/src/fetch/arguments/mod.rs b/git-protocol/src/fetch/arguments/mod.rs index 6a0ebda438e..64c3ae72389 100644 --- a/git-protocol/src/fetch/arguments/mod.rs +++ b/git-protocol/src/fetch/arguments/mod.rs @@ -1,6 +1,6 @@ use std::fmt; -use bstr::{BStr, BString, ByteVec}; +use bstr::{BStr, BString, ByteSlice, ByteVec}; /// The arguments passed to a server command. #[derive(Debug)] @@ -30,7 +30,7 @@ impl Arguments { /// This can happen if callers assure that they won't add 'wants' if their 'have' is the same, i.e. if the remote has nothing /// new for them. pub fn is_empty(&self) -> bool { - self.args.is_empty() + self.haves.is_empty() && !self.args.iter().rev().any(|arg| arg.starts_with_str("want ")) } /// Return true if ref filters is supported. pub fn can_use_filter(&self) -> bool { From 8e9e0b20840e0a2479525e79d3e63071d20859c8 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:47:00 +0800 Subject: [PATCH 15/34] Improve error description with local context (#450) That way it's easier where the error is coming from in terms of the protocol. --- git-repository/src/remote/connection/fetch/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index 4cf38ba47ca..a4c6d0e5291 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -16,7 +16,7 @@ mod error { desired: Option, source: Option, }, - #[error(transparent)] + #[error("Could not decode server reply")] FetchResponse(#[from] git_protocol::fetch::response::Error), #[error(transparent)] Negotiate(#[from] super::negotiate::Error), From 57fab9afe520b9e3377d51510d86815635f7533c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:47:45 +0800 Subject: [PATCH 16/34] Fetch works, but needs more printing of information after the fact (#450) --- gitoxide-core/src/repository/fetch.rs | 1 - gitoxide-core/src/repository/remote.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index fe31a078463..11634f9c61e 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -46,7 +46,6 @@ pub(crate) mod function { .with_dry_run(dry_run) .receive(&git::interrupt::IS_INTERRUPTED)?; - dbg!(res); Ok(()) } } diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index f895cecc707..2f9f47b97ae 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -258,7 +258,7 @@ pub(crate) fn by_name_or_url<'repo>( use anyhow::Context; Ok(match name_or_url { Some(name) => { - if name.contains('/') { + if name.contains('/') || name == "." { repo.remote_at(git::url::parse(name.into())?)? } else { repo.find_remote(&name)? From d03488211ec5bd186f6d274c55cd96cfd9d119d5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 16:55:12 +0800 Subject: [PATCH 17/34] fix build (#450) --- gitoxide-core/src/repository/mod.rs | 2 + gitoxide-core/src/repository/remote.rs | 11 ++- src/plumbing/main.rs | 109 +++++++++++++------------ src/plumbing/options/mod.rs | 4 +- 4 files changed, 64 insertions(+), 62 deletions(-) diff --git a/gitoxide-core/src/repository/mod.rs b/gitoxide-core/src/repository/mod.rs index ef57ce26ab2..296b48b5c1e 100644 --- a/gitoxide-core/src/repository/mod.rs +++ b/gitoxide-core/src/repository/mod.rs @@ -19,7 +19,9 @@ pub mod config; mod credential; pub use credential::function as credential; pub mod exclude; +#[cfg(feature = "blocking-client")] pub mod fetch; +#[cfg(feature = "blocking-client")] pub use fetch::function::fetch; pub mod mailmap; pub mod odb; diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 2f9f47b97ae..279038dad06 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -249,24 +249,23 @@ mod refs_impl { #[cfg(any(feature = "blocking-client", feature = "async-client"))] pub use refs_impl::{refs, refs_fn as refs, JsonRef}; -use git_repository as git; - +#[cfg(any(feature = "blocking-client", feature = "async-client"))] pub(crate) fn by_name_or_url<'repo>( - repo: &'repo git::Repository, + repo: &'repo git_repository::Repository, name_or_url: Option<&str>, -) -> anyhow::Result> { +) -> anyhow::Result> { use anyhow::Context; Ok(match name_or_url { Some(name) => { if name.contains('/') || name == "." { - repo.remote_at(git::url::parse(name.into())?)? + repo.remote_at(git_repository::url::parse(name.into())?)? } else { repo.find_remote(&name)? } } None => repo .head()? - .into_remote(git::remote::Direction::Fetch) + .into_remote(git_repository::remote::Direction::Fetch) .context("Cannot find a remote for unborn branch")??, }) } diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 6c8404cc2bb..aedd39255d0 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -13,10 +13,9 @@ use git_repository::bstr::io::BufReadExt; use gitoxide_core as core; use gitoxide_core::pack::verify; -use crate::plumbing::options::fetch; use crate::{ plumbing::{ - options::{commit, config, credential, exclude, free, mailmap, odb, remote, revision, tree, Args, Subcommands}, + options::{commit, config, credential, exclude, free, mailmap, odb, revision, tree, Args, Subcommands}, show_progress, }, shared::pretty::prepare_and_run, @@ -114,7 +113,7 @@ pub fn main() -> Result<()> { match cmd { #[cfg(feature = "gitoxide-core-blocking-client")] - Subcommands::Fetch(fetch::Platform { + Subcommands::Fetch(crate::plumbing::options::fetch::Platform { dry_run, remote, ref_spec, @@ -145,63 +144,65 @@ pub fn main() -> Result<()> { credential::Subcommands::Reject => git::credentials::program::main::Action::Erase, }, ), - #[cfg_attr(feature = "small", allow(unused_variables))] - Subcommands::Remote(remote::Platform { + #[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] + Subcommands::Remote(crate::plumbing::options::remote::Platform { name, cmd, handshake_info, - }) => match cmd { - #[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] - remote::Subcommands::Refs | remote::Subcommands::RefMap { .. } => { - let kind = match cmd { - remote::Subcommands::Refs => core::repository::remote::refs::Kind::Remote, - remote::Subcommands::RefMap { ref_spec } => { - core::repository::remote::refs::Kind::Tracking { ref_specs: ref_spec } + }) => { + use crate::plumbing::options::remote; + match cmd { + remote::Subcommands::Refs | remote::Subcommands::RefMap { .. } => { + let kind = match cmd { + remote::Subcommands::Refs => core::repository::remote::refs::Kind::Remote, + remote::Subcommands::RefMap { ref_spec } => { + core::repository::remote::refs::Kind::Tracking { ref_specs: ref_spec } + } + }; + let context = core::repository::remote::refs::Options { + name_or_url: name, + format, + handshake_info, + }; + #[cfg(feature = "gitoxide-core-blocking-client")] + { + prepare_and_run( + "remote-refs", + verbose, + progress, + progress_keep_open, + core::repository::remote::refs::PROGRESS_RANGE, + move |progress, out, err| { + core::repository::remote::refs( + repository(Mode::LenientWithGitInstallConfig)?, + kind, + progress, + out, + err, + context, + ) + }, + ) + } + #[cfg(feature = "gitoxide-core-async-client")] + { + let (_handle, progress) = async_util::prepare( + verbose, + "remote-refs", + Some(core::repository::remote::refs::PROGRESS_RANGE), + ); + futures_lite::future::block_on(core::repository::remote::refs( + repository(Mode::LenientWithGitInstallConfig)?, + kind, + progress, + std::io::stdout(), + std::io::stderr(), + context, + )) } - }; - let context = core::repository::remote::refs::Options { - name_or_url: name, - format, - handshake_info, - }; - #[cfg(feature = "gitoxide-core-blocking-client")] - { - prepare_and_run( - "remote-refs", - verbose, - progress, - progress_keep_open, - core::repository::remote::refs::PROGRESS_RANGE, - move |progress, out, err| { - core::repository::remote::refs( - repository(Mode::LenientWithGitInstallConfig)?, - kind, - progress, - out, - err, - context, - ) - }, - ) - } - #[cfg(feature = "gitoxide-core-async-client")] - { - let (_handle, progress) = async_util::prepare( - verbose, - "remote-refs", - Some(core::repository::remote::refs::PROGRESS_RANGE), - ); - futures_lite::future::block_on(core::repository::remote::refs( - repository(Mode::LenientWithGitInstallConfig)?, - kind, - progress, - std::io::stdout(), - std::io::stderr(), - context, - )) } } - }, + } Subcommands::Config(config::Platform { filter }) => prepare_and_run( "config-list", verbose, diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 7207af6f2dd..65885877571 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -88,6 +88,7 @@ pub enum Subcommands { #[clap(subcommand)] Mailmap(mailmap::Subcommands), /// Interact with the remote hosts. + #[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] Remote(remote::Platform), /// Interact with the exclude files like .gitignore. #[clap(subcommand)] @@ -135,6 +136,7 @@ pub mod fetch { } } +#[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] pub mod remote { use git_repository as git; @@ -159,10 +161,8 @@ pub mod remote { #[clap(visible_alias = "remotes")] pub enum Subcommands { /// Print all references available on the remote. - #[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] Refs, /// Print all references available on the remote as filtered through ref-specs. - #[cfg(any(feature = "gitoxide-core-async-client", feature = "gitoxide-core-blocking-client"))] RefMap { /// Override the built-in and configured ref-specs with one or more of the given ones. #[clap(parse(try_from_os_str = git::env::os_str_to_bstring))] From 9c9df03336ada4b78de7b40b51e9b1cea6d9949d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 19:51:54 +0800 Subject: [PATCH 18/34] fix journeytests (#450) --- gitoxide-core/src/repository/remote.rs | 2 +- tests/journey/gix.sh | 18 +++++++++--------- ...ote ref-list-no-networking-in-small-failure | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 279038dad06..a7a58ddc43f 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -257,7 +257,7 @@ pub(crate) fn by_name_or_url<'repo>( use anyhow::Context; Ok(match name_or_url { Some(name) => { - if name.contains('/') || name == "." { + if name.contains('/') || name.contains('.') { repo.remote_at(git_repository::url::parse(name.into())?)? } else { repo.find_remote(&name)? diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index cded2d91421..2b3a536e97b 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -83,20 +83,20 @@ title "gix (with repository)" (with "version 1" it "generates the correct output" && { WITH_SNAPSHOT="$snapshot/file-v-any" \ - expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=1 remote -u .git refs + expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=1 remote -n .git refs } ) (with "version 2" it "generates the correct output" && { WITH_SNAPSHOT="$snapshot/file-v-any" \ - expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -u "$PWD" refs + expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -n "$PWD" refs } ) if test "$kind" = "max"; then (with "--format json" it "generates the correct output in JSON format" && { WITH_SNAPSHOT="$snapshot/file-v-any-json" \ - expect_run $SUCCESSFULLY "$exe_plumbing" --format json remote -u . refs + expect_run $SUCCESSFULLY "$exe_plumbing" --format json remote -n . refs } ) fi @@ -108,13 +108,13 @@ title "gix (with repository)" (with "version 1" it "generates the correct output" && { WITH_SNAPSHOT="$snapshot/file-v-any" \ - expect_run $SUCCESSFULLY "$exe_plumbing" --config protocol.version=1 remote --url git://localhost/ refs + expect_run $SUCCESSFULLY "$exe_plumbing" --config protocol.version=1 remote --name git://localhost/ refs } ) (with "version 2" it "generates the correct output" && { WITH_SNAPSHOT="$snapshot/file-v-any" \ - expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -u git://localhost/ refs + expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -n git://localhost/ refs } ) ) @@ -122,7 +122,7 @@ title "gix (with repository)" (with "https:// protocol (in small builds)" it "fails as http is not compiled in" && { WITH_SNAPSHOT="$snapshot/fail-http-in-small" \ - expect_run $WITH_FAILURE "$exe_plumbing" -c protocol.version=1 remote -u https://github.com/byron/gitoxide refs + expect_run $WITH_FAILURE "$exe_plumbing" -c protocol.version=1 remote -n https://github.com/byron/gitoxide refs } ) fi @@ -131,12 +131,12 @@ title "gix (with repository)" (with "https:// protocol" (with "version 1" it "generates the correct output" && { - expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=1 remote -u https://github.com/byron/gitoxide refs + expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=1 remote -n https://github.com/byron/gitoxide refs } ) (with "version 2" it "generates the correct output" && { - expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -u https://github.com/byron/gitoxide refs + expect_run $SUCCESSFULLY "$exe_plumbing" -c protocol.version=2 remote -n https://github.com/byron/gitoxide refs } ) ) @@ -145,7 +145,7 @@ title "gix (with repository)" else it "fails as the CLI doesn't include networking in 'small' mode" && { WITH_SNAPSHOT="$snapshot/remote ref-list-no-networking-in-small-failure" \ - expect_run 2 "$exe_plumbing" -c protocol.version=1 remote -u .git refs + expect_run 2 "$exe_plumbing" -c protocol.version=1 remote -n .git refs } fi ) diff --git a/tests/snapshots/plumbing/repository/remote/refs/remote ref-list-no-networking-in-small-failure b/tests/snapshots/plumbing/repository/remote/refs/remote ref-list-no-networking-in-small-failure index 7279eeb4f14..ebb5294e607 100644 --- a/tests/snapshots/plumbing/repository/remote/refs/remote ref-list-no-networking-in-small-failure +++ b/tests/snapshots/plumbing/repository/remote/refs/remote ref-list-no-networking-in-small-failure @@ -1,6 +1,6 @@ -error: Found argument 'refs' which wasn't expected, or isn't valid in this context +error: Found argument 'remote' which wasn't expected, or isn't valid in this context USAGE: - gix remote [OPTIONS] + gix [OPTIONS] For more information try --help \ No newline at end of file From 981488b413ede3442f2213632ee52f4c207629bb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 20:20:15 +0800 Subject: [PATCH 19/34] better error messages if no ref-specs are provided where needed (#450) --- .../src/remote/connection/fetch/mod.rs | 17 +++++++++++++++-- gitoxide-core/src/repository/fetch.rs | 5 +---- gitoxide-core/src/repository/remote.rs | 3 +++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index a4c6d0e5291..d4c4a0bac11 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -1,6 +1,6 @@ use crate::remote::fetch::{DryRun, RefMap}; use crate::remote::{fetch, ref_map, Connection}; -use crate::Progress; +use crate::{remote, Progress}; use git_odb::FindExt; use git_protocol::transport::client::Transport; use std::sync::atomic::AtomicBool; @@ -62,6 +62,16 @@ pub struct Outcome<'spec> { /// pub mod negotiate; +pub mod prepare { + #[derive(Debug, thiserror::Error)] + pub enum Error { + #[error("Cannot perform a meaningful fetch operation without any configured ref-specs")] + MissingRefSpecs, + #[error(transparent)] + RefMap(#[from] crate::remote::ref_map::Error), + } +} + impl<'remote, 'repo, T, P> Connection<'remote, 'repo, T, P> where T: Transport, @@ -78,7 +88,10 @@ where /// Note that this implementation is currently limited to blocking mode as it relies on Drop semantics to close the connection /// should the fetch not be performed. Furthermore, there the code doing the fetch is inherently blocking so there is no benefit. /// It's best to unblock it by placing it into its own thread or offload it should usage in an async context be required. - pub fn prepare_fetch(mut self, options: ref_map::Options) -> Result, ref_map::Error> { + pub fn prepare_fetch(mut self, options: ref_map::Options) -> Result, prepare::Error> { + if self.remote.refspecs(remote::Direction::Fetch).is_empty() { + return Err(prepare::Error::MissingRefSpecs); + } let ref_map = self.ref_map_inner(options)?; Ok(Prepare { con: Some(self), diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 11634f9c61e..fe777a84b95 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -36,11 +36,8 @@ pub(crate) mod function { bail!("JSON output isn't yet supported for fetching."); } 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)?; - } - let res = remote + let res: git::remote::fetch::Outcome<'_> = remote .connect(git::remote::Direction::Fetch, progress)? .prepare_fetch(Default::default())? .with_dry_run(dry_run) diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index a7a58ddc43f..65fc779ce85 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -166,6 +166,9 @@ mod refs_impl { refspecs.len() )?; } + 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.") + } Ok(()) } From cd1d2aab0ba90d0d06a0c3a3569bc4810f0323c5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 20:28:28 +0800 Subject: [PATCH 20/34] first basic printing of result when no change was found (#450) This just prints the ref-map, which will be determined to have no change at all. --- gitoxide-core/src/repository/fetch.rs | 12 +++++++++++- gitoxide-core/src/repository/remote.rs | 4 ++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index fe777a84b95..88d2572a45e 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -19,6 +19,7 @@ pub(crate) mod function { use crate::OutputFormat; use anyhow::bail; use git_repository as git; + use git_repository::remote::fetch::Status; pub fn fetch( repo: git::Repository, @@ -43,6 +44,15 @@ pub(crate) mod function { .with_dry_run(dry_run) .receive(&git::interrupt::IS_INTERRUPTED)?; - Ok(()) + match res.status { + Status::NoChange => crate::repository::remote::refs::print_refmap( + &repo, + remote.refspecs(git::remote::Direction::Fetch), + res.ref_map, + out, + err, + ), + Status::Change { update_refs, .. } | Status::DryRun { update_refs } => todo!("change printing or dry-run"), + } } } diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 65fc779ce85..37f67dee884 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -27,7 +27,7 @@ mod refs_impl { pub handshake_info: bool, } - pub(crate) use super::print; + pub(crate) use super::{print, print_refmap}; } #[git::protocol::maybe_async::maybe_async] @@ -91,7 +91,7 @@ mod refs_impl { } } - fn print_refmap( + pub(crate) fn print_refmap( repo: &git::Repository, refspecs: &[RefSpec], mut map: git::remote::fetch::RefMap<'_>, From 169a979228e4f82b7c465d88dce0a304d864aeab Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 20:55:00 +0800 Subject: [PATCH 21/34] Display for `remote::fetch::update::refs::Mode` (#450) --- .../connection/fetch/update_refs/update.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) 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 67a4b39d628..4dd4f69dab2 100644 --- a/git-repository/src/remote/connection/fetch/update_refs/update.rs +++ b/git-repository/src/remote/connection/fetch/update_refs/update.rs @@ -67,6 +67,29 @@ pub enum Mode { }, } +impl std::fmt::Display for Mode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Mode::NoChangeNeeded => "up-to-date", + Mode::FastForward => "fast-forward", + Mode::Forced => "forced-update", + Mode::New => "new", + Mode::RejectedSourceObjectNotFound { id } => return write!(f, "rejected ({} not found)", id), + Mode::RejectedTagUpdate => "rejected (would overwrite existing tag)", + Mode::RejectedNonFastForward => "rejected (non-fast-forward)", + Mode::RejectedSymbolic => "rejected (refusing to write symbolic refs)", + Mode::RejectedCurrentlyCheckedOut { worktree_dir } => { + return write!( + f, + "rejected (cannot write into checked-out branch at \"{}\")", + worktree_dir.display() + ) + } + } + .fmt(f) + } +} + impl Outcome { /// Produce an iterator over all information used to produce the this outcome, ref-update by ref-update, using the `mappings` /// used when producing the ref update. From 13ac9ba3846a6facba78714fbb79e2feaf5be4de Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 21:07:48 +0800 Subject: [PATCH 22/34] =?UTF-8?q?first=20sketch=20of=20printing=20fetch=20?= =?UTF-8?q?results,=20but=E2=80=A6=20(#450)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …it still violates the protocol in dry-run mode as it actually has to get the pack, apparently the server can't be told to stop otherwise. Needs more experimentation as well. --- gitoxide-core/src/repository/fetch.rs | 98 +++++++++++++++++++++++--- gitoxide-core/src/repository/remote.rs | 4 +- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 88d2572a45e..030317af65d 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -13,17 +13,17 @@ pub struct Options { pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=2; pub(crate) mod function { - #![allow(unused_variables, unused_mut)] - use super::Options; use crate::OutputFormat; use anyhow::bail; use git_repository as git; + use git_repository::prelude::ObjectIdExt; + use git_repository::refspec::match_group::validate::Fix; use git_repository::remote::fetch::Status; pub fn fetch( repo: git::Repository, - mut progress: impl git::Progress, + progress: impl git::Progress, mut out: impl std::io::Write, err: impl std::io::Write, Options { @@ -36,23 +36,99 @@ pub(crate) mod function { if format != OutputFormat::Human { bail!("JSON output isn't yet supported for fetching."); } - let mut remote = crate::repository::remote::by_name_or_url(&repo, remote.as_deref())?; + 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)?; + } let res: git::remote::fetch::Outcome<'_> = remote .connect(git::remote::Direction::Fetch, progress)? .prepare_fetch(Default::default())? .with_dry_run(dry_run) .receive(&git::interrupt::IS_INTERRUPTED)?; + let ref_specs = remote.refspecs(git::remote::Direction::Fetch); match res.status { - Status::NoChange => crate::repository::remote::refs::print_refmap( - &repo, - remote.refspecs(git::remote::Direction::Fetch), - res.ref_map, - out, + Status::NoChange => { + crate::repository::remote::refs::print_refmap(&repo, ref_specs, res.ref_map, &mut out, err) + } + Status::Change { update_refs, .. } | Status::DryRun { update_refs } => { + print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err) + } + }?; + if dry_run { + writeln!(out, "DRY-RUN: No ref was updated and no pack was received.").ok(); + } + Ok(()) + } + + pub(crate) fn print_updates( + repo: &git::Repository, + update_refs: git::remote::fetch::refs::update::Outcome, + refspecs: &[git::refspec::RefSpec], + mut map: git::remote::fetch::RefMap<'_>, + mut out: impl std::io::Write, + mut err: impl std::io::Write, + ) -> anyhow::Result<()> { + let mut last_spec_index = usize::MAX; + let mut updates = update_refs + .iter_mapping_updates(&map.mappings, refspecs) + .collect::>(); + updates.sort_by_key(|t| t.2); + for (update, mapping, spec, edit) in updates { + if mapping.spec_index != last_spec_index { + last_spec_index = mapping.spec_index; + spec.to_ref().write_to(&mut out)?; + writeln!(out)?; + } + + write!(out, "\t")?; + match &mapping.remote { + git::remote::fetch::Source::ObjectId(id) => { + write!(out, "{}", id.attach(repo).shorten_or_id())?; + } + git::remote::fetch::Source::Ref(r) => { + crate::repository::remote::refs::print_ref(&mut out, r)?; + } + }; + match edit { + Some(edit) => { + writeln!(out, " -> {} [{}]", edit.name, update.mode) + } + None => writeln!(out, " (fetch only)"), + }?; + } + if !map.fixes.is_empty() { + writeln!( + err, + "The following destination refs were removed as they didn't start with 'ref/'" + )?; + map.fixes.sort_by_key(|f| match f { + Fix::MappingWithPartialDestinationRemoved { spec, .. } => *spec, + }); + let mut prev_spec = None; + for fix in &map.fixes { + match fix { + Fix::MappingWithPartialDestinationRemoved { name, spec } => { + if prev_spec.map_or(true, |prev_spec| prev_spec != spec) { + prev_spec = spec.into(); + spec.write_to(&mut err)?; + writeln!(err)?; + } + writeln!(err, "\t{name}")?; + } + } + } + } + if map.remote_refs.len() - map.mappings.len() != 0 { + writeln!( err, - ), - Status::Change { update_refs, .. } | Status::DryRun { update_refs } => todo!("change printing or dry-run"), + "server sent {} tips, {} were filtered due to {} refspec(s).", + map.remote_refs.len(), + map.remote_refs.len() - map.mappings.len(), + refspecs.len() + )?; } + Ok(()) } } diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 37f67dee884..01d08f39961 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -27,7 +27,7 @@ mod refs_impl { pub handshake_info: bool, } - pub(crate) use super::{print, print_refmap}; + pub(crate) use super::{print, print_ref, print_refmap}; } #[git::protocol::maybe_async::maybe_async] @@ -222,7 +222,7 @@ mod refs_impl { } } - fn print_ref(mut out: impl std::io::Write, r: &fetch::Ref) -> std::io::Result<&git::hash::oid> { + pub(crate) fn print_ref(mut out: impl std::io::Write, r: &fetch::Ref) -> std::io::Result<&git::hash::oid> { match r { fetch::Ref::Direct { full_ref_name: path, From ce9b59115bc66f052324e169c574f2515565a496 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 21:13:53 +0800 Subject: [PATCH 23/34] fix hang in V1 mode (#450) This happens if the local transport is used, which goes by V1 by default and it's probably good to try supporting it if it's easy enough, instead of forcing V2 which we probably could. --- .../src/remote/connection/fetch/mod.rs | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index d4c4a0bac11..5f1abd25622 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -96,7 +96,7 @@ where Ok(Prepare { con: Some(self), ref_map, - dry_run: fetch::DryRun::No, + dry_run: DryRun::No, }) } } @@ -193,7 +193,21 @@ where options, )?) } else { - drop(reader); + if protocol_version == git_protocol::transport::Protocol::V1 { + git_pack::Bundle::write_to_directory( + reader, + None::, + con.progress, + should_interrupt, + Some(Box::new({ + let repo = repo.clone(); + move |oid, buf| repo.objects.find(oid, buf).ok() + })), + options, + )?; + } else { + drop(reader); + } None }; @@ -205,7 +219,7 @@ where repo, "fetch", &self.ref_map.mappings, - con.remote.refspecs(crate::remote::Direction::Fetch), + con.remote.refspecs(remote::Direction::Fetch), self.dry_run, )?; @@ -247,7 +261,7 @@ where { con: Option>, ref_map: RefMap<'remote>, - dry_run: fetch::DryRun, + dry_run: DryRun, } /// Builder @@ -259,7 +273,7 @@ where /// /// This works by not actually fetching the pack after negotiating it, nor will refs be updated. pub fn with_dry_run(mut self, enabled: bool) -> Self { - self.dry_run = enabled.then(|| fetch::DryRun::Yes).unwrap_or(DryRun::No); + self.dry_run = enabled.then(|| DryRun::Yes).unwrap_or(DryRun::No); self } } From c47dcc69e54c635650b540c590131dbe7d32d05b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 21:37:54 +0800 Subject: [PATCH 24/34] support for handshake information in `gix fetch` (#450) --- gitoxide-core/src/repository/fetch.rs | 7 +++++++ src/plumbing/main.rs | 2 ++ src/plumbing/options/mod.rs | 6 +++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 030317af65d..1d23f29af2f 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -8,6 +8,7 @@ pub struct Options { pub remote: Option, /// If non-empty, override all ref-specs otherwise configured in the remote pub ref_specs: Vec, + pub handshake_info: bool, } pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=2; @@ -30,6 +31,7 @@ pub(crate) mod function { format, dry_run, remote, + handshake_info, ref_specs, }: Options, ) -> anyhow::Result<()> { @@ -47,6 +49,11 @@ pub(crate) mod function { .with_dry_run(dry_run) .receive(&git::interrupt::IS_INTERRUPTED)?; + if handshake_info { + writeln!(out, "Handshake Information")?; + writeln!(out, "\t{:?}", res.ref_map.handshake)?; + } + let ref_specs = remote.refspecs(git::remote::Direction::Fetch); match res.status { Status::NoChange => { diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index aedd39255d0..b8c561ddcd5 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -115,6 +115,7 @@ pub fn main() -> Result<()> { #[cfg(feature = "gitoxide-core-blocking-client")] Subcommands::Fetch(crate::plumbing::options::fetch::Platform { dry_run, + handshake_info, remote, ref_spec, }) => { @@ -122,6 +123,7 @@ pub fn main() -> Result<()> { format, dry_run, remote, + handshake_info, ref_specs: ref_spec, }; prepare_and_run( diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 65885877571..7072c8b47bc 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -124,6 +124,10 @@ pub mod fetch { #[clap(long, short = 'n')] pub dry_run: bool, + /// Output additional typically information provided by the server as part of the connection handshake. + #[clap(long, short = 'H')] + pub handshake_info: bool, + /// The name of the remote to connect to, or the url of the remote to connect to directly. /// /// If unset, the current branch will determine the remote. @@ -149,7 +153,7 @@ pub mod remote { pub name: Option, /// Output additional typically information provided by the server as part of the connection handshake. - #[clap(long)] + #[clap(long, short = 'H')] pub handshake_info: bool, /// Subcommands From 6e2a2375f84b5e3fcbc5aa85803f79d65e3d07fb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 21:53:24 +0800 Subject: [PATCH 25/34] adjust fetch-progress range (#450) --- gitoxide-core/src/repository/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 1d23f29af2f..488640a4bff 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -11,7 +11,7 @@ pub struct Options { pub handshake_info: bool, } -pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=2; +pub const PROGRESS_RANGE: std::ops::RangeInclusive = 1..=3; pub(crate) mod function { use super::Options; From d782ff080f43e8a60e8ec522a9f5b55c583abdec Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 2 Oct 2022 22:05:49 +0800 Subject: [PATCH 26/34] also inform about the location of the new pack and index files (#450) --- gitoxide-core/src/repository/fetch.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index 488640a4bff..7920e08eb64 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -59,8 +59,19 @@ pub(crate) mod function { Status::NoChange => { crate::repository::remote::refs::print_refmap(&repo, ref_specs, res.ref_map, &mut out, err) } - Status::Change { update_refs, .. } | Status::DryRun { 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 { + update_refs, + write_pack_bundle, + } => { + print_updates(&repo, update_refs, ref_specs, res.ref_map, &mut out, err)?; + if let Some(data_path) = write_pack_bundle.data_path { + writeln!(out, "pack file: \"{}\"", data_path.display()).ok(); + } + if let Some(index_path) = write_pack_bundle.index_path { + writeln!(out, "index file: \"{}\"", index_path.display()).ok(); + } + Ok(()) } }?; if dry_run { From a745512185fb0a46e35daaa6d28829aec05edb55 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 09:05:38 +0800 Subject: [PATCH 27/34] fix: increase pack-receive performance using a BufWriter (#450) Previously the NamedTempFile would receive every little write request for millions of objects, consuming considerable amounts of time. Now a buf writer alleviates this issue entirely. --- git-pack/src/bundle/write/mod.rs | 31 +++++++++++++++++++----------- git-pack/src/bundle/write/types.rs | 8 +++++--- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/git-pack/src/bundle/write/mod.rs b/git-pack/src/bundle/write/mod.rs index ecc18c4e4d2..f39fa287c11 100644 --- a/git-pack/src/bundle/write/mod.rs +++ b/git-pack/src/bundle/write/mod.rs @@ -1,3 +1,4 @@ +use std::io::Write; use std::{ io, path::{Path, PathBuf}, @@ -5,7 +6,7 @@ use std::{ }; use git_features::{interrupt, progress, progress::Progress}; -use git_tempfile::{handle::Writable, AutoRemove, ContainingDirectory}; +use git_tempfile::{AutoRemove, ContainingDirectory}; use crate::data; @@ -13,6 +14,7 @@ mod error; pub use error::Error; mod types; +use crate::bundle::write::types::SharedTempFile; use types::{LockWriter, PassThrough}; pub use types::{Options, Outcome}; @@ -55,10 +57,13 @@ impl crate::Bundle { }; let object_hash = options.object_hash; - let data_file = Arc::new(parking_lot::Mutex::new(match directory.as_ref() { - Some(directory) => git_tempfile::new(directory, ContainingDirectory::Exists, AutoRemove::Tempfile)?, - None => git_tempfile::new(std::env::temp_dir(), ContainingDirectory::Exists, AutoRemove::Tempfile)?, - })); + let data_file = Arc::new(parking_lot::Mutex::new(io::BufWriter::with_capacity( + 64 * 1024, + match directory.as_ref() { + Some(directory) => git_tempfile::new(directory, ContainingDirectory::Exists, AutoRemove::Tempfile)?, + None => git_tempfile::new(std::env::temp_dir(), ContainingDirectory::Exists, AutoRemove::Tempfile)?, + }, + ))); let (pack_entries_iter, pack_kind): ( Box>>, _, @@ -97,7 +102,7 @@ impl crate::Bundle { }, writer: Some(data_file.clone()), }; - // This buff-reader is required to assure we call 'read()' in order to fill the (extra) buffer. Otherwise all the counting + // This buf-reader is required to assure we call 'read()' in order to fill the (extra) buffer. Otherwise all the counting // we do with the wrapped pack reader doesn't work as it does not expect anyone to call BufRead functions directly. // However, this is exactly what's happening in the ZipReader implementation that is eventually used. // The performance impact of this is probably negligible, compared to all the other work that is done anyway :D. @@ -153,10 +158,10 @@ impl crate::Bundle { progress: progress::ThroughputOnDrop::new(read_progress), }; - let data_file = Arc::new(parking_lot::Mutex::new(match directory.as_ref() { + let data_file = Arc::new(parking_lot::Mutex::new(io::BufWriter::new(match directory.as_ref() { Some(directory) => git_tempfile::new(directory, ContainingDirectory::Exists, AutoRemove::Tempfile)?, None => git_tempfile::new(std::env::temp_dir(), ContainingDirectory::Exists, AutoRemove::Tempfile)?, - })); + }))); let object_hash = options.object_hash; let eight_pages = 4096 * 8; let (pack_entries_iter, pack_kind): ( @@ -231,7 +236,7 @@ impl crate::Bundle { index_version: index_kind, object_hash, }: Options, - data_file: Arc>>, + data_file: SharedTempFile, pack_entries_iter: impl Iterator>, should_interrupt: &AtomicBool, ) -> Result<(crate::index::write::Outcome, Option, Option), Error> { @@ -261,6 +266,8 @@ impl crate::Bundle { Arc::try_unwrap(data_file) .expect("only one handle left after pack was consumed") .into_inner() + .into_inner() + .map_err(|err| Error::from(err.into_error()))? .persist(&data_path)?; index_file .persist(&index_path) @@ -292,10 +299,12 @@ impl crate::Bundle { } fn new_pack_file_resolver( - data_file: Arc>>, + data_file: SharedTempFile, ) -> io::Result) -> Option<()> + Send + Clone> { + let mut guard = data_file.lock(); + guard.flush()?; let mapped_file = Arc::new(crate::mmap::read_only( - &data_file.lock().with_mut(|f| f.path().to_owned())?, + &guard.get_mut().with_mut(|f| f.path().to_owned())?, )?); let pack_data_lookup = move |range: std::ops::Range, out: &mut Vec| -> Option<()> { mapped_file diff --git a/git-pack/src/bundle/write/types.rs b/git-pack/src/bundle/write/types.rs index bf4a22161bd..13c3576eb29 100644 --- a/git-pack/src/bundle/write/types.rs +++ b/git-pack/src/bundle/write/types.rs @@ -55,9 +55,11 @@ impl Outcome { } } +pub(crate) type SharedTempFile = Arc>>>; + pub(crate) struct PassThrough { pub reader: R, - pub writer: Option>>>, + pub writer: Option, } impl io::Read for PassThrough @@ -87,7 +89,7 @@ where } pub(crate) struct LockWriter { - pub writer: Arc>>, + pub writer: SharedTempFile, } impl io::Write for LockWriter { @@ -102,7 +104,7 @@ impl io::Write for LockWriter { impl io::Read for LockWriter { fn read(&mut self, buf: &mut [u8]) -> io::Result { - self.writer.lock().read(buf) + self.writer.lock().get_mut().read(buf) } } From e489b10878c85289e0a9869804abee2418de6989 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 09:26:11 +0800 Subject: [PATCH 28/34] prefix created pack files with `pack-` (#450) --- git-pack/src/bundle/write/mod.rs | 2 +- git-pack/tests/pack/bundle.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/git-pack/src/bundle/write/mod.rs b/git-pack/src/bundle/write/mod.rs index f39fa287c11..9159d0711f1 100644 --- a/git-pack/src/bundle/write/mod.rs +++ b/git-pack/src/bundle/write/mod.rs @@ -260,7 +260,7 @@ impl crate::Bundle { object_hash, )?; - let data_path = directory.join(format!("{}.pack", outcome.data_hash.to_hex())); + let data_path = directory.join(format!("pack-{}.pack", outcome.data_hash.to_hex())); let index_path = data_path.with_extension("idx"); Arc::try_unwrap(data_file) diff --git a/git-pack/tests/pack/bundle.rs b/git-pack/tests/pack/bundle.rs index dc6faf0489c..1913eb35aca 100644 --- a/git-pack/tests/pack/bundle.rs +++ b/git-pack/tests/pack/bundle.rs @@ -125,10 +125,10 @@ mod write_to_directory { assert_eq!(sorted_entries.len(), 2, "we want a pack and the corresponding index"); let pack_hash = res.index.data_hash.to_hex(); - assert_eq!(file_name(&sorted_entries[0]), format!("{}.idx", pack_hash)); + assert_eq!(file_name(&sorted_entries[0]), format!("pack-{}.idx", pack_hash)); assert_eq!(Some(sorted_entries[0].path()), index_path); - assert_eq!(file_name(&sorted_entries[1]), format!("{}.pack", pack_hash)); + assert_eq!(file_name(&sorted_entries[1]), format!("pack-{}.pack", pack_hash)); assert_eq!(Some(sorted_entries[1].path()), data_path); res.index_path = index_path; From f10a3e0772b1317ab8e0a1ba38d2b137346f0868 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 09:54:54 +0800 Subject: [PATCH 29/34] fix journey-test expecations (#450) Changed due to new naming of index and packs. --- .../plumbing/no-repo/pack/index/create/output-dir-content | 4 ++-- .../plumbing/no-repo/pack/receive/file-v-any-with-output | 4 ++-- .../snapshots/plumbing/no-repo/pack/receive/ls-in-output-dir | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/snapshots/plumbing/no-repo/pack/index/create/output-dir-content b/tests/snapshots/plumbing/no-repo/pack/index/create/output-dir-content index a649eb341f0..4da7421442e 100644 --- a/tests/snapshots/plumbing/no-repo/pack/index/create/output-dir-content +++ b/tests/snapshots/plumbing/no-repo/pack/index/create/output-dir-content @@ -1,2 +1,2 @@ -f1cd3cc7bc63a4a2b357a475a58ad49b40355470.idx -f1cd3cc7bc63a4a2b357a475a58ad49b40355470.pack \ No newline at end of file +pack-f1cd3cc7bc63a4a2b357a475a58ad49b40355470.idx +pack-f1cd3cc7bc63a4a2b357a475a58ad49b40355470.pack \ No newline at end of file diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-with-output b/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-with-output index 6ea910a5bda..821e8b0d90b 100644 --- a/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-with-output +++ b/tests/snapshots/plumbing/no-repo/pack/receive/file-v-any-with-output @@ -1,5 +1,5 @@ -index: c787de2aafb897417ca8167baeb146eabd18bc5f (out/346574b7331dc3a1724da218d622c6e1b6c66a57.idx) -pack: 346574b7331dc3a1724da218d622c6e1b6c66a57 (out/346574b7331dc3a1724da218d622c6e1b6c66a57.pack) +index: c787de2aafb897417ca8167baeb146eabd18bc5f (out/pack-346574b7331dc3a1724da218d622c6e1b6c66a57.idx) +pack: 346574b7331dc3a1724da218d622c6e1b6c66a57 (out/pack-346574b7331dc3a1724da218d622c6e1b6c66a57.pack) 3f72b39ad1600e6dac63430c15e0d875e9d3f9d6 HEAD symref-target:refs/heads/main ee3c97678e89db4eab7420b04aef51758359f152 refs/heads/dev diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/ls-in-output-dir b/tests/snapshots/plumbing/no-repo/pack/receive/ls-in-output-dir index 9c73e822fe2..98e5ef743ef 100644 --- a/tests/snapshots/plumbing/no-repo/pack/receive/ls-in-output-dir +++ b/tests/snapshots/plumbing/no-repo/pack/receive/ls-in-output-dir @@ -1,2 +1,2 @@ -346574b7331dc3a1724da218d622c6e1b6c66a57.idx -346574b7331dc3a1724da218d622c6e1b6c66a57.pack \ No newline at end of file +pack-346574b7331dc3a1724da218d622c6e1b6c66a57.idx +pack-346574b7331dc3a1724da218d622c6e1b6c66a57.pack \ No newline at end of file From 6d8b66a9bee901eb8cb869e6e28dbb25988f1fed Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 11:26:46 +0800 Subject: [PATCH 30/34] fix: remove `Drop` for `SpawnProcessOnDemand`. (#450) It is well-intended but is likely to hang the calling process if for any reason not all process output was consumed. Even though not waiting for the process leaves it running, it will stop naturally once its output pipe breaks once once our handles for it are inevitable dropped at the same time. --- git-transport/src/client/blocking_io/file.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/git-transport/src/client/blocking_io/file.rs b/git-transport/src/client/blocking_io/file.rs index 2ea33b27bda..70c21f0cd8f 100644 --- a/git-transport/src/client/blocking_io/file.rs +++ b/git-transport/src/client/blocking_io/file.rs @@ -42,14 +42,6 @@ pub struct SpawnProcessOnDemand { child: Option, } -impl Drop for SpawnProcessOnDemand { - fn drop(&mut self) { - if let Some(mut child) = self.child.take() { - child.wait().ok(); - } - } -} - impl SpawnProcessOnDemand { pub(crate) fn new_ssh( url: git_url::Url, From 1777fa13e9e821c7a720227b6d7f1ad79f751840 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 12:00:45 +0800 Subject: [PATCH 31/34] no need to fetch the whole pack in dry-run mode, even in V1. (#450) This demands explanation. Previously the issue was a blocking transport implementation on drop, but that has been fixed by simply letting the child process run into a broken pipe to stop it. Now existing early won't hang because of the transport being dropped with data still available on the stdout pipe. The issue to fix here is that keep-alive packets are not currently understood in V1, and git can send us empty packs. --- .../src/remote/connection/fetch/mod.rs | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index 5f1abd25622..34fc66b38ad 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -193,21 +193,7 @@ where options, )?) } else { - if protocol_version == git_protocol::transport::Protocol::V1 { - git_pack::Bundle::write_to_directory( - reader, - None::, - con.progress, - should_interrupt, - Some(Box::new({ - let repo = repo.clone(); - move |oid, buf| repo.objects.find(oid, buf).ok() - })), - options, - )?; - } else { - drop(reader); - } + drop(reader); None }; From 41b0c19e1aca9406015932862058756af2a26dda Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 12:19:45 +0800 Subject: [PATCH 32/34] fix: set the protocol version for local git transports as well. (#450) Previously this was only done for ssh based connections, and requires setting an environment variable. --- git-transport/src/client/blocking_io/file.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/git-transport/src/client/blocking_io/file.rs b/git-transport/src/client/blocking_io/file.rs index 70c21f0cd8f..bbf79a3c7b4 100644 --- a/git-transport/src/client/blocking_io/file.rs +++ b/git-transport/src/client/blocking_io/file.rs @@ -68,7 +68,9 @@ impl SpawnProcessOnDemand { path, ssh_program: None, ssh_args: Vec::new(), - ssh_env: Vec::new(), + ssh_env: (version != Protocol::V1) + .then(|| vec![("GIT_PROTOCOL", format!("version={}", version as usize))]) + .unwrap_or_default(), child: None, connection: None, desired_version: version, From e616174e40b5dd11217d540ba7b17e3164f7ca30 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 15:27:31 +0800 Subject: [PATCH 33/34] fix test expectations to handle V1/V2 differences. (#450) Now V2 is actually used in our local tests as well. --- git-repository/tests/remote/fetch.rs | 23 ++++++++++++++--------- git-repository/tests/remote/ref_map.rs | 13 +++++++------ 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/git-repository/tests/remote/fetch.rs b/git-repository/tests/remote/fetch.rs index 08f8248d45f..4495dd55d2c 100644 --- a/git-repository/tests/remote/fetch.rs +++ b/git-repository/tests/remote/fetch.rs @@ -23,10 +23,18 @@ mod blocking_io { #[test] fn fetch_pack() -> crate::Result { - for version in [ - None, - Some(git::protocol::transport::Protocol::V2), - Some(git::protocol::transport::Protocol::V1), + for (version, expected_objects, expected_hash) in [ + (None, 4, "d07c527cf14e524a8494ce6d5d08e28079f5c6ea"), + ( + Some(git::protocol::transport::Protocol::V2), + 4, + "d07c527cf14e524a8494ce6d5d08e28079f5c6ea", + ), + ( + Some(git::protocol::transport::Protocol::V1), + 3, + "c75114f60ab2c9389916f3de1082bbaa47491e3b", + ), ] { let (mut repo, _tmp) = repo_rw("two-origins"); if let Some(version) = version { @@ -69,15 +77,12 @@ mod blocking_io { } => { assert_eq!(write_pack_bundle.pack_kind, git::odb::pack::data::Version::V2); assert_eq!(write_pack_bundle.object_hash, repo.object_hash()); - assert_eq!(write_pack_bundle.index.num_objects, 3, "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, expected_objects, "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("c75114f60ab2c9389916f3de1082bbaa47491e3b") - ); + assert_eq!(write_pack_bundle.index.index_hash, hex_to_id(expected_hash)); 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())); diff --git a/git-repository/tests/remote/ref_map.rs b/git-repository/tests/remote/ref_map.rs index dc1194bec41..93e3654deb2 100644 --- a/git-repository/tests/remote/ref_map.rs +++ b/git-repository/tests/remote/ref_map.rs @@ -8,10 +8,10 @@ mod blocking_io { #[test] fn all() -> crate::Result { - for version in [ - None, - Some(git::protocol::transport::Protocol::V2), - Some(git::protocol::transport::Protocol::V1), + 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. ] { let mut repo = remote::repo("clone"); if let Some(version) = version { @@ -27,8 +27,9 @@ mod blocking_io { let map = remote.connect(Fetch, progress::Discard)?.ref_map(Default::default())?; assert_eq!( map.remote_refs.len(), - 14, - "it gets all remote refs, independently of the refspec." + expected_remote_refs, + "{:?}: 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); From d7f62b441700c6d3526517c8c4f369cb9a72c102 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 3 Oct 2022 18:34:43 +0800 Subject: [PATCH 34/34] fix: support keepalive packets. (#450) Keepalive packets are side-band only empty datalines that should just be ignored. This is now happening, allowing longer git operations to work as they will send keepalive packets every 5 seconds, and previously we would choke on it. Note that empty datalines are never send otherwise, making it a previously unused marker that can safely be skipped. --- git-packetline/src/read/sidebands/async_io.rs | 7 +++- .../src/read/sidebands/blocking_io.rs | 7 +++- git-protocol/tests/fetch/response.rs | 23 +++++++---- git-protocol/tests/fetch/v1.rs | 38 ++++++++++-------- .../fixtures/v1/clone-with-keepalive.response | Bin 0 -> 1467 bytes .../v2/clone-only-with-keepalive.response | Bin 0 -> 1140 bytes 6 files changed, 49 insertions(+), 26 deletions(-) create mode 100644 git-protocol/tests/fixtures/v1/clone-with-keepalive.response create mode 100644 git-protocol/tests/fixtures/v2/clone-only-with-keepalive.response diff --git a/git-packetline/src/read/sidebands/async_io.rs b/git-packetline/src/read/sidebands/async_io.rs index bfd408374ae..5c67c16fa78 100644 --- a/git-packetline/src/read/sidebands/async_io.rs +++ b/git-packetline/src/read/sidebands/async_io.rs @@ -238,7 +238,12 @@ where .map_err(|err| io::Error::new(io::ErrorKind::Other, err))?; const ENCODED_BAND: usize = 1; match band { - BandRef::Data(d) => break (U16_HEX_BYTES + ENCODED_BAND, d.len()), + BandRef::Data(d) => { + if d.is_empty() { + continue; + } + break (U16_HEX_BYTES + ENCODED_BAND, d.len()); + } BandRef::Progress(d) => { let text = TextRef::from(d).0; handle_progress(false, text); diff --git a/git-packetline/src/read/sidebands/blocking_io.rs b/git-packetline/src/read/sidebands/blocking_io.rs index 3fc8ea865b0..e8a83466841 100644 --- a/git-packetline/src/read/sidebands/blocking_io.rs +++ b/git-packetline/src/read/sidebands/blocking_io.rs @@ -113,7 +113,12 @@ where .map_err(|err| io::Error::new(io::ErrorKind::Other, err))?; const ENCODED_BAND: usize = 1; match band { - BandRef::Data(d) => break (U16_HEX_BYTES + ENCODED_BAND, d.len()), + BandRef::Data(d) => { + if d.is_empty() { + continue; + } + break (U16_HEX_BYTES + ENCODED_BAND, d.len()); + } BandRef::Progress(d) => { let text = TextRef::from(d).0; handle_progress(false, text); diff --git a/git-protocol/tests/fetch/response.rs b/git-protocol/tests/fetch/response.rs index 7b5679a67a1..842467f78da 100644 --- a/git-protocol/tests/fetch/response.rs +++ b/git-protocol/tests/fetch/response.rs @@ -124,14 +124,21 @@ mod v2 { #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] async fn clone() -> crate::Result { - let mut provider = mock_reader("v2/clone-only.response"); - let mut reader = provider.as_read_without_sidebands(); - let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader).await?; - assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile"); - assert!(r.has_pack()); - let mut buf = Vec::new(); - let bytes_read = reader.read_to_end(&mut buf).await?; - assert_eq!(bytes_read, 1089, "should be able to read the whole pack"); + for keepalive in [false, true] { + let fixture = format!( + "v2/clone-only{}.response", + keepalive.then(|| "-with-keepalive").unwrap_or_default() + ); + let mut provider = mock_reader(&fixture); + let mut reader = provider.as_read_without_sidebands(); + let r = fetch::Response::from_line_reader(Protocol::V2, &mut reader).await?; + assert!(r.acknowledgements().is_empty(), "it should go straight to the packfile"); + assert!(r.has_pack()); + reader.set_progress_handler(Some(Box::new(|_is_err, _text| ()))); + let mut buf = Vec::new(); + let bytes_read = reader.read_to_end(&mut buf).await?; + assert_eq!(bytes_read, 876, "should be able to read the whole pack"); + } Ok(()) } diff --git a/git-protocol/tests/fetch/v1.rs b/git-protocol/tests/fetch/v1.rs index 2820ebab364..d689b094dc3 100644 --- a/git-protocol/tests/fetch/v1.rs +++ b/git-protocol/tests/fetch/v1.rs @@ -7,22 +7,28 @@ use crate::fetch::{helper_unused, oid, transport, CloneDelegate, LsRemoteDelegat #[maybe_async::test(feature = "blocking-client", async(feature = "async-client", async_std::test))] async fn clone() -> crate::Result { - let out = Vec::new(); - let mut dlg = CloneDelegate::default(); - git_protocol::fetch( - transport( - out, - "v1/clone.response", - Protocol::V1, - git_transport::client::git::ConnectMode::Daemon, - ), - &mut dlg, - helper_unused, - progress::Discard, - FetchConnection::TerminateOnSuccessfulCompletion, - ) - .await?; - assert_eq!(dlg.pack_bytes, 876, "It be able to read pack bytes"); + for with_keepalive in [false, true] { + let out = Vec::new(); + let mut dlg = CloneDelegate::default(); + let fixture = format!( + "v1/clone{}.response", + with_keepalive.then(|| "-with-keepalive").unwrap_or_default() + ); + git_protocol::fetch( + transport( + out, + &fixture, + Protocol::V1, + git_transport::client::git::ConnectMode::Daemon, + ), + &mut dlg, + helper_unused, + progress::Discard, + FetchConnection::TerminateOnSuccessfulCompletion, + ) + .await?; + assert_eq!(dlg.pack_bytes, 876, "{}: It be able to read pack bytes", fixture); + } Ok(()) } diff --git a/git-protocol/tests/fixtures/v1/clone-with-keepalive.response b/git-protocol/tests/fixtures/v1/clone-with-keepalive.response new file mode 100644 index 0000000000000000000000000000000000000000..952832b1a8702ea7ce618277f4de4f5637ac3ec3 GIT binary patch literal 1467 zcmXpoG%&R=ut+sENHI4uNi#D5V#~xNOXFm-ltd#-L(60XLlZMI^Q6?2Bn1ywM;C_N z(wvga_{8LFg_4ZSJlz5it2i?yRW~UyF9pHUH8aUp$WJTQO-aouNmMA#NX*H}FIPxO zO)W^x(=E=-OHPGx^72by%%aqs#FEUiRE4~J-GZY0^rF<_Vg;buoKm0#C5h<@aNFZk zQcDsub5em;R^}F^rrClVX9Z*w>u01UrWEVvCKi{Z7AfQ>Wu+#U=%(ct5zsez z=}<=$TPYapD5T`)rRo9I85uD-=a=T8$}1Qft14(1>KkkF;!hvTA#il53;+?#xnc=ny$>i*t=TR=J-dZ z<4=o}J*{~3R4=}go6S4@pl;Us%`Bd~!^5NcDTOg8Trud2DrJ0)38 zS|GgiV#oV*rn{~!eqYMg>U1WKi0*b)vd=(Zfpi z>btYk+iF*&T5|^3CGM`v->i@vXydE-@576T>U1UF7jBZ1gfE81+qazewAXNdsL?r1 zrqVBH*?spvniZRZEyed8tIgDHTfTm--r_1}$u>3Jhu7NO*PiUMQC<-7jwSXc``e8< z0c{J#TK`Vst5Pgi%HFnK_`BYe#_%7!n-^@=p1gPSqQ;3T=i5JY>X;_9E{ zVGAy*s=ZyxGe2b8be6_-({80NzHFBBbf4e+)5ksuzAxRQ8&&!wqrF%`VQy9Fsq;JM zubO@%>gHmt_T-G0Gv1!)zS85Id)j%8zG%zqRU~lNO^PBJ+& zQ^+$e+t#ize#yyAW+xfel?InQ`2C5!Cc3_{e%2c4_@CQz-u!1{IcuciXE=dTgRfBL z=c3xp>yG|^yK-4prq+k+>px6dbn`%<%!BN%v$4$nzosiQF!rvNwK@Kg>G;zkWlt*} zJ=Ke^U2O5}>t8j!FQpkzp2UCb&&gZxdlviW&g>1px?wd#4gtz{BwK`u#Lp7Zauim||+h@Lrs!rOLDBb%@8yS>#ZJns@UG%V$z54F#^tRd+ zsn(o9c8R;|@;56a2io{*{`>GEqB>p4_l29}B;kvp@%Am}J?%BzA8K??ld1FzT6W+4 zk7mWDU`z3R$7(Zm+m^4NtGBqyS+Y${_u;j6_q8XxY?K#7ykm*I$^Ld@PC(m2vDUwn z_^K4km9n?37yhm{r7`>m@8$(twI}c0yr^-a%K7#W9s9yuwmTG@vAFu@c-VrAs%meS z^2`s}Hl3w$-LzZji!YnyJl*Fv|Mam>g6~WB=th-3$!ITDP?%d)dg}bn`KzYih`PC0 zt35g6<&3u{y07#&=bm<6BQW1@ev@c9U#5DV+sT-ZrCkeux-mRIsUMb5^(!?@OPk9^l78)kj<$9#exYDD*ESGUa>MS-Mb<8 z-OL3#eSQ}4xf1KATzGxyx1P4Mi0wB!PL9W^cl@9JwTs+(Y4xY@!x2H3wJu)p{bXD9 z_SRJ!$C=VIn`b|+5Drm1r1$NFwpj1hb?4{U{$OUbKiBv`dH>vrIUo1;I9}Vu@Hn91 z{yZgnMy;zpuNN{^%rQ1GFfcX@JmGQn