overwrite-existing support with tests (#301)

We do our best to test all the cases, and tests discovered quite a few special cases. Directories need to be removed recursively, but when that is done it should be quite equivalent to the capabiliites of git itself.

Sebastian Thiel committed 4 years ago

Commit 49d1d341 parent 52c00585 files · +167 −83Browse files

Unified · Split

5 files

git-worktree/src/index/entry.rs +54 −7
git-worktree/src/index/mod.rs +2 −19
git-worktree/src/os.rs +12 −0
git-worktree/tests/fixtures/make_mixed_without_submodules.sh +2 −2
git-worktree/tests/index/checkout.rs +97 −55

git-worktree/src/index/entry.rs+54−7

@@ -5,8 +5,8 @@ use bstr::BStr; · 4 unmodified lines
5	5	use git_hash::oid;
6	6	use git_index::Entry;
7	7
8		−use crate::index;
9	8	use crate::index::checkout::PathCache;
	9	+use crate::{index, os};
10	10
11	11	#[cfg_attr(not(unix), allow(unused_variables))]
12	12	pub fn checkout<Find, E>(
@@ -45,14 +45,26 @@ where · 32 unmodified lines
45	45	})?;
46	46
47	47	let mut options = open_options(dest, destination_is_initially_empty, overwrite_existing);
	48	+ let needs_executable_bit = executable_bit && entry.mode == git_index::entry::Mode::FILE_EXECUTABLE;
48	49	#[cfg(unix)]
49		− if executable_bit && entry.mode == git_index::entry::Mode::FILE_EXECUTABLE {
	50	+ if needs_executable_bit && destination_is_initially_empty {
50	51	use std::os::unix::fs::OpenOptionsExt;
	52	+ // Note that these only work if the file was newly created, but won't if it's already
	53	+ // existing, possibly without the executable bit set. Thus we do this only if the file is new.
51	54	options.mode(0o777);
52	55	}
53	56
54		− let mut file = options.open(&dest)?;
	57	+ let mut file = try_write_or_unlink(dest, overwrite_existing, \|p\| options.open(p))?;
55	58	file.write_all(obj.data)?;
	59	+
	60	+ // For possibly existing, overwritten files, we must change the file mode explicitly.
	61	+ #[cfg(unix)]
	62	+ if needs_executable_bit && !destination_is_initially_empty {
	63	+ use std::os::unix::fs::PermissionsExt;
	64	+ let mut perm = std::fs::symlink_metadata(&dest)?.permissions();
	65	+ perm.set_mode(0o777);
	66	+ std::fs::set_permissions(&dest, perm)?;
	67	+ }
56	68	// NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well.
57	69	// revisit this once there is a bug to fix.
58	70	update_fstat(entry, file.metadata()?)?;
@@ -74,11 +86,14 @@ where · 15 unmodified lines
74	86	// or directories or symlinks. Doing this shouldn't invalidate the cache as it's only valid till
75	87	// our parent anyway, but it may invalidate caches in other threads without their knowledge.
76	88	// collisions with overwrite mode must be handled with great care in parallel mode.
77		− crate::os::create_symlink(symlink_destination, dest)?;
	89	+ try_write_or_unlink(dest, overwrite_existing, \|p\| {
	90	+ crate::os::create_symlink(symlink_destination, p)
	91	+ })?;
78	92	} else {
79		− open_options(dest, destination_is_initially_empty, overwrite_existing)
80		− .open(&dest)?
81		− .write_all(obj.data)?;
	93	+ try_write_or_unlink(dest, overwrite_existing, \|p\| {
	94	+ open_options(p, destination_is_initially_empty, overwrite_existing).open(&dest)
	95	+ })?
	96	+ .write_all(obj.data)?;
82	97	}
83	98
84	99	update_fstat(entry, std::fs::symlink_metadata(&dest)?)?;
@@ -91,6 +106,38 @@ where · 6 unmodified lines
91	106	Ok(object_size)
92	107	}
93	108
	109	+/// Note that this works only because we assume to not race ourselves when symlinks are involved, and we do this by
	110	+/// delaying symlink creation to the end and will always do that sequentially.
	111	+/// It's still possible to fall for a race if other actors create symlinks in our path, but that's nothing to defend against.
	112	+fn try_write_or_unlink<T>(
	113	+ path: &Path,
	114	+ overwrite_existing: bool,
	115	+ op: impl Fn(&Path) -> std::io::Result<T>,
	116	+) -> std::io::Result<T> {
	117	+ if overwrite_existing {
	118	+ match op(path) {
	119	+ Ok(res) => Ok(res),
	120	+ Err(err) if os::indicates_collision(&err) => {
	121	+ try_unlink_anything_non_recursive(path, &std::fs::symlink_metadata(path)?)?;
	122	+ op(path)
	123	+ }
	124	+ Err(err) => Err(err),
	125	+ }
	126	+ } else {
	127	+ op(path)
	128	+ }
	129	+}
	130	+
	131	+fn try_unlink_anything_non_recursive(path: &Path, path_meta: &std::fs::Metadata) -> std::io::Result<()> {
	132	+ if path_meta.is_dir() {
	133	+ std::fs::remove_dir(path)
	134	+ } else if path_meta.is_symlink() {
	135	+ symlink::remove_symlink_auto(path)
	136	+ } else {
	137	+ std::fs::remove_file(path)
	138	+ }
	139	+}
	140	+
94	141	/// This is a debug assertion as we expect the machinery calling this to prevent this possibility in the first place
95	142	fn debug_assert_dest_is_no_symlink(path: &Path) {
96	143	#[cfg(debug_assertions)]

git-worktree/src/index/mod.rs+2−19

@@ -2,8 +2,8 @@ use bstr::BStr; · 1 unmodified line
2	2	use git_features::progress::Progress;
3	3	use git_hash::oid;
4	4
5		−use crate::index;
6	5	use crate::index::checkout::{ErrorRecord, PathCache};
	6	+use crate::{index, os};
7	7
8	8	pub mod checkout;
9	9	pub(crate) mod entry;
@@ -20,10 +20,6 @@ where · 10 unmodified lines
20	20	Find: for<'a> FnMut(&oid, &'a mut Vec<u8>) -> Result<git_object::BlobRef<'a>, E>,
21	21	E: std::error::Error + Send + Sync + 'static,
22	22	{
23		− if !options.destination_is_initially_empty {
24		− todo!("deal with non-clone checkouts")
25		− }
26		−
27	23	let mut path_cache = PathCache::new(dir.into());
28	24	path_cache.unlink_on_collision = options.overwrite_existing;
29	25
@@ -103,24 +99,11 @@ where · 73 unmodified lines
103	99	P1: Progress,
104	100	P2: Progress,
105	101	{
106		− use std::io::ErrorKind::AlreadyExists;
107		−
108	102	let res = entry::checkout(entry, entry_path, find, path_cache, *options, buf);
109	103	files.inc();
110	104	match res {
111	105	Ok(object_size) => bytes.inc_by(object_size),
112		− #[cfg(windows)]
113		− Err(index::checkout::Error::Io(err))
114		− if err.kind() == AlreadyExists \|\| err.kind() == std::io::ErrorKind::PermissionDenied =>
115		− {
116		− collisions.push(checkout::Collision {
117		− path: entry_path.into(),
118		− error_kind: err.kind(),
119		− });
120		− }
121		− // TODO: use ::IsDirectory as well when stabilized instead of raw_os_error()
122		− #[cfg(not(windows))]
123		− Err(index::checkout::Error::Io(err)) if err.kind() == AlreadyExists \|\| err.raw_os_error() == Some(21) => {
	106	+ Err(index::checkout::Error::Io(err)) if os::indicates_collision(&err) => {
124	107	// We are here because a file existed or was blocked by a directory which shouldn't be possible unless
125	108	// we are on a file insensitive file system.
126	109	files.fail(format!("{}: collided ({:?})", entry_path, err.kind()));

git-worktree/src/os.rs+12−0

@@ -1,4 +1,5 @@
1	1	use std::io;
	2	+use std::io::ErrorKind::AlreadyExists;
2	3	use std::path::Path;
3	4
4	5	#[cfg(not(windows))]
@@ -26,3 +27,14 @@ pub fn create_symlink(original: &Path, link: &Path) -> io::Result<()> { · 21 unmodified lines
26	27	symlink_file(original, link)
27	28	}
28	29	}
	30	+
	31	+#[cfg(not(windows))]
	32	+pub fn indicates_collision(err: &std::io::Error) -> bool {
	33	+ // TODO: use ::IsDirectory as well when stabilized instead of raw_os_error(), and ::FileSystemLoop respectively
	34	+ err.kind() == AlreadyExists \|\| err.raw_os_error() == Some(21) \|\| err.raw_os_error() == Some(62)
	35	+}
	36	+
	37	+#[cfg(windows)]
	38	+pub fn indicates_collision(err: &std::io::Error) -> bool {
	39	+ err.kind() == AlreadyExists \|\| err.kind() == std::io::ErrorKind::PermissionDenied
	40	+}

git-worktree/tests/fixtures/make_mixed_without_submodules.sh+2−2

@@ -5,11 +5,11 @@ git init -q · 4 unmodified lines
5	5	git config commit.gpgsign false
6	6
7	7	touch empty
8		−echo "content" > executable
	8	+echo -n "content" > executable
9	9	chmod +x executable
10	10
11	11	mkdir dir
12		−echo "other content" > dir/content
	12	+echo -n "other content" > dir/content
13	13	mkdir dir/sub-dir
14	14	(cd dir/sub-dir && ln -sf ../content symlink)
15	15

git-worktree/tests/index/checkout.rs+97−55

@@ -116,7 +116,7 @@ use crate::fixture_path; · 115 unmodified lines
116	116
117	117	#[test]
118	118	fn accidental_writes_through_symlinks_are_prevented_if_overwriting_is_forbidden() {
119		− let mut opts = opts_with_symlink(true);
	119	+ let mut opts = opts_from_probe();
120	120	// without overwrite mode, everything is safe.
121	121	opts.overwrite_existing = false;
122	122	let (source_tree, destination, _index, outcome) =
@@ -125,8 +125,7 @@ fn accidental_writes_through_symlinks_are_prevented_if_overwriting_is_forbidden( · 2 unmodified lines
125	125	let source_files = dir_structure(&source_tree);
126	126	let worktree_files = dir_structure(&destination);
127	127
128		− let fs_caps = probe_gitoxide_dir().unwrap();
129		− if fs_caps.ignore_case {
	128	+ if opts.fs.ignore_case {
130	129	assert_eq!(
131	130	stripped_prefix(&source_tree, &source_files),
132	131	stripped_prefix(&destination, &worktree_files),
@@ -153,9 +152,9 @@ fn accidental_writes_through_symlinks_are_prevented_if_overwriting_is_forbidden( · 20 unmodified lines
153	152	}
154	153
155	154	#[test]
156		−#[ignore]
	155	+#[ignore] // TODO: needs recursive directory deletion
157	156	fn writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed() {
158		− let mut opts = opts_with_symlink(true);
	157	+ let mut opts = opts_from_probe();
159	158	// with overwrite mode
160	159	opts.overwrite_existing = true;
161	160	let (source_tree, destination, _index, outcome) =
@@ -164,29 +163,12 @@ fn writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed() { · 2 unmodified lines
164	163	let source_files = dir_structure(&source_tree);
165	164	let worktree_files = dir_structure(&destination);
166	165
167		− let fs_caps = probe_gitoxide_dir().unwrap();
168		− if fs_caps.ignore_case {
	166	+ if opts.fs.ignore_case {
169	167	assert_eq!(
170	168	stripped_prefix(&source_tree, &source_files),
171		− paths(["A-dir/a", "A-file", "fake-dir/b", "fake-file"])
172		− );
173		− assert_eq!(
174	169	stripped_prefix(&destination, &worktree_files),
175		− paths(["A-dir/a", "A-file", "FAKE-DIR", "FAKE-FILE"])
176		− );
177		− assert_eq!(
178		− outcome.collisions,
179		− vec![
180		− Collision {
181		− path: "fake-dir/b".into(),
182		− error_kind: AlreadyExists
183		− },
184		− Collision {
185		− path: "fake-file".into(),
186		− error_kind: AlreadyExists
187		− }
188		− ]
189	170	);
	171	+ assert!(outcome.collisions.is_empty());
190	172	} else {
191	173	let expected = ["A-dir/a", "A-file", "FAKE-DIR", "FAKE-FILE", "fake-dir/b", "fake-file"];
192	174	assert_eq!(stripped_prefix(&source_tree, &source_files), paths(expected));
@@ -196,23 +178,69 @@ fn writes_through_symlinks_are_prevented_even_if_overwriting_is_allowed() { · 3 unmodified lines
196	178	}
197	179
198	180	#[test]
199		−fn symlinks_become_files_if_disabled() -> crate::Result {
200		− let opts = opts_with_symlink(false);
201		− let (source_tree, destination, _index, outcome) = checkout_index_in_tmp_dir(opts, "make_mixed_without_submodules")?;
	181	+fn overwriting_files_and_lone_directories_works() {
	182	+ let mut opts = opts_from_probe();
	183	+ opts.overwrite_existing = true;
	184	+ opts.destination_is_initially_empty = false;
	185	+ let (_source_tree, destination, _index, outcome) = checkout_index_in_tmp_dir_opts(
	186	+ opts,
	187	+ "make_mixed_without_submodules",
	188	+ \|_\| true,
	189	+ \|d\| {
	190	+ let empty = d.join("empty");
	191	+ symlink::symlink_dir(d.join(".."), &empty)?; // empty is symlink to the directory above
	192	+ std::fs::write(d.join("executable"), b"foo")?; // executable is regular file and has different content
	193	+ let dir = d.join("dir");
	194	+ std::fs::create_dir(&dir)?;
	195	+ std::fs::create_dir(dir.join("content"))?; // 'content' is a directory now
	196	+
	197	+ let dir = dir.join("sub-dir");
	198	+ std::fs::create_dir(&dir)?;
	199	+
	200	+ symlink::symlink_dir(empty, dir.join("symlink"))?; // 'symlink' is a symlink to another file
	201	+ Ok(())
	202	+ },
	203	+ )
	204	+ .unwrap();
202	205
203		− assert_equality(&source_tree, &destination, opts.fs.symlink)?;
204	206	assert!(outcome.collisions.is_empty());
205		− Ok(())
	207	+
	208	+ assert_eq!(
	209	+ stripped_prefix(&destination, &dir_structure(&destination)),
	210	+ paths(["dir/content", "dir/sub-dir/symlink", "empty", "executable"])
	211	+ );
	212	+ let meta = std::fs::symlink_metadata(destination.path().join("empty")).unwrap();
	213	+ assert!(meta.is_file(), "'empty' is now a file");
	214	+ assert_eq!(meta.len(), 0, "'empty' is indeed empty");
	215	+
	216	+ let exe = destination.path().join("executable");
	217	+ assert_eq!(
	218	+ std::fs::read(&exe).unwrap(),
	219	+ b"content",
	220	+ "'exe' has the correct content"
	221	+ );
	222	+
	223	+ let meta = std::fs::symlink_metadata(exe).unwrap();
	224	+ assert!(meta.is_file());
	225	+ if opts.fs.executable_bit {
	226	+ #[cfg(unix)]
	227	+ assert_eq!(meta.mode() & 0o700, 0o700, "the executable bit is set where supported");
	228	+ }
	229	+
	230	+ assert_eq!(
	231	+ std::fs::read(destination.path().join("dir/content")).unwrap(),
	232	+ b"other content"
	233	+ );
	234	+
	235	+ let symlink = destination.path().join("dir/sub-dir/symlink");
	236	+ assert!(std::fs::symlink_metadata(&symlink).unwrap().is_symlink(),);
	237	+ assert_eq!(std::fs::read(symlink).unwrap(), b"other content");
206	238	}
207	239
208	240	#[test]
209		−fn allow_symlinks() -> crate::Result {
210		− let opts = opts_with_symlink(true);
211		− if !probe_gitoxide_dir()?.symlink {
212		− eprintln!("IGNORING symlink test on file system without symlink support");
213		− // skip if symlinks aren't supported anyway.
214		− return Ok(());
215		− };
	241	+fn symlinks_become_files_if_disabled() -> crate::Result {
	242	+ let mut opts = opts_from_probe();
	243	+ opts.fs.symlink = false;
216	244	let (source_tree, destination, _index, outcome) = checkout_index_in_tmp_dir(opts, "make_mixed_without_submodules")?;
217	245
218	246	assert_equality(&source_tree, &destination, opts.fs.symlink)?;
@@ -220,21 +248,39 @@ fn allow_symlinks() -> crate::Result { · 1 unmodified line
220	248	Ok(())
221	249	}
222	250
	251	+#[test]
	252	+fn allow_or_disallow_symlinks() -> crate::Result {
	253	+ let mut opts = opts_from_probe();
	254	+ for allowed in &[false, true] {
	255	+ opts.fs.symlink = *allowed;
	256	+ let (source_tree, destination, _index, outcome) =
	257	+ checkout_index_in_tmp_dir(opts, "make_mixed_without_submodules")?;
	258	+
	259	+ assert_equality(&source_tree, &destination, opts.fs.symlink)?;
	260	+ assert!(outcome.collisions.is_empty());
	261	+ }
	262	+ Ok(())
	263	+}
	264	+
223	265	#[test]
224	266	fn keep_going_collects_results() {
225		− let mut opts = opts_with_symlink(true);
	267	+ let mut opts = opts_from_probe();
226	268	opts.keep_going = true;
227	269	let mut count = 0;
228		− let (_source_tree, destination, _index, outcome) =
229		− checkout_index_in_tmp_dir_prep_dest(opts, "make_mixed_without_submodules", \|_id\| {
	270	+ let (_source_tree, destination, _index, outcome) = checkout_index_in_tmp_dir_opts(
	271	+ opts,
	272	+ "make_mixed_without_submodules",
	273	+ \|_id\| {
230	274	if count < 2 {
231	275	count += 1;
232	276	false
233	277	} else {
234	278	true
235	279	}
236		− })
237		− .unwrap();
	280	+ },
	281	+ \|_\| Ok(()),
	282	+ )
	283	+ .unwrap();
238	284
239	285	assert_eq!(
240	286	stripped_prefix(&destination, &dir_structure(&destination)),
@@ -255,13 +301,11 @@ fn keep_going_collects_results() { · 14 unmodified lines
255	301
256	302	#[test]
257	303	fn no_case_related_collisions_on_case_sensitive_filesystem() {
258		− let fs_caps = probe_gitoxide_dir().unwrap();
259		− if fs_caps.ignore_case {
	304	+ let opts = opts_from_probe();
	305	+ if opts.fs.ignore_case {
260	306	eprintln!("Skipping case-sensitive testing on what would be a case-insensitive file system");
261	307	return;
262	308	}
263		− let mut opts = opts_with_symlink(true);
264		− opts.fs = fs_caps;
265	309	let (source_tree, destination, index, outcome) =
266	310	checkout_index_in_tmp_dir(opts, "make_ignorecase_collisions").unwrap();
267	311
@@ -272,12 +316,11 @@ fn no_case_related_collisions_on_case_sensitive_filesystem() { · 4 unmodified lines
272	316
273	317	#[test]
274	318	fn collisions_are_detected_on_a_case_sensitive_filesystem() {
275		− let fs_caps = probe_gitoxide_dir().unwrap();
276		− if !fs_caps.ignore_case {
	319	+ let opts = opts_from_probe();
	320	+ if !opts.fs.ignore_case {
277	321	eprintln!("Skipping case-insensitive testing on what would be a case-senstive file system");
278	322	return;
279	323	}
280		− let opts = opts_with_symlink(fs_caps.symlink);
281	324	let (source_tree, destination, _index, outcome) =
282	325	checkout_index_in_tmp_dir(opts, "make_ignorecase_collisions").unwrap();
283	326
@@ -379,13 +422,14 @@ fn checkout_index_in_tmp_dir( · 95 unmodified lines
379	422	git_index::File,
380	423	git_worktree::index::checkout::Outcome,
381	424	)> {
382		− checkout_index_in_tmp_dir_prep_dest(opts, name, \|_d\| true)
	425	+ checkout_index_in_tmp_dir_opts(opts, name, \|_d\| true, \|_\| Ok(()))
383	426	}
384	427
385		−fn checkout_index_in_tmp_dir_prep_dest(
	428	+fn checkout_index_in_tmp_dir_opts(
386	429	opts: index::checkout::Options,
387	430	name: &str,
388	431	mut allow_return_object: impl FnMut(&git_hash::oid) -> bool,
	432	+ prep_dest: impl Fn(&Path) -> std::io::Result<()>,
389	433	) -> crate::Result<(
390	434	PathBuf,
391	435	TempDir,
@@ -397,6 +441,7 @@ fn checkout_index_in_tmp_dir_prep_dest( · 5 unmodified lines
397	441	let mut index = git_index::File::at(git_dir.join("index"), Default::default())?;
398	442	let odb = git_odb::at(git_dir.join("objects"))?;
399	443	let destination = tempfile::tempdir_in(std::env::current_dir()?)?;
	444	+ prep_dest(destination.path())?;
400	445
401	446	let outcome = index::checkout(
402	447	&mut index,
@@ -425,12 +470,9 @@ fn probe_gitoxide_dir() -> crate::Result<Capabilities> { · 22 unmodified lines
425	470	))
426	471	}
427	472
428		−fn opts_with_symlink(symlink: bool) -> index::checkout::Options {
	473	+fn opts_from_probe() -> index::checkout::Options {
429	474	index::checkout::Options {
430		− fs: git_worktree::fs::Capabilities {
431		− symlink,
432		− ..Default::default()
433		− },
	475	+ fs: probe_gitoxide_dir().unwrap(),
434	476	destination_is_initially_empty: true,
435	477	..Default::default()
436	478	}

overwrite-existing support with tests (#301)

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code