Skip to content

Commit 8606b7a

Browse files
codexByron
codex
and
Byron
committed
review
Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
1 parent e75fbfa commit 8606b7a

10 files changed

Lines changed: 180 additions & 73 deletions

File tree

‎gix/src/clone/access.rs‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,7 @@ impl PrepareFetch {
7373
impl Drop for PrepareFetch {
7474
fn drop(&mut self) {
7575
if let Some(repo) = self.repo.take() {
76-
if self.destination_was_empty {
77-
std::fs::remove_dir_all(repo.workdir().unwrap_or_else(|| repo.path())).ok();
78-
} else {
79-
// The destination held pre-existing user files; only remove the `.git` we created.
80-
std::fs::remove_dir_all(repo.path()).ok();
81-
}
76+
super::cleanup_clone_destination_on_drop(&repo, self.remove_worktree_on_drop);
8277
}
8378
}
8479
}

‎gix/src/clone/checkout.rs‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,7 @@ impl PrepareCheckout {
171171
impl Drop for PrepareCheckout {
172172
fn drop(&mut self) {
173173
if let Some(repo) = self.repo.take() {
174-
if self.destination_was_empty {
175-
std::fs::remove_dir_all(repo.workdir().unwrap_or_else(|| repo.path())).ok();
176-
} else {
177-
// The destination held pre-existing user files; only remove the `.git` we created.
178-
std::fs::remove_dir_all(repo.path()).ok();
179-
}
174+
super::cleanup_clone_destination_on_drop(&repo, self.remove_worktree_on_drop);
180175
}
181176
}
182177
}

‎gix/src/clone/fetch/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ impl PrepareFetch {
318318
crate::clone::PrepareCheckout {
319319
repo: repo.into(),
320320
ref_name: self.ref_name.clone(),
321-
destination_was_empty: self.destination_was_empty,
321+
remove_worktree_on_drop: self.remove_worktree_on_drop,
322322
},
323323
fetch_outcome,
324324
))

‎gix/src/clone/mod.rs‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,8 @@ pub struct PrepareFetch {
4242
/// The name of the reference to fetch. If `None`, the reference pointed to by `HEAD` will be checked out.
4343
#[cfg_attr(not(feature = "blocking-network-client"), allow(dead_code))]
4444
ref_name: Option<gix_ref::PartialName>,
45-
/// `true` iff the destination directory was empty (or did not exist) when this handle was created.
46-
/// When `false`, drop will avoid removing pre-existing user files and only clean up the `.git` we created.
47-
destination_was_empty: bool,
45+
/// If `true`, drop removes the entire worktree. Otherwise leave it alone.
46+
remove_worktree_on_drop: bool,
4847
}
4948

5049
/// The error returned by [`PrepareFetch::new()`].
@@ -110,10 +109,12 @@ impl PrepareFetch {
110109
}
111110

112111
// Capture this before init_opts creates `.git`, otherwise the check below would see our own files.
113-
let destination_was_empty = match std::fs::read_dir(path) {
112+
let remove_worktree_on_drop = match std::fs::read_dir(path) {
114113
Ok(mut entries) => entries.next().is_none(),
115-
// Non-existent (or unreadable) — init_opts will create the directory.
116-
Err(_) => true,
114+
// Non-existent destinations will be created by init_opts.
115+
Err(err) if err.kind() == std::io::ErrorKind::NotFound => true,
116+
// If we can't verify emptiness, keep cleanup conservative and leave the destination untouched.
117+
Err(_) => false,
117118
};
118119

119120
let mut repo = crate::ThreadSafeRepository::init_opts(path, kind, create_opts, open_opts)?.to_thread_local();
@@ -135,7 +136,7 @@ impl PrepareFetch {
135136
configure_connection: None,
136137
shallow: remote::fetch::Shallow::NoChange,
137138
ref_name: None,
138-
destination_was_empty,
139+
remove_worktree_on_drop,
139140
})
140141
}
141142
}
@@ -150,9 +151,21 @@ pub struct PrepareCheckout {
150151
pub(self) repo: Option<crate::Repository>,
151152
/// The name of the reference to check out. If `None`, the reference pointed to by `HEAD` will be checked out.
152153
pub(self) ref_name: Option<gix_ref::PartialName>,
153-
/// `true` iff the destination directory was empty (or did not exist) when the parent [`PrepareFetch`] was created.
154-
/// When `false`, drop must avoid removing pre-existing user files and only clean up the `.git` we created.
155-
pub(self) destination_was_empty: bool,
154+
/// If `true`, drop removes the entire worktree. Otherwise leave it alone.
155+
pub(self) remove_worktree_on_drop: bool,
156+
}
157+
158+
fn cleanup_clone_destination_on_drop(repo: &crate::Repository, remove_worktree_on_drop: bool) {
159+
let path_to_remove = if remove_worktree_on_drop {
160+
Some(repo.workdir().unwrap_or_else(|| repo.path()))
161+
} else {
162+
// The destination held pre-existing user files. Leave everything, including the `.git` we created,
163+
// so the user can inspect or clean up the partially cloned repository with Git tooling.
164+
None
165+
};
166+
if let Some(path_to_remove) = path_to_remove {
167+
std::fs::remove_dir_all(path_to_remove).ok();
168+
}
156169
}
157170

158171
// This module encapsulates functionality that works with both feature toggles. Can be combined with `fetch`

‎gix/src/create.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,20 @@ fn create_dir(p: &Path) -> Result<(), Error> {
108108
}
109109

110110
/// Options for use in [`into()`];
111-
#[derive(Copy, Clone, Default)]
111+
#[derive(Copy, Default, Clone)]
112112
pub struct Options {
113113
/// Control whether the destination directory must be empty when creating a repository with a worktree.
114114
///
115-
/// - `None` (default): worktree repos may be initialized into a non-empty directory as long as no `.git`
116-
/// directory is present. [`crate::clone::PrepareFetch::new`] interprets `None` as `Some(true)` to preserve
117-
/// the historical strict-by-default behavior for clones.
115+
/// - `None` (default): initialize like Git and allow a non-empty destination directory, as long as no `.git`
116+
/// directory is present.
118117
/// - `Some(true)`: require an empty destination directory.
119-
/// - `Some(false)`: explicitly allow initialization into a non-empty destination directory (still requires
120-
/// that no `.git` directory is present).
118+
/// - `Some(false)`: explicitly allow initialization into a non-empty destination directory (still requires that no
119+
/// `.git` directory is present).
120+
///
121+
/// For clones, checkout failure cleanup is based on whether the destination was already present and non-empty before
122+
/// initialization began, not on this option alone. In particular, if the destination was empty or had to be created,
123+
/// cleanup may remove the entire destination, including the created `.git` directory. Preservation of the destination
124+
/// for inspection or manual cleanup is only guaranteed when the destination was non-empty before the clone started.
121125
///
122126
/// Bare repositories always require an empty destination, regardless of this option.
123127
pub destination_must_be_empty: Option<bool>,

‎gix/src/init.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,10 @@ pub enum Error {
4242
impl ThreadSafeRepository {
4343
/// Create a repository with work-tree within `directory`, creating intermediate directories as needed.
4444
///
45-
/// Fails without action if there is already a `.git` repository inside of `directory`, but
46-
/// won't mind if the `directory` otherwise is non-empty.
45+
/// Fails without action if the destination directory isn't empty unless
46+
/// [`create::Options::destination_must_be_empty`][crate::create::Options::destination_must_be_empty] is `None`
47+
/// or `Some(false)`. Note that initialization still fails if a `.git` directory already exists in
48+
/// the destination.
4749
pub fn init(
4850
directory: impl AsRef<Path>,
4951
kind: crate::create::Kind,

‎gix/tests/fixtures/generated-archives/make_clone_destinations.tar‎

10 KB
Binary file not shown.

‎gix/tests/fixtures/make_clone_destinations.sh‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
#!/usr/bin/env bash
2+
set -eu -o pipefail
3+
4+
mkdir non-empty
5+
printf "Pre-existing user content.\n" > non-empty/existing.txt
6+
7+
mkdir non-empty-with-conflicting-file
8+
printf "Pre-existing user content.\n" > non-empty-with-conflicting-file/file
9+
10+
cp -R non-empty non-empty-with-dot-git
11+
mkdir non-empty-with-dot-git/.git
12+
printf "ref: refs/heads/pre-existing\n" > non-empty-with-dot-git/.git/HEAD

‎gix/tests/gix/clone.rs‎

Lines changed: 102 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ mod blocking_io {
2020
use gix_ref::TargetRef;
2121
use gix_refspec::parse::Operation;
2222

23+
const EXISTING_CONTENT: &[u8] = b"Pre-existing user content.\n";
24+
const EXISTING_HEAD_CONTENT: &[u8] = b"ref: refs/heads/pre-existing\n";
25+
2326
fn shallow_ids(repo: &gix::Repository, expected: &'static str) -> crate::Result<Vec<gix::ObjectId>> {
2427
let commits = repo.shallow_commits()?.expect(expected);
2528
Ok(std::iter::once(commits.head)
@@ -587,14 +590,13 @@ mod blocking_io {
587590

588591
#[test]
589592
fn fetch_and_checkout_into_non_empty_directory() -> crate::Result {
590-
let tmp = gix_testtools::tempfile::TempDir::new()?;
591-
let existing_path = tmp.path().join("existing.txt");
592-
let existing_content = b"I was here before you";
593-
std::fs::write(&existing_path, existing_content)?;
593+
let fixture = gix_testtools::scripted_fixture_writable("make_clone_destinations.sh")?;
594+
let destination = fixture.path().join("non-empty");
595+
let existing_path = destination.join("existing.txt");
594596

595597
let mut prepare = gix::clone::PrepareFetch::new(
596598
remote::repo("base").path(),
597-
tmp.path(),
599+
&destination,
598600
gix::create::Kind::WithWorktree,
599601
gix::create::Options {
600602
destination_must_be_empty: Some(false),
@@ -610,20 +612,94 @@ mod blocking_io {
610612
assert_eq!(index.entries().len(), 1, "All entries are known as per HEAD tree");
611613
assure_index_entries_on_disk(&index, repo.workdir().expect("non-bare"));
612614

613-
assert_eq!(std::fs::read(&existing_path)?, existing_content);
615+
assert_eq!(std::fs::read(&existing_path)?, EXISTING_CONTENT);
614616
Ok(())
615617
}
616618

617619
#[test]
618-
fn drop_after_failed_fetch_into_non_empty_directory_preserves_pre_existing_files() -> crate::Result {
619-
let tmp = gix_testtools::tempfile::TempDir::new()?;
620-
let existing_path = tmp.path().join("existing.txt");
621-
let existing_content = b"I was here before you";
622-
std::fs::write(&existing_path, existing_content)?;
620+
fn fetch_and_checkout_into_non_empty_directory_does_not_overwrite_pre_existing_tracked_file() -> crate::Result {
621+
let fixture = gix_testtools::scripted_fixture_writable("make_clone_destinations.sh")?;
622+
let destination = fixture.path().join("non-empty-with-conflicting-file");
623+
let existing_path = destination.join("file");
624+
let remote_file_content = std::fs::read(remote::repo("base").workdir().expect("non-bare").join("file"))?;
625+
assert_ne!(
626+
EXISTING_CONTENT, remote_file_content,
627+
"the fixture must differ from the file that checkout would write"
628+
);
623629

624630
let mut prepare = gix::clone::PrepareFetch::new(
625631
remote::repo("base").path(),
626-
tmp.path(),
632+
&destination,
633+
gix::create::Kind::WithWorktree,
634+
gix::create::Options {
635+
destination_must_be_empty: Some(false),
636+
..Default::default()
637+
},
638+
restricted(),
639+
)?;
640+
let (mut checkout, _out) = prepare.fetch_then_checkout(gix::progress::Discard, &AtomicBool::default())?;
641+
let (repo, outcome) = checkout.main_worktree(gix::progress::Discard, &AtomicBool::default())?;
642+
643+
assert_eq!(
644+
std::fs::read(&existing_path)?,
645+
EXISTING_CONTENT,
646+
"checkout must not overwrite the pre-existing tracked path"
647+
);
648+
assert_eq!(repo.index()?.entries().len(), 1, "the index is still written");
649+
assert_eq!(
650+
outcome.collisions,
651+
[gix_worktree_state::checkout::Collision {
652+
path: BString::from("file"),
653+
error_kind: std::io::ErrorKind::AlreadyExists
654+
}],
655+
"the pre-existing tracked path is reported as a normal checkout collision"
656+
);
657+
Ok(())
658+
}
659+
660+
#[test]
661+
fn fetch_and_checkout_into_non_empty_directory_with_existing_dot_git_is_rejected() -> crate::Result {
662+
let fixture = gix_testtools::scripted_fixture_writable("make_clone_destinations.sh")?;
663+
let destination = fixture.path().join("non-empty-with-dot-git");
664+
let existing_path = destination.join("existing.txt");
665+
let dot_git = destination.join(".git");
666+
let head_path = dot_git.join("HEAD");
667+
668+
let err = gix::clone::PrepareFetch::new(
669+
remote::repo("base").path(),
670+
&destination,
671+
gix::create::Kind::WithWorktree,
672+
gix::create::Options {
673+
destination_must_be_empty: Some(false),
674+
..Default::default()
675+
},
676+
restricted(),
677+
)
678+
.map(drop)
679+
.expect_err("an existing .git directory must not be reused for clone");
680+
681+
assert!(
682+
matches!(
683+
err,
684+
gix::clone::Error::Init(gix::init::Error::Init(gix::create::Error::DirectoryExists { ref path }))
685+
if *path == dot_git
686+
),
687+
"unexpected error: {err}"
688+
);
689+
assert_eq!(std::fs::read(&existing_path)?, EXISTING_CONTENT);
690+
assert_eq!(std::fs::read(&head_path)?, EXISTING_HEAD_CONTENT);
691+
Ok(())
692+
}
693+
694+
#[test]
695+
fn drop_after_failed_fetch_into_non_empty_directory_preserves_destination() -> crate::Result {
696+
let fixture = gix_testtools::scripted_fixture_writable("make_clone_destinations.sh")?;
697+
let destination = fixture.path().join("non-empty");
698+
let existing_path = destination.join("existing.txt");
699+
700+
let mut prepare = gix::clone::PrepareFetch::new(
701+
remote::repo("base").path(),
702+
&destination,
627703
gix::create::Kind::WithWorktree,
628704
gix::create::Options {
629705
destination_must_be_empty: Some(false),
@@ -640,12 +716,12 @@ mod blocking_io {
640716

641717
assert_eq!(
642718
std::fs::read(&existing_path)?,
643-
existing_content,
719+
EXISTING_CONTENT,
644720
"pre-existing user files must survive a failed clone+drop"
645721
);
646722
assert!(
647-
!tmp.path().join(".git").exists(),
648-
"only the .git directory we created should have been cleaned up"
723+
destination.join(".git").is_dir(),
724+
"the .git directory we created should remain for user cleanup"
649725
);
650726
Ok(())
651727
}
@@ -899,21 +975,21 @@ fn clone_and_destination_must_be_empty() -> crate::Result {
899975

900976
#[test]
901977
fn clone_with_worktree_and_destination_must_be_empty() -> crate::Result {
902-
let tmp = gix_testtools::tempfile::TempDir::new()?;
903-
std::fs::write(tmp.path().join("file"), b"hello")?;
904-
match gix::clone::PrepareFetch::new(
978+
let fixture = gix_testtools::scripted_fixture_writable("make_clone_destinations.sh")?;
979+
let destination = fixture.path().join("non-empty");
980+
let err = gix::clone::PrepareFetch::new(
905981
remote::repo("base").path(),
906-
tmp.path(),
982+
&destination,
907983
gix::create::Kind::WithWorktree,
908984
Default::default(),
909985
restricted(),
910-
) {
911-
Ok(_) => unreachable!("this should fail as the directory isn't empty"),
912-
Err(err) => assert!(
913-
err.to_string()
914-
.starts_with("Refusing to initialize the non-empty directory as ")
915-
),
916-
}
986+
)
987+
.map(drop)
988+
.expect_err("this should fail as the directory isn't empty");
989+
assert!(
990+
err.to_string()
991+
.starts_with("Refusing to initialize the non-empty directory as ")
992+
);
917993
Ok(())
918994
}
919995

‎gix/tests/gix/init.rs‎

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,32 @@ mod non_bare {
164164
}
165165

166166
#[test]
167-
fn init_into_non_empty_directory_is_not_allowed_if_option_is_set_as_used_for_clone() -> crate::Result {
167+
fn init_into_non_empty_directory_is_allowed_if_option_is_none_or_false() -> crate::Result {
168+
for destination_must_be_empty in [None, Some(false)] {
169+
let tmp = tempfile::tempdir()?;
170+
std::fs::write(tmp.path().join("existing.txt"), b"I was here before you")?;
171+
let repo: gix::Repository = gix::ThreadSafeRepository::init_opts(
172+
tmp.path(),
173+
gix::create::Kind::WithWorktree,
174+
gix::create::Options {
175+
destination_must_be_empty,
176+
..Default::default()
177+
},
178+
gix::open::Options::isolated(),
179+
)?
180+
.into();
181+
assert_eq!(repo.workdir().expect("present"), tmp.path());
182+
assert_eq!(
183+
repo.git_dir(),
184+
tmp.path().join(".git"),
185+
"gitdir is inside of the workdir"
186+
);
187+
}
188+
Ok(())
189+
}
190+
191+
#[test]
192+
fn init_into_non_empty_directory_is_not_allowed_if_option_is_true() -> crate::Result {
168193
let tmp = tempfile::tempdir()?;
169194
std::fs::write(tmp.path().join("existing.txt"), b"I was here before you")?;
170195

@@ -184,19 +209,4 @@ mod non_bare {
184209
);
185210
Ok(())
186211
}
187-
188-
#[test]
189-
fn init_into_non_empty_directory_is_allowed_by_default() -> crate::Result {
190-
let tmp = tempfile::tempdir()?;
191-
std::fs::write(tmp.path().join("existing.txt"), b"I was here before you")?;
192-
193-
let repo = gix::init(tmp.path())?;
194-
assert_eq!(repo.workdir().expect("present"), tmp.path());
195-
assert_eq!(
196-
repo.git_dir(),
197-
tmp.path().join(".git"),
198-
"gitdir is inside of the workdir"
199-
);
200-
Ok(())
201-
}
202212
}

0 commit comments

Comments
 (0)