kevincox added a comment.
The latest changes are looking really good. I have a couple more comments but I didn't have time for a full review. I'll try to get more reviewed tomorrow. It seems that you still have a lot of stuff still in-flight so I'll try to slowly review the changes as I have time. If you want input/feedback on any particular part just ask and I will prioritize it. This change is very large so it might be worth splitting off a smaller component and getting that submitted first. However I do realize that for starting out it is often helpful to get some actual use cases implemented before committing the base structures. INLINE COMMENTS > Ivzhh wrote in base85.rs:23 > This crate is my previous try to integrate rust into hg. Right now I guess > mine main pursue is to add hg r-* commands for rust. I will follow your > suggestion when I am implementing the wire protocol and reuse the code for > pure rust crate. I get that, but I still think it makes the code easier to read when the python-interop and the logic as separated where it is easy to do so. > Ivzhh wrote in base85.rs:91 > when I removed the unsafe, I got error: error[E0133]: use of mutable static > requires unsafe function or block I meant safe not as it it didn't need the unsafe keyword, but in that the use of the `unsafe` block is safe. It should really be called the `trust_me,_I_know_this_is_safe` block. But since you are not getting the compiler checking it is often useful to add a comment why the action you are performing is correct. In this case it is correct because the caller initializes this variable before the function is called. > base85.rs:105 > + } > + }; > + If this computation only depends on `len` it would be nice to put it in a helper function. > main.rs:260 > + let res = repo.status(); > + for f in res.modified.iter() { > + println!("M {}", f.to_str().unwrap()); These are `HashSet`'s which don't have a defined iterator order. IIRC the python implementation sorts the results which is probably desirable. > config.rs:50 > + pub revlog_format: RevlogFormat, > + /// where to get delta base, please refer to wiki page > + pub delta: DeltaPolicy, A link to the mentioned wiki page would be very helpful to readers. > Ivzhh wrote in dirstate.rs:170 > I kind of get borrow check compile error here. Later I use Occupied() when > possible. Sorry, I misunderstood the logic. You can do this: diff -r ccc683587fdb rust/hgstorage/src/dirstate.rs --- a/rust/hgstorage/src/dirstate.rs Sat Mar 24 10:05:53 2018 +0000 +++ b/rust/hgstorage/src/dirstate.rs Sat Mar 24 10:14:58 2018 +0000 @@ -184,8 +184,7 @@ continue; } - if self.dir_state_map.contains_key(rel_path) { - let dir_entry = &self.dir_state_map[rel_path]; + if let Some(dir_entry) = self.dir_state_map.get(rel_path) { files_not_in_walkdir.remove(rel_path); DirState::check_status(&mut res, abs_path, rel_path, dir_entry); } else if !matcher.check_path_ignored(rel_path.to_str().unwrap()) { > dirstate.rs:103 > + if let Ok(meta_data) = meta_data { > + let mtime = meta_data.modified().expect("default mtime must > exist"); > + Switch the return type to `std::io::Result` and then you can have let metadata = p.metadata()?; let mtime = metadata.modified()?; // ... > dirstate.rs:263 > + .unwrap() > + .as_secs() != dir_entry.mtime as u64 > + { Does it make sense to make `DirStateEntry.mtime` be a `std::time::SystemTime` and convert upon reading the structure in? If not I would prefer doing the conversion here: else if mtd.modified().unwrap() == UNIX_EPOCH + Duration::from_secs(dir_entry.mtime as u64) { (Maybe extract the system time to higher up, or even a helper function on dir_entry) > Ivzhh wrote in local_repo.rs:136 > I think LRU will update reference count (or timestamp) when the data is > accessed. Actually I didn't realize that RwLock doesn't get a regular `get()` since it is doing a compile time borrow check. https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.get_mut. My mistake, the code is fine. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2057 To: Ivzhh, #hg-reviewers, kevincox Cc: quark, yuja, glandium, krbullock, indygreg, durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel