This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9671

rust: fix file folding map
ClosedPublic

Authored by danchr on Dec 30 2020, 7:01 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG5bae4bc9bd42: rust: fix file folding map
Summary

The file folding map, frequently used on macOS, had two issues:

  • the means for converting it to Python didn't work
  • a minor typo when copying the python code, where != became ==

With this, the rust code passes all tests on macOS.

Test Plan

I'm currently doing a full test run on a case-insensitive file system. If it passes, perhaps we can change the platform check from an error to a warning?

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

danchr created this revision.Dec 30 2020, 7:01 AM
danchr edited the test plan for this revision. (Show Details)Dec 30 2020, 7:05 AM
danchr added a reviewer: Alphare.
danchr added a comment.EditedDec 30 2020, 7:38 AM

As a minor addendum: With this change, the Rust dirstate code almost works with case folding, but has issues detecting folded directory names. This appears mostly unimplemented AFAICT.

baymax updated this revision to Diff 24558.Dec 31 2020, 5:07 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Jan 5 2021, 4:22 AM

Thanks for fixing these. The code for handling filename normalization is incomplete, and I'm pretty much assuming some subtle bugs exist for whatever bits of it are implemented.

We (read: Octobus) are planning a dirstate overhaul in the next few months that will probably have a big influence on the structure of the code, FYI. That said, the normalization code itself will still be useful and the overall logic should be similar, so feel free to have a stab at it.

I'm not a big fan of the idea of people using this version with incomplete normalization by lifting the compiler error since it may cause bugs, wrong repo states or even security issues.

danchr added a comment.Jan 9 2021, 10:39 PM
In D9671#146004, @Alphare wrote:

I'm not a big fan of the idea of people using this version with incomplete normalization by lifting the compiler error since it may cause bugs, wrong repo states or even security issues.

To be blunt, the compiler error has nothing to do with normalisation. It just prevents anyone from trying the Rust code on any platform other than Linux. A warning would be fine, in my opinion, but an error is quite aggressive. As far as I can tell, the Rust code correctly disables itself when detecting a case-insensitive file system. Yes, this is quite common on macOS, but I happen to have almost all source code on a case-sensitive volume. I was also able to trigger the bug on FreeBSD (after disabling the error) and Linux, by creating a case-insensitive ZFS filesystem.

(Personally, I've installed an hg with Rust extensions on my Mac; I have yet to run into issues since fixing this.)

Anyway, this change has nothing to do with the error; it just fixes some bugs in the existing code.

Alphare added a comment.Jan 11 2021, 4:05 AM
In D9671#146689, @danchr wrote:

To be blunt, the compiler error has nothing to do with normalisation. It just prevents anyone from trying the Rust code on any platform other than Linux.

Let's downgrade to a warning, you're right. I'm not sure people will see it, but Rust extensions are still experimental enough that it's not a big issue, I was probably being overzealous.

(Personally, I've installed an hg with Rust extensions on my Mac; I have yet to run into issues since fixing this.)

Nice!

Anyway, this change has nothing to do with the error; it just fixes some bugs in the existing code.

Yep, this patch should definitely be taken.

pulkit accepted this revision.Jan 13 2021, 4:55 AM
This revision is now accepted and ready to land.Jan 13 2021, 4:55 AM

Revision Contents
Changeset List

Diff 24784

rust/hg-core/src/dirstate/dirstate_map.rs

#[cfg(not(feature = "dirstate-tree"))] #[cfg(not(feature = "dirstate-tree"))]
pub fn build_file_fold_map(&mut self) -> &FileFoldMap { pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map { if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map; return file_fold_map;
} }
let mut new_file_fold_map = FileFoldMap::default(); let mut new_file_fold_map = FileFoldMap::default();
for (filename, DirstateEntry { state, .. }) in self.state_map.iter() { for (filename, DirstateEntry { state, .. }) in self.state_map.iter() {
if *state == EntryState::Removed { if *state != EntryState::Removed {
new_file_fold_map new_file_fold_map
.insert(normalize_case(&filename), filename.to_owned()); .insert(normalize_case(&filename), filename.to_owned());
} }
} }
self.file_fold_map = Some(new_file_fold_map); self.file_fold_map = Some(new_file_fold_map);
self.file_fold_map.as_ref().unwrap() self.file_fold_map.as_ref().unwrap()
} }
#[cfg(feature = "dirstate-tree")] #[cfg(feature = "dirstate-tree")]
pub fn build_file_fold_map(&mut self) -> &FileFoldMap { pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map { if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map; return file_fold_map;
} }
let mut new_file_fold_map = FileFoldMap::default(); let mut new_file_fold_map = FileFoldMap::default();
for (filename, DirstateEntry { state, .. }) in self.state_map.iter() { for (filename, DirstateEntry { state, .. }) in self.state_map.iter() {
if state == EntryState::Removed { if state != EntryState::Removed {
new_file_fold_map new_file_fold_map
.insert(normalize_case(&filename), filename.to_owned()); .insert(normalize_case(&filename), filename.to_owned());
} }
} }
self.file_fold_map = Some(new_file_fold_map); self.file_fold_map = Some(new_file_fold_map);
self.file_fold_map.as_ref().unwrap() self.file_fold_map.as_ref().unwrap()
} }
} }

rust/hg-cpython/src/dirstate/dirstate_map.rs

def filefoldmapasdict(&self) -> PyResult<PyDict> { def filefoldmapasdict(&self) -> PyResult<PyDict> {
let dict = PyDict::new(py); let dict = PyDict::new(py);
for (key, value) in for (key, value) in
self.inner(py).borrow_mut().build_file_fold_map().iter() self.inner(py).borrow_mut().build_file_fold_map().iter()
{ {
dict.set_item( dict.set_item(
py, py,
key.as_bytes().to_vec(), PyBytes::new(py, key.as_bytes()).into_object(),
value.as_bytes().to_vec(), PyBytes::new(py, value.as_bytes()).into_object(),
)?; )?;
} }
Ok(dict) Ok(dict)
} }
def __len__(&self) -> PyResult<usize> { def __len__(&self) -> PyResult<usize> {
Ok(self.inner(py).borrow().len()) Ok(self.inner(py).borrow().len())
} }