This is an archive of the discontinued Mercurial Phabricator instance.

rust-status: add util for listing a directory
ClosedPublic

Authored by Alphare on Jan 17 2020, 10:33 AM.

Details

Summary

I debated moving it to utils, but it is not used anywhere else for now, and
its skip behavior is pretty specific to status.

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

Alphare created this revision.Jan 17 2020, 10:33 AM
marmoute accepted this revision.Feb 5 2020, 5:31 PM
marmoute added a subscriber: marmoute.

I am not a rust expert, but I don't see anything obviously wrong in there. I make a couple of minor comments.

rust/hg-core/src/dirstate/status.rs
68

note: took me some time to realize that skipping the full directory if there is a .hg make sense. Because we detected a another repository and that the wc is part of that repository.

Maybe follow up with a documentation update that clarify this.

74

Silly question: would it make sense to sort the filename before the loop ? It might result is less bytes to move around since the entry will not be attached yet.

Alphare marked an inline comment as done.Feb 6 2020, 10:02 AM
Alphare added inline comments.
rust/hg-core/src/dirstate/status.rs
74

I'm not entirely sure I understand your question.

marmoute added inline comments.Feb 6 2020, 2:46 PM
rust/hg-core/src/dirstate/status.rs
74

If I understand you code correctly you currently do:

filenames = listdir()
result = [(f, entry(f)) for f in filenames]
result.sort()

Would it make sense to do:

filenames = listdir()
filenames.sort()
result = [(f, entry(f)) for f in filenames]
Alphare added inline comments.Feb 10 2020, 11:11 AM
rust/hg-core/src/dirstate/status.rs
74

std::fs::read_dir returns an iterator, so I would have to collect it first, unsure if that would be more expensive... and also, a Vec of heap-allocated elements being just an array of pointers I'm not convinced that would help. Maybe we can do some benchmarking further down the line.

kevincox accepted this revision.Feb 12 2020, 5:09 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
76

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_key.

Alphare marked an inline comment as done.Feb 12 2020, 8:53 AM
Alphare added inline comments.Feb 12 2020, 9:02 AM
rust/hg-core/src/dirstate/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 later.

kevincox added inline comments.Feb 12 2020, 11:02 AM
rust/hg-core/src/dirstate/status.rs
76

sort_unstable_by_key doesn't require the function to return a T at all. It just needs to return an Ord type. Note that &Ord is Ord automatically.

https://doc.rust-lang.org/std/primitive.slice.html#method.sort_unstable_by_key

Alphare updated this revision to Diff 20183.Feb 13 2020, 12:36 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.