cp progress bar implementation

[Xoffio]

NOTE

Clean duplicate of #7825
Sorry about all the mess guys... I got confuse with GitHub and and ended up mankind that mess.
This the same code I just cleaned the commits.

Description

Progress bar implementation for the cp command. Now if the flag -p or --progress is set, then the user will be able to see the progress of the file or files being copy

User-Facing Changes

A new flag (--progress -p) was added to the cp command
Examples:

cp -p test_file.txt test_folder_1\
cp -r -p test_folder\* test_folder_1\
cp -r -p -i test_folder\* test_folder_1\

Notes

• The progress bar uses std::io::{Read, Write} instead of std::fs::copy to get the progress. that means that when the progress bar is used the copy process might be a bit slower.
• Progress bar for symbolic links TBD: Since symbolic links are usually very light I think is not worth it to duplicate the function copy_symlink just to add a progress bar that will be so fast to the point is not needed, but.. for consistency purposes we might need to added it, In that case I would have to pass the variable progress by parameter (to not duplicate code unnecessary). If I do that i would have to pass the progress var to every function to respect copy_impl: impl Fn(PathBuf, PathBuf, Span). Please let me know if this is not clear :p

[rgwood]

Thanks! Looks like tests are failing.

[Xoffio]

Thanks! Looks like tests are failing.

Yeah for some reason I can't get the test working even in the main branch and I can't check the code.. looking into it

[rgwood]

Thank you for the PR! We appreciate you taking the time to get this ready, seems like it was a lot of work. Some comments:

Testing

I'm a little unsure about the way the tests are implemented. It would be nice to eliminate some of the duplication/redundancy. For example, instead of this:

fn copy_test() {
    nu!("cp ...");
    assert!(...)

    // same thing again but for --progress
    nu!("cp -p ...");
    assert!(...)
}

What about something like this?

fn copy_test(progress: bool) {
    let progress_flag: String = if progress {"-p"} else {""};
    nu!("cp {progress_flag} ...");
    assert!(...)
}

I think the latter would be easier to maintain.

1 progress bar per file

I noticed that cp -p shows 1 progress bar per file:

This is a bit overwhelming when copying large files with -r. The progress bars are created and finished much too quickly for the user to read. Would it make more sense to show a single progress bar for all files?

ctrl+c not supported

I noticed that ctrl+c doesn't work to interrupt a file copy with -p. We should probably fix that.

[rgwood]

FYI, this is a bit expensive. Not gonna say we shouldn't do it this way, but nu! launches a new instance of Nushell and that's a lot slower than a function call. Probably OK to leave as is for now though.

[Xoffio]

@rgwood Really appreciate you taking the time!!

Test - I think the latter would be easier to maintain.

I love it! 100% agreed! I will be working on those changes.

1 progress bar per file

Is it ok to have both? I think the way is working right now is useful for some people... For example, I used to work for a VFX company where they manage a lot of big files. When artist wanted to move those files and work and the same time (because of crazy deadlines) it is important to know the status of each file. I am pretty sure they are other industries or cases likes this.

Maybe we can have two flags and the default is the clean one (the one you suggested )?

ctrl+c not supported

Could you check with out the -p flag. Right know i have that issue with my Nushell and I have the latest version.

[rgwood]

Maybe we can have two flags and the default is the clean one (the one you suggested )?

Let's do just 1 flag. We try to keep the number of flags on commands to the bare minimum.

it is important to know the status of each file

Would it be enough to have the following information in the progress display?

1. The name of the file currently being copied
2. The total bytes successfully copied (across all files)

I haven't thought this through in detail. We can go with the current approach if needed, but... it is a pretty bad UX for large numbers of files.

Could you check with out the -p flag.

ctrl+c doesn't work without -p, but it should. At the very least we should check ctrlc before copying each file. Should be easy, something like:

if nu_utils::ctrl_c::was_pressed(&ctrlc) {
    break;
}

[Xoffio]

We try to keep the number of flags on commands to the bare minimum.

Totally understand

Would it be enough to have the following information in the progress display?

I think this might do it

it is a pretty bad UX for large numbers of files.

Yeah let's go for the cleaner way. Do you mind accepting this PR first ( after I fix the tests ) and then I can make it cleaner? I want to avoid dealing with merging issues if someone works on the cp file. I had a bad experience last time xd but if that is a problem that's ok too, it's just that it would be easier

ctrlc before copying each file. Should be easy,

Cool! I can do that too in a different PR!

[rgwood]

Do you mind accepting this PR first ( after I fix the tests ) and then I can make it cleaner?

Sure 👍

[Xoffio]

Working on this today!

[codecov]

Codecov Report

Merging #8012 (6be0eff) into main (68ad854) will decrease coverage by 0.42%.
The diff coverage is 0.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8012      +/-   ##
==========================================
- Coverage   54.61%   54.19%   -0.42%     
==========================================
  Files         608      608              
  Lines       99003    99093      +90     
==========================================
- Hits        54068    53702     -366     
- Misses      44935    45391     +456     

Impacted Files	Coverage Δ
crates/nu-command/src/progress_bar.rs	0.00% <0.00%> (ø)
crates/nu-command/src/filesystem/cp.rs	14.42% <0.98%> (-3.04%)	⬇️
crates/nu-color-config/src/nu_style.rs	9.31% <0.00%> (-70.92%)	⬇️
crates/nu-command/src/filters/update.rs	86.45% <0.00%> (-0.65%)	⬇️
crates/nu-color-config/src/style_computer.rs	80.76% <0.00%> (-0.55%)	⬇️
crates/nu-command/src/viewers/table.rs	31.19% <0.00%> (-0.49%)	⬇️
crates/nu_plugin_formats/src/lib.rs	0.00% <0.00%> (ø)
crates/nu-cli/src/completions/completer.rs	0.00% <0.00%> (ø)
crates/nu-command/src/conversions/fill.rs	90.77% <0.00%> (+0.04%)	⬆️
... and 1 more

[Xoffio]

fn copy_test(progress: bool) {
let progress_flag: String = if progress {"-p"} else {""};
nu!("cp {progress_flag} ...");
assert!(...)
}

@rgwood Actually we can't pass parameter when doing test. I get this error:
functions used as tests can not have any arguments
Maybe there is a way I am not aware of? If there is not, what do you think about a loop that goes through the two cp ?

[sholderbach]

@rgwood Actually we can't pass parameter when doing test. I get this error: functions used as tests can not have any arguments Maybe there is a way I am not aware of? If there is not, what do you think about a loop that goes through the two cp ?

I think in some of the nushell crates we use rstest which has macros to do fixtures/variant testing like pytest. This might be an option if you have a more complex test setup.

[rgwood]

@Xoffio try something like:

#[test]
fn copy_test() {
  copy_test_impl(true);
  copy_test_impl(false);
}

fn copy_test_impl(progress: bool) {
    let progress_flag: String = if progress {"-p"} else {""};
    nu!("cp {progress_flag} ...");
    assert!(...)
}

[Xoffio]

@rgwood The tests are done! :D If you can we can merge this one and I am starting to work to make it one progress bar. This will be a bit more work but it would be easy for me to merge later if someone works in the cp file

[rgwood]

Thanks for all the work on the tests. This looks like a step in the right direction, we appreciate it. Will probably merge soon once the dust settles from the last release.