proper reflog entries for all other updated refs during cloning (#450)

Sebastian Thiel committed 4 years ago

Commit ff4412e1 parent c25cb007 files · +119 −48Browse files

Unified · Split

7 files

git-repository/src/clone/fetch.rs +9 −4
git-repository/src/remote/connection/fetch/error.rs +31 −0
git-repository/src/remote/connection/fetch/mod.rs +35 −33
git-repository/src/remote/connection/fetch/update_refs/mod.rs +3 −2
git-repository/src/remote/connection/fetch/update_refs/tests.rs +30 −7
git-repository/src/remote/fetch.rs +1 −1
git-repository/tests/clone/mod.rs +10 −1

         P::SubProgress: 'static,
     {
         use crate::bstr::{BStr, ByteVec};
+        use crate::remote::fetch::RefLogMessage;
         use git_ref::transaction::{LogChange, RefLog};
         fn write_remote_to_local_config_file(
             remote: &mut crate::Remote<'_>,
             remote_name: String,
         if pending_pack.ref_map().object_hash != repo.object_hash() {
             unimplemented!("configure repository to expect a different object hash as advertised by the server")
         }
-        let outcome = pending_pack.receive(should_interrupt)?;
-        replace_changed_local_config_file(repo, config);
         let reflog_message = {
             let mut b = self.url.to_bstring();
             b.insert_str(0, "clone: from ");
             b
         };
+        let outcome = pending_pack
+            .with_reflog_message(RefLogMessage::Override {
+                message: reflog_message.clone(),
+            })
+            .receive(should_interrupt)?;
+        replace_changed_local_config_file(repo, config);
         update_head(repo, &outcome.ref_map.remote_refs, reflog_message.as_ref())?;
         Ok((self.repo.take().expect("still present"), outcome))

git-repository/src/remote/connection/fetch/error.rs+31−0

@@ -0,0 +1,31 @@
	1	/// The error returned by [`receive()`](super::Prepare::receive()).
	2	#[derive(Debug, thiserror::Error)]
	3	#[allow(missing_docs)]
	4	pub enum Error {
	5	#[error("{message}{}", desired.map(\|n\| format!(" (got {})", n)).unwrap_or_default())]
	6	Configuration {
	7	message: &'static str,
	8	desired: Option<i64>,
	9	source: Option<git_config::value::Error>,
	10	},
	11	#[error("Could not decode server reply")]
	12	FetchResponse(#[from] git_protocol::fetch::response::Error),
	13	#[error("Cannot fetch from a remote that uses {remote} while local repository uses {local} for object hashes")]
	14	IncompatibleObjectHash {
	15	local: git_hash::Kind,
	16	remote: git_hash::Kind,
	17	},
	18	#[error(transparent)]
	19	Negotiate(#[from] super::negotiate::Error),
	20	#[error(transparent)]
	21	Client(#[from] git_protocol::transport::client::Error),
	22	#[error(transparent)]
	23	WritePack(#[from] git_pack::bundle::write::Error),
	24	#[error(transparent)]
	25	UpdateRefs(#[from] super::refs::update::Error),
	26	#[error("Failed to remove .keep file at \"{}\"", path.display())]
	27	RemovePackKeepFile {
	28	path: std::path::PathBuf,
	29	source: std::io::Error,
	30	},
	31	}

git-repository/src/remote/connection/fetch/mod.rs+35−33

@@ -1,5 +1,6 @@
1	use std::sync::atomic::AtomicBool;	1	use std::sync::atomic::AtomicBool;
2		2
		3	use crate::bstr::BString;
3	use git_odb::FindExt;	4	use git_odb::FindExt;
4	use git_protocol::transport::client::Transport;	5	use git_protocol::transport::client::Transport;
5		6
@@ -13,40 +14,31 @@ use crate::{ · 7 unmodified lines
13	Progress,	14	Progress,
14	};	15	};
15		16
16	mod error {	17	mod error;
17	/// The error returned by [`receive()`](super::Prepare::receive()).	18	pub use error::Error;
18	#[derive(Debug, thiserror::Error)]	19
19	#[allow(missing_docs)]	20	/// The way reflog messages should be composed whenever a ref is written with recent objects from a remote.
20	pub enum Error {	21	pub enum RefLogMessage {
21	#[error("{message}{}", desired.map(\|n\| format!(" (got {})", n)).unwrap_or_default())]	22	/// Prefix the log with `action` and generate the typical suffix as `git` would.
22	Configuration {	23	Prefixed {
23	message: &'static str,	24	/// The action to use, like `fetch` or `pull`.
24	desired: Option<i64>,	25	action: String,
25	source: Option<git_config::value::Error>,	26	},
26	},	27	/// Control the entire message, using `message` verbatim.
27	#[error("Could not decode server reply")]	28	Override {
28	FetchResponse(#[from] git_protocol::fetch::response::Error),	29	/// The complete reflog message.
29	#[error("Cannot fetch from a remote that uses {remote} while local repository uses {local} for object hashes")]	30	message: BString,
30	IncompatibleObjectHash {	31	},
31	local: git_hash::Kind,	32	}
32	remote: git_hash::Kind,	33
33	},	34	impl RefLogMessage {
34	#[error(transparent)]	35	pub(crate) fn compose(&self, context: &str) -> BString {
35	Negotiate(#[from] super::negotiate::Error),	36	match self {
36	#[error(transparent)]	37	RefLogMessage::Prefixed { action } => format!("{}: {}", action, context).into(),
37	Client(#[from] git_protocol::transport::client::Error),	38	RefLogMessage::Override { message } => message.to_owned(),
38	#[error(transparent)]	39	}
39	WritePack(#[from] git_pack::bundle::write::Error),
40	#[error(transparent)]
41	UpdateRefs(#[from] super::refs::update::Error),
42	#[error("Failed to remove .keep file at \"{}\"", path.display())]
43	RemovePackKeepFile {
44	path: std::path::PathBuf,
45	source: std::io::Error,
46	},
47	}	40	}
48	}	41	}
49	pub use error::Error;
50		42
51	/// The status of the repository after the fetch operation	43	/// The status of the repository after the fetch operation
52	#[derive(Debug, Clone)]	44	#[derive(Debug, Clone)]
@@ -118,6 +110,7 @@ where · 65 unmodified lines
118	con: Some(self),	110	con: Some(self),
119	ref_map,	111	ref_map,
120	dry_run: DryRun::No,	112	dry_run: DryRun::No,
		113	reflog_message: None,
121	})	114	})
122	}	115	}
123	}	116	}
@@ -267,7 +260,9 @@ where · 143 unmodified lines
267		260
268	let update_refs = refs::update(	261	let update_refs = refs::update(
269	repo,	262	repo,
270	"fetch",	263	self.reflog_message
		264	.take()
		265	.unwrap_or_else(\|\| RefLogMessage::Prefixed { action: "fetch".into() }),
271	&self.ref_map.mappings,	266	&self.ref_map.mappings,
272	con.remote.refspecs(remote::Direction::Fetch),	267	con.remote.refspecs(remote::Direction::Fetch),
273	self.dry_run,	268	self.dry_run,
@@ -323,6 +318,7 @@ where · 49 unmodified lines
323	con: Option<Connection<'remote, 'repo, T, P>>,	318	con: Option<Connection<'remote, 'repo, T, P>>,
324	ref_map: RefMap,	319	ref_map: RefMap,
325	dry_run: DryRun,	320	dry_run: DryRun,
		321	reflog_message: Option<RefLogMessage>,
326	}	322	}
327		323
328	/// Builder	324	/// Builder
@@ -337,6 +333,12 @@ where · 8 unmodified lines
337	self.dry_run = enabled.then(\|\| DryRun::Yes).unwrap_or(DryRun::No);	333	self.dry_run = enabled.then(\|\| DryRun::Yes).unwrap_or(DryRun::No);
338	self	334	self
339	}	335	}
		336
		337	/// Set the reflog message to use when updating refs after fetching a pack.
		338	pub fn with_reflog_message(mut self, reflog_message: RefLogMessage) -> Self {
		339	self.reflog_message = reflog_message.into();
		340	self
		341	}
340	}	342	}
341		343
342	impl<'remote, 'repo, T, P> Drop for Prepare<'remote, 'repo, T, P>	344	impl<'remote, 'repo, T, P> Drop for Prepare<'remote, 'repo, T, P>

git-repository/src/remote/connection/fetch/update_refs/mod.rs+3−2

     Target, TargetRef,
 };
+use crate::remote::fetch::RefLogMessage;
 use crate::{
     ext::ObjectIdExt,
     remote::{fetch, fetch::refs::update::Mode},
 /// It can be used to produce typical information that one is used to from `git fetch`.
 pub(crate) fn update(
     repo: &Repository,
-    action: &str,
+    message: RefLogMessage,
     mappings: &[fetch::Mapping],
     refspecs: &[git_refspec::RefSpec],
     dry_run: fetch::DryRun,
                         log: LogChange {
                             mode: RefLog::AndReference,
                             force_create_reflog: false,
-                            message: format!("{}: {}", action, reflog_message).into(),
+                            message: message.compose(reflog_message),
                         },
                         expected: previous_value,
                         new: Target::Peeled(remote_id.into()),

git-repository/src/remote/connection/fetch/update_refs/tests.rs+30−7

         let repo = git::open_opts(dir.path().join(name), git::open::Options::isolated()).unwrap();
         (repo, dir)
     }
+    use crate::bstr::BString;
     use git_ref::{transaction::Change, TargetRef};
     use crate::remote::fetch;
+    use crate::remote::fetch::RefLogMessage;
     #[test]
     fn various_valid_updates() {
             let (mapping, specs) = mapping_from_spec(spec, &repo);
             let out = fetch::refs::update(
                 &repo,
-                "action",
+                prefixed("action"),
                 &mapping,
                 &specs,
                 reflog_message.map(|_| fetch::DryRun::Yes).unwrap_or(fetch::DryRun::No),
         ] {
             let spec = format!("refs/heads/main:refs/heads/{}", branch);
             let (mappings, specs) = mapping_from_spec(&spec, &repo);
-            let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::Yes)?;
+            let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::Yes)?;
             assert_eq!(
                 out.updates,
     fn symbolic_refs_are_never_written() {
         let repo = repo("two-origins");
         let (mappings, specs) = mapping_from_spec("refs/heads/main:refs/heads/symbolic", &repo);
-        let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::Yes).unwrap();
+        let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::Yes).unwrap();
         assert_eq!(
             out.updates,
     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();
+        let reflog_message: BString = "very special".into();
+        let out = fetch::refs::update(
+            &repo,
+            RefLogMessage::Override {
+                message: reflog_message.clone(),
+            },
+            &mappings,
+            &specs,
+            fetch::DryRun::Yes,
+        )
+        .unwrap();
         assert_eq!(
             out.updates,
             "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);
+        let edit = &out.edits[0];
+        match &edit.change {
+            Change::Update { log, .. } => {
+                assert_eq!(log.message, reflog_message);
+            }
+            _ => unreachable!("only updates"),
+        }
     }
     #[test]
     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/remotes/origin/g:refs/heads/not-currently-checked-out", &repo);
-        let out = fetch::refs::update(&repo, "action", &mappings, &specs, fetch::DryRun::No).unwrap();
+        let out = fetch::refs::update(&repo, prefixed("action"), &mappings, &specs, fetch::DryRun::No).unwrap();
         assert_eq!(
             out.updates,
         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();
+        let out = fetch::refs::update(&repo, prefixed("prefix"), &mappings, &specs, fetch::DryRun::No).unwrap();
         assert_eq!(
             out.updates,
     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();
+        let out = fetch::refs::update(&repo, prefixed("prefix"), &mappings, &specs, fetch::DryRun::No).unwrap();
         assert_eq!(
             out.updates,
             object,
         }
     }
+    fn prefixed(action: &str) -> RefLogMessage {
+        RefLogMessage::Prefixed { action: action.into() }
+    }
 }

git-repository/src/remote/fetch.rs+1−1

@@ -60,4 +60,4 @@ pub struct Mapping { · 59 unmodified lines
60	}	60	}
61		61
62	#[cfg(feature = "blocking-network-client")]	62	#[cfg(feature = "blocking-network-client")]
63	pub use super::connection::fetch::{negotiate, prepare, refs, Error, Outcome, Prepare, Status};	63	pub use super::connection::fetch::{negotiate, prepare, refs, Error, Outcome, Prepare, RefLogMessage, Status};

git-repository/tests/clone/mod.rs+10−1

                         repo.objects.contains(edit.change.new_value().expect("always set").id()),
                         "part of the fetched pack"
                     );
+                    let r = repo.find_reference(edit.name.as_ref()).expect("created");
+                    if r.name().category().expect("known") != git_ref::Category::Tag {
+                        let mut logs = r.log_iter();
+                        assert_reflog(logs.all());
+                    }
                 }
             }
             _ => unreachable!("clones are always causing changes and dry-runs aren't possible"),
             .unwrap();
         assert_eq!(logs.len(), 1, "just created");
         let line = &logs[0];
-        assert!(line.message.starts_with(b"clone: from "), "{:?} unexpected", line);
+        assert!(
+            line.message.starts_with(b"clone: from "),
+            "{:?} unexpected",
+            line.message
+        );
         let path = git_path::from_bstr(line.message.rsplit(|b| *b == b' ').next().expect("path").as_bstr());
         assert!(path.is_absolute(), "{:?} must be absolute", path);
     }