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

[kevincox (Kevin Cox)]

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<Path> + Sync + Send,
> +    work: mpsc::Sender<&'a HgPath>,
>      options: StatusOptions,

Please describe this parameter.

> status.rs:241
> +                            // The channel always outlives the sender, unwrap
> +                            work.send(normalized).unwrap()
> +                        } else {

This seems like something that should be handled in the caller. It is trivial for the caller to separate files from directories if they wish to do so.

> status.rs:275
> +        .filter(|s| s.is_some())
> +        .map(|s| s.unwrap())
>  }

I believe these two can be replaced with `.flatten()`

> status.rs:456
>      options: StatusOptions,
> -) -> impl ParallelIterator<Item = IoResult<(&HgPath, Dispatch)>> {
> +) -> impl ParallelIterator<Item = IoResult<(Cow<HgPath>, Dispatch)>> {
>      dmap.par_iter().map(move |(filename, entry)| {

Why was this changed when IIUC all of the returns are references? If a caller wants that I would prefer to do a map in the caller.

> status.rs:617
> +    // Step 1: check the files explicitly mentioned by the user
> +    let (tx, rx) = mpsc::channel();
> +    results.par_extend(walk_explicit(files, &dmap, root_dir, tx, options));

tx and rx are bad names, please indicate what is being sent and recieved.

> status.rs:618
> +    let (tx, rx) = mpsc::channel();
> +    results.par_extend(walk_explicit(files, &dmap, root_dir, tx, options));
> +    while let Ok(dir) = rx.recv() {

Instead of creating a vec then extending use collect.

> status.rs:619
> +    results.par_extend(walk_explicit(files, &dmap, root_dir, tx, options));
> +    while let Ok(dir) = rx.recv() {
> +        if options.list_ignored || options.list_unknown && !dir_ignore_fn(dir)

You are first collecting all of `results`, then collecting all of `work`. It looks like you intended to do this in parallel but I don't think that is what is happening.

A better approach is to make walk_explicit return both items and partition them here.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7929/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7929

To: Alphare, #hg-reviewers, kevincox
Cc: marmoute, kevincox, mercurial-devel