D8098: rust-nodemap: a method for full invalidation

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:577 > +/// immutable part. > +pub fn invalidate_all(&mut self) -> () { > +self.root = Block::new(); `-> ()` is unnecessary. > nodemap.rs:580 > +self.growable = Vec::new(); > +

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:191 > fn new() -> Self { > -Block([-1; 16]) > +Block([255u8; BLOCK_SIZE]) > } A comment why 255 is a good number would be nice. > nodemap.rs:334 > +// Assert that

D7797: rust-nodemap: pure Rust example

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in index.rs:76 > I am not sure exactly what the `debug_assert` should be? Do you want to check > that the `len` is a multiple of the `align_of::`? Yes, that seems reasonable. It shows what data you are ignoring and will alert us i

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in nodemap.rs:268 > I think that would be sufficient indeed. How about making `Block` into a > `[u8; 64]`? It will be less expressive, but this will no longer be needed. I like that. It makes it much more explicit that it is a back

D8110: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in dirstate_map.rs:251 > `as_mut()` only does `&mut Option -> Option<&mut T>`, which isn't always > what we want (however you can argue that it would save a few keystrokes), but > calling `unwrap()` circles back to the same `error[

D7797: rust-nodemap: pure Rust example

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > Alphare wrote in index.rs:76 > I'm not sure of the point of doing so with an mmap. We have to trust what the > mmap is giving us. In general an assertion is better than a comment because it expresses the things

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in nodemap.rs:268 > @kevincox @durin42: I've changed the code to use the type-level > `ManuallyDrop` instead of `mem::forget`, added the `transmute` to be extra > paranoid and added a comment. This doesn't address the primary prob

D8110: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in dirstate_map.rs:251 > Unwrapping creates a temporary value that is dropped instantly, so unless I'm > not seeing something, I don't think we can. You should be able to use `.as_mut()` just like you are currently doing in the ca

D8110: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:38 > +non_normal_set: Option>, > +other_parent_set: Option>, > parents: Option, I don't understand why an `Option` is faster than a `HashSet`. Could you add some explanation to the commit message? Is this to avoid at

D7929: rust-status: add bare `hg status` support in hg-core

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in status.rs:456 > `traverse` returns some `Cow::Owned`, I felt that it was simpler to share the > same return signature for all functions, but I can map the output, I guess. I can definitely see that this is a judgement call. Howe

D7927: rust-status: add util for listing a directory

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in status.rs:76 > I didn't realize that `sort_unstable_by_key` needed to return a `T` and not a > `&T`, that forces a `clone()`. I'm not sure that matters, we'll go with the > shortest code and see if that shows up in profiling lat

D7797: rust-nodemap: pure Rust example

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > index.rs:6 > + > +/// Minimal `RevlogIndex`, readable from standard Mercurial file format > +use hg::*; Since this is a file-level comment it should use `//!` https://doc.rust-lang.org/reference/comments.html#exa

D7929: rust-status: add bare `hg status` support in hg-core

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > status.rs:201 > root_dir: impl AsRef + Sync + Send, > +work: mpsc::Sender<&'a HgPath>, > options: StatusOptions, Please describe this parame

D7927: rust-status: add util for listing a directory

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:76 > + > +results.sort_by(|a, b| a.0.cmp(&b.0)); > +Ok(results) It would be more clear to do `.sort_by_key(|e| e.0)` And I don't think stability matters here so you could use `sort_unstable_by_

D8048: rust-dirstatemap: cache non normal and other parent set

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:251 > +self.set_non_normal_other_parent_entries(false); > +(&mut self.non_normal_set, &mut self.other_parent_set) > +} If we have just set the fields to `Some(..)` in the previous line can't we do the unwrap h

D7930: rust-status: update rust-cpython bridge to account for the changes in core

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:76 > +BadType::Unknown => "unknown", > +} > +) This should probably be a method on BadType. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https:

D8086: rust-status: refactor options into a `StatusOptions` struct

kevincox added inline comments. INLINE COMMENTS > status.rs:208 > +pub list_clean: bool, > +} > + These names aren't very meaningful to me but maybe that is because I'm not super familiar with the domain. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.or

D8088: rust-status: add missing variants to `Dispatch` enum

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:288 > /* TODO ignored > * TODO unknown */ > } TODO is done. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D8088/new/ REVISION DETAIL https://phab.

D7924: rust-matchers: add `build_regex_match` function

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > matchers.rs:248 > +/// Returns a function that matches an `HgPath` against the regex formed by > +/// the given patterns. > +fn build_regex_match<'a>( Can you describe the return value here? It seems odd that we r

D7923: rust-matchers: add functions to get roots, dirs and parents from patterns

kevincox added a comment. Thanks for the heads up. I'll try to dedup them. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7923/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7923 To: Alphare, #hg-reviewers, kevincox Cc: durin42, kevincox, merc

D7921: rust-dirs-multiset: improve temporary error message

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > dirs_multiset.rs:86 > +subpath.as_bytes(), > +); > +let second_slash_index = offset.unwrap() + subpath.

D7923: rust-matchers: add functions to get roots, dirs and parents from patterns

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > matchers.rs:250 > +/// This calculates the roots and directories exactly matching the patterns > and > +/// returns a tuple of (roots, dirs) for each. It

D7908: rust-filepatterns: improve API and robustness for pattern files parsing

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in filepatterns.rs:237 > I might be using godbolt wrong but it looks like you are using different > versions of rustc. When using 1.40 for both sides, I don't see any clear win > on either side... but I'm not fluent in assembly eit

D7909: rust-filepatterns: add support for `ignore` and `subignore` patterns

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in filepatterns.rs:462 > The `match` arms would be incompatible. Or maybe I'm missing something. You would have to make sure you return a Vec on both arms. You will probably want `inner_pats.into_iter().collect()` on one branch and

D7908: rust-filepatterns: improve API and robustness for pattern files parsing

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > filepatterns.rs:227 > +return acc; > +} > +if component != b".." Can you move this condition into a `.filter()`? > Alphare wrote in filepatterns.rs:237 > I'm no

D7910: rust-re2: add wrapper for calling Re2 from Rust

kevincox added a comment. While I think the rust regex crate is heavily inspired by RE2 and uses a fairly compatible syntax I don't think there is a goal to be 100% compatible and I would be surprised if there weren't any differences. What I am trying to say is that if we are going to change

D7867: rust-hg-path: add useful methods to `HgPath`

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > hg_path.rs:169 > +&mut self.inner > +} > pub fn contains(&self, other: u8) -> bool { I don't think we should have this method. It provide

D7910: rust-re2: add wrapper for calling Re2 from Rust

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > rust_re2.cpp:22 > + > + return new RE2(StringPiece(data, len), o); > + } This is never freed. Should we add a `Drop` implementation in rus

D7910: rust-re2: add wrapper for calling Re2 from Rust

kevincox added a comment. kevincox accepted this revision. Does mercurial currently use RE2? If not then can you explain why we don't just use the rust `regex` crate? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7910/new/ REVISION DETAIL https://p

D7796: rust-nodemap: input/output primitives

kevincox added a comment. Maybe what we should do is keep the test I proposed, but change the definition of `BLOCK_SIZE` to be based upon the number of contained `i32` instead of `size_of`. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REV

D7796: rust-nodemap: input/output primitives

kevincox added a comment. Ah yes. So my test would be ineffective. The problem that IIUC padding is not guaranteed to be initialized, and in rust all values need to be initialized. So if you cast a type with padding to a `u8` it is undefined behaviour. So we would need a test that s

D7909: rust-filepatterns: add support for `ignore` and `subignore` patterns

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > filepatterns.rs:456 > +.into_iter() > +.filter_map(|entry| -> Option> { > +let IgnorePattern { I don't see anywhere where this

D7908: rust-filepatterns: improve API and robustness for pattern files parsing

kevincox added inline comments. INLINE COMMENTS > filepatterns.rs:220 > +initial_slashes = 2; > +} > +let components = It might be clearer to do `bytes.iter().take_while(|b| b == sep).count()` > filepatterns.rs:237 > +acc > +}); > +let mut new_byt

D7869: rust-dirs-multiset: add `DirsChildrenMultiset`

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:197 > +/// ``` > +pub fn split_at_suffix(&self) -> (&Self, &Self) { > +match &self.inner.iter().rposition(|c| *c == b'/') { "suffix" isn't clear to me. Maybe `split_dir_from_file`? That'

D7870: rust-utils: add `Escaped` trait

kevincox added a comment. kevincox accepted this revision. I think it's better. Maybe still a little vauge but I'll let subject experts make that decision. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7870/new/ REVISION DETAIL https://phab.mercuri

D7819: rust-nodemap: core implementation for shortest

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:337 > /// Main working method for `NodeTree` searches > fn lookup<'p>( > &self, Can you describe the return value in the doc comment. > nodemap.rs:575 > + > +fn shortest_bin<'a>(

D7796: rust-nodemap: input/output primitives

kevincox added inline comments. INLINE COMMENTS > kevincox wrote in nodemap.rs:268 > This makes me uncomfortable. There is really no guarantee that there is no > padding around `Block` or `RawElement`. If there is any padding this is UB. I > would much prefer one of: > > 1. Store growable as a

D7796: rust-nodemap: input/output primitives

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > nodemap.rs:268 > +Vec::from_raw_parts( > +vec.as_ptr() as *mut u8, > +vec.len() * BLOCK_SIZE, This makes me un

D7795: rust-nodemap: insert method

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:268 > +/// itself because of the mutable borrow taken with the returned `Block` > +fn mutable_block(&mut self, idx: usize) -> (usize, &mut Block, usize) { > +let ro_blocks = &self.readonl

D7788: rust-node: binary Node and conversion utilities

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > node.rs:14 > +/// Binary revisions SHA > +pub type Node = [u8; 20]; > + I would recommend making this a proper type because almost no one should be poking at the bytes of the hash. struct Node([u8; 20]); You

D7794: rust-nodemap: generic NodeTreeVisitor

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > nodemap.rs:254 > +impl<'n, 'p> Iterator for NodeTreeVisitor<'n, 'p> { > +type Item = (bool, usize, u8, Option); > + I would prefer making this a struc

D7793: rust-nodemap: mutable NodeTree data structure

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:158 > readonly: Box + Send>, > +growable: Vec, > +root: Block, This strikes me as a weird name. The fact that it is an adjective rather than a noun is a hint. Can you rename to answer "Gro

D7792: rust-nodemap: abstracting the indexing

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:192 > ) -> Result, NodeMapError> { > -let blocks: &[Block] = &*self.readonly; > -if blocks.is_empty() { > +let len = self.len(); > +if len == 0 { I don't think this

D7791: rust-nodemap: NodeMap trait with simplest implementor

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:37 > +/// whose data should not be owned by the `NodeMap`. > +pub trait NodeMap { > +fn find_bin<'a>( Can you please add doc-comments for this? I find that documenting trait methods is especially i

D7864: rust-utils: add Rust implementation of Python's "os.path.splitdrive"

kevincox added a comment. kevincox accepted this revision. One last note is why not just put this in HgPath? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7864/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7864 To: Alphare, #hg-reviewers, ke

D7864: rust-utils: add Rust implementation of Python's "os.path.splitdrive"

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in files.rs:109 > Indeed, this is much better. While trying to adapt this code to fit with > `HgPath`, I find myself needing to translate to and from bytes whenever > indexing or when using `split_at`. Should we give a `HgPath` a `

D7866: rust-pathauditor: add Rust implementation of the `pathauditor`

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > path_auditor.rs:53 > +.as_bytes() > +.split(|c| *c as char == std::path::MAIN_SEPARATOR) > +.collect(); Should this be `std::path::is_separator(*c as char)`?. If not please add

D7870: rust-utils: add `PrettyPrint` trait

kevincox added a comment. kevincox accepted this revision. I'm not convinced PrettyPrint is the best name. I might call it something more to do with escaping instead of "pretty". However I can't think of anything great. INLINE COMMENTS > utils.rs:146 > +acc.push(HEX_DIGITS[(

D7869: rust-dirs-multiset: add `DirsChildrenMultiset`

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > dirs_multiset.rs:170 > +Some(i) => i.contains(&directory), > +} { > +continue; I would put this check into a helper function. > files.rs:86 > +)), > +

D7867: rust-hg-path: add useful methods to `HgPath`

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:189 > +&self.inner[..] > +}; > +HgPath::new(match inner.iter().rposition(|b| *b == b'/') { It would be nice to have a `trim_trailing_slash` helper. REPOSITORY rHG Mercuria

D7864: rust-utils: add Rust implementation of Python's "os.path.splitdrive"

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > files.rs:109 > +/// Paths cannot contain both a drive letter and a UNC path. > +pub fn split_drive(path: impl AsRef) -> (HgPathBuf, HgPathBuf) { > +let

D7787: rust-nodemap: building blocks for nodetree structures

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > nodemap.rs:92 > + > +fn read(&self, nybble: u8) -> Element { > +Element::from(RawElement::from_be(self.0[nybble as usize])) I would call these `get` and `set`. > nodemap.rs:111 > +} > +

D7604: rust-hg-path: implement more readable custom Debug for HgPath{, Buf}

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:186 > +fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > +write!(f, "HgPath({})", String::from_utf8_lossy(&self.inner)) > +} I would recommend `"HgPath({:?})"` so that the res

D7577: hg-core: implement Mercurial's config file discovery logic

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > config.rs:145 > +let paths = glob_res > +.filter_map(|x| if let Ok(x) = x { Some(x) } else { None }) > +.sorted(); I believe this can just be `.flatten()`. REPOSITORY

D7529: rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > status.rs:125 > +/// TODO subrepos > +fn walk_explicit<'a: 'c, 'b: 'c, 'c>( > +files: &'a HashSet<&HgPath>, It seems to me that you can just use one l

D7526: rust-hg-path: add method to get part of a path relative to a prefix

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > hg_path.rs:131 > +let base = base.as_ref(); > +if base.len() == 0 { > +return Some(self); `base.is_empty()`? If we don't have

D7525: rust-matchers: improve `Matcher` trait ergonomics

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > matchers.rs:87 > +None > } > fn exact_match(&self, _filename: impl AsRef) -> bool { Another option here would just to have a global empty hash set. REPOSITORY rHG Mercurial CHANGES SINCE LAST

D7523: rust-hg-path: implement `Display` for `HgPath` and `HgPathBuf`

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > hg_path.rs:168 > +fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > +write!(f, "HgPath {{ {} }}", String::from_utf8_lossy(&self.inne

D7300: rust-status: refactor dispatch case for normal files

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:31 > > #[inline] > +/// Dates and times that are outside the 31-bit signed range are compared I see a lot of these and in general I would recommend against it. The compiler is smart. Unless profiling

D7299: rust-status: return a ParallelIterator instead of a Vec from stat_dmap_entries

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:128 > +Err(e) => return Err(e.into()), > +}; > This can be replaced with: let meta = root_dir.as_ref().join(hg_path_to_path_buf(filename)?).symlink_metadata(); Although I would

D7254: rust-status: improve status performance

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > status.rs:41 > +last_normal_time: i64, > +) -> (HgPathBuf, Dispatch) { > +let filename = filename.as_ref(); Every exit path is just `filename.to_owned()`. It would be better to just let the caller do that

D7178: [RFC] rust-matchers: add `Matcher` trait and implement `AlwaysMatcher`

kevincox added a comment. I see a lot of the functions are here to give optimization hints. In order to make someone non-familiar with the code able to understand it each method should state the contracts that it is making. I am having a really hard time reconciling how the different functio

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. In D7116#104769 , @Alphare wrote: > The following comparison shows that for input > 20 bytes, fnv has worse overall performance than xx Sounds good. We can always do benchmarks in the future and swap it. REPOSITO

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. kevincox accepted this revision. I've seen very good results with https://github.com/servo/rust-fnv in the past so it is probably worth including that in the comparison and possibly using it. It is especially good for small keys which seems like a common case in hg.

D7116: rust-performance: introduce FastHashMap type alias for HashMap

kevincox added a comment. In D7116#104617 , @durin42 wrote: > OOC, have you compared this with the hashbrown crate for perf? std::collections::HashMap now uses hashbrown https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html REPOSITORY

D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`

kevincox added a comment. kevincox accepted this revision. Thanks, looks good. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7058/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7058 To: Alphare, #hg-reviewers, kevincox Cc: yuja, martinvonz, d

D7058: rust-dirstate-status: add first Rust implementation of `dirstate.status`

This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > Alphare wrote in status.rs:83 > Collecting into a `Result` prevents us from doing a `filter_map` during > `collect`. Implementing a `ParallelIterator` ada

D6773: rust-hgpath: add HgPath and HgPathBuf structs to encapsulate handling of paths

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > hg_path.rs:175 > +pub fn push(&mut self, byte: u8) { > +self.inner.push(byte); > +} debug_assert check after the mutation. > hg_path.rs:242 > +fn extend>(&mut self, iter: T) { > +self.

D6774: rust-hgpath: replace all paths and filenames with HgPath/HgPathBuf

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > files.rs:75 > +/// TODO more than ASCII? > +pub fn normalize_case(path: &HgPath) -> HgPathBuf { > +#[cfg(windows)] // NTFS compares via upper() Is this used? Can you either use, delete or add tests. REPOSITOR

D6773: rust-hgpath: add HgPath and HgPathBuf structs to encapsulate handling of paths

This revision now requires changes to proceed. kevincox added a comment. kevincox requested changes to this revision. It looks good overall. I just would like to have a bit more strict definition of what an HgPath can contain and preferably some init-time validation. INLINE COMMENTS > hg_pat

D6765: rustfilepatterns: shorter code for concatenating slices

kevincox added inline comments. INLINE COMMENTS > valentin.gatienbaron wrote in filepatterns.rs:161 > Indeed, although it took a compiler upgrade. > I don't really understand why the compiler accepts the change here but not in > the other places. It seems a bit random to remove the borrow only h

mercurial-devel@mercurial-scm.org

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > utils.rs:44 > fn trim(&self) -> &Self; > +fn chop_prefix(&self, needle:&Self) -> Option<&Self>; > } I have a small preference for `drop_prefix`. > utils.rs:44 > fn trim(&self) -> &Self; > +fn c

D6765: rustfilepatterns: shorter code for concatenating slices

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > filepatterns.rs:161 > } > -let mut res = b".*".to_vec(); > -res.extend(pattern); > -res > +[&b".*"[..], pattern].concat() > } I don't think you

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. kevincox accepted this revision. In D6631#97887 , @Alphare wrote: > @kevincox I've renamed the file since it no longer just contains macros. > I've moved whatever I could move to separate structs and used `Cell` inste

D6516: rust-discovery: accept the new 'respectsize' init arg

kevincox added a comment. kevincox accepted this revision. It seems weird to add this argument without using it. I would much rather it was added as the implementation was added. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6516/new/ REVISION DETAIL

D6632: rust-dirstate: rust implementation of dirstatemap

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > dirstate_map.rs:191 > +changed = true; > +*entry = DirstateEntry { > +mtime: MTIME_UNSET, Can you just do `entry.mtime = MTIME_UNSET`? >

D6629: rust-dirstate: use EntryState enum instead of literals

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in lib.rs:85 > Did you mean `{:?}`. I can't find anything related to your syntax. Sorry yes. I've been writing too much python at work. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6629/ne

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. In D6631#97216 , @kevincox wrote: > Is there any reason this can't be done using `Rc>`? I have an example here: https://rust.godbolt.org/z/MNNR_F I see now that since python is managing the reference it does make

D6631: rust-cpython: add macro for sharing references

kevincox added a comment. Is there any reason this can't be done using `Rc>`? I have an example here: https://rust.godbolt.org/z/MNNR_F It uses `Rc` and `RefCell` however it does need to do some unsafe work to keep the RefCell borrowed for longer than would otherwise be easy to do. The

D6628: rust-parsers: switch to parse/pack_dirstate to mutate-on-loop

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > mod.rs:16 > +pub p1: Vec, > +pub p2: Vec, > } If these are always 20 bytes long you are best to use `[u8; 20]`. That way there will be no indirection. And since `Vec` is 3 words it will have a bigger im

D6629: rust-dirstate: use EntryState enum instead of literals

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > lib.rs:85 > +Overflow => "Overflow in dirstate.".to_string(), > +CorruptedEntry(e) => format!("Corrupted entry: {}.", e), > +Damaged => "Dirstate appears to be damaged.".to_strin

D6626: rust-dirstate: create dirstate submodule in hg-cpython

kevincox added inline comments. kevincox accepted this revision. INLINE COMMENTS > dirs_multiset.rs:34 > +skip_state = Some(skip.extract::(py)?.data(py)[0] as > i8); > +} > +let dirs_map; let skip_state = skip.map(|skip| skip.extract::(py)?.data(py)[0] as i8); > di

D6594: RFC dirstatemap

kevincox added a comment. In D6594#96518 , @yuja wrote: > Yes, so we'll probably want to minimize the data exchanged between Rust > and Python. This means we'll eventually move more logic to Rust side, which > hopefully reduce the number of e

D6593: rust-minor-fixes: remove Deref in favor of explicit methods

kevincox added inline comments. INLINE COMMENTS > dirs_multiset.rs:131 > + > +pub fn iter(&self) -> Iter, u32> { > +self.inner.iter() Returning a `std::collections::hash_map::Iter` binds you to this implementation. I would recommend instead returning `impl Iterator`. REPOSITORY r

D6597: rust-2018: switch hg-core and hg-cpython to rust 2018 edition

kevincox added a comment. kevincox accepted this revision. Is the SliceExt change related to the 2018 change? If not could you split the two? INLINE COMMENTS > utils.rs:3 > + > +pub fn replace_slice(buf: &mut [T], from: &[T], to: &[T]) > +where This could really use a doc comment. > utils.

D6594: RFC dirstatemap

kevincox added inline comments. INLINE COMMENTS > dirstate_map.rs:47 > + > +impl Default for DirstateMap { > +fn default() -> Self { Can you just `#[derive(Default)]`? > dirstate_map.rs:95 > +filename: &[u8], > +old_state: u8, > +entry: DirstateEntry, This should pr

D6393: rust-dirstate: add "dirs" Rust implementation

kevincox added inline comments. INLINE COMMENTS > martinvonz wrote in dirs_multiset.rs:43 > Maybe Kevin meant something like this? > > if skip_state == None | skip_state == Some(state) { > multiset.add_path(filename); > } No, you are right. I misread the condition. REPOSITORY rHG M

D6393: rust-dirstate: add "dirs" Rust implementation

kevincox added a comment. kevincox accepted this revision. Sorry. I was busy. In general don't worry about blocking on me, I can't promise any sort of reasonable response time. Worst case I can review the changes after submission and changes can be made afterwards. INLINE COMMENTS > dirs_mu

D6429: rust-discovery: optimization of add commons/missings for empty arguments

kevincox added a comment. kevincox accepted this revision. Thanks, looks good. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6429/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6429 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox,

D6394: rust-dirstate: add "dirs" rust-cpython binding

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > dirstate.rs:221 > +} > +let dirs_map; > + let dirs_map = if ... Then just let the if statement evaluate to this value. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org

D6428: rust-discovery: using the children cache in add_missing

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:233 > +/// sets, for which the discovery process will engage into sampling > +/// that needs the children cache anyway. > pub fn add_missing_revisions( These added comments seem more rele

D6429: rust-discovery: avoid useless calls to addcommons/addmissings

kevincox requested changes to this revision. kevincox added a comment. This revision now requires changes to proceed. Wouldn't it be better to make `add_missing_revisions` work properly on empty inputs? It seems like it is too error prone to try and catch every caller. If possible I would lik

D6389: rust-dirstate: create dirstate submodule

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > Alphare wrote in mod.rs:7 > The current Python public API expects `parents` as return value and does > mutate-on-parse for other in-memory objects. Sorry if I'm missing something > obvious. Ok, I'm still not con

D6389: rust-dirstate: create dirstate submodule

kevincox added inline comments. INLINE COMMENTS > Alphare wrote in mod.rs:7 > This is public so that `hg-cpython` can use it. I think it helps with > readability too, a "bag of bytes" is sometimes not really helpful when > stumbling on code. I guess the point I am trying to make is why do we p

D6424: rust-discovery: takefullsample() core implementation

kevincox accepted this revision. kevincox added inline comments. INLINE COMMENTS > discovery.rs:124 > +fn next(&mut self) -> Option { > +if self.cur > 1 { > +return None; I think it would be nice to remove the magic number. if self.cur >= self.parents.len() REPOSITORY

D6389: rust-dirstate: create dirstate submodule

kevincox added inline comments. INLINE COMMENTS > mod.rs:6 > +pub p1: &'a [u8], > +pub p2: &'a [u8], > +} If 1 and 2 are the best names why not just make it `pub struct DirstateParetens([&[u8]; 2])`? > mod.rs:7 > +pub p2: &'a [u8], > +} > + It seems odd that this struct is public

D6348: rust-dirstate: add rust implementation of `parse_dirstate` and `pack_dirstate`

kevincox requested changes to this revision. kevincox added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > dirstate.rs:16 > +pub type DirstateVec = Vec<(Vec, DirstateTuple)>; > +pub type CopyMap<'a> = Vec<(&'a [u8], &'a [u8])>; > + You have a lot of tuples here

D6345: rust-discovery: optionally don't randomize at all, for tests

kevincox accepted this revision. kevincox added a comment. The code and rust style looks fine. I'm not sure that taking the N smallest elements is the best approach for testing though. I can imagine it hiding some issues with larger values. REPOSITORY rHG Mercurial REVISION DETAIL https

D6271: rust-filepatterns: add a Rust implementation of pattern-related utils

kevincox accepted this revision. kevincox added a comment. Thanks for the changes. INLINE COMMENTS > Alphare wrote in filepatterns.rs:18 > I think I hit the same problem as the person in this issue: > `https://github.com/rust-lang/regex/issues/451`. It's a hassle to escape > bytes using the

D6271: rust-filepatterns: add a Rust implementation of pattern-related utils

kevincox requested changes to this revision. kevincox added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > filepatterns.rs:18 > +}; > +} > + Why not use https://docs.rs/regex/1.1.6/regex/fn.escape.html? If you are worried about performance you can use htt

  1   2   >