D2057: rust implementation of hg status

[kevincox (Kevin Cox)]

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