coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

`cp`: add progress bar

Open tertsdiepraam opened this issue 1 year ago • 7 comments

Closes #1455

Based on the recent discord discussion around https://github.com/uutils/coreutils/issues/1455, I figured I would give it a go to see what the feature would look like. This is mostly meant to spark some discussion about what this should look like and how it should behave.

Details of this implementation:

  • indicatif is used to draw the progress bar.
  • Before copying all the files, we have to recursively read all the files to get the total size (like du)
  • With very large directories, this means a bit of lag before we can actually show the bar. The entire application is also slowed down noticeably, though I haven't done any benchmarks.
  • I ignored the size of the directories themselves as it seems insignificant in most cases.
  • We update the progress bar with each copied file.
  • This behaviour might need to be tweaked when using cp with the symlink options as that wouldn't actually copy that many bytes.

If we actually want to land this, the progress bar (and accompanying performance hit) would of course need to be opt-in. I'm leaning towards adding both a compile-time feature gate and a run-time flag to enable this.

This is what it looks like: image

tertsdiepraam avatar Sep 05 '22 09:09 tertsdiepraam

it is magic how small it is :)

sylvestre avatar Sep 05 '22 09:09 sylvestre

Indeed! indicatif is amazing :)

tertsdiepraam avatar Sep 05 '22 09:09 tertsdiepraam

what about adding an option --progress ?

sylvestre avatar Sep 05 '22 10:09 sylvestre

Yeah that makes sense. I added -g/--progress. I'm not sure what the g stands for, but it's used in advcpmv, which seems to be the most notable "prior art" for this. xcp also has a progress bar, but only has a flag to disable the bar (--no-progress).

I think a compile-time flag might be unnecessary, because the bloat by indicatif seems to be fairly minimal (about 70Kib, which is 1.5% of cp), so it might not be necessary.

tertsdiepraam avatar Sep 05 '22 11:09 tertsdiepraam

Maybe we should have a page in which we list all the stuff that we added compared to GNU's?

for example, I think we have more hashing functions

sylvestre avatar Sep 05 '22 12:09 sylvestre

I'll add something like that to the docs in this PR. I'll also add the bar to mv and then I'll mark it ready for review. In using it for testing, I already found myself agreeing with the issue author that this is very useful.

We can also bikeshed the layout and appearance of the bar itself. I think it makes sense to put cp: as a prefix like we do with warnings and errors so the user can see what command in a script is showing the bar. I think I'll also add the ETA.

tertsdiepraam avatar Sep 05 '22 18:09 tertsdiepraam

I changed my mind about mv, because it just renames files, it's usually very fast anyway, so doesn't really need a progress bar.

With the documentation on the extensions over GNU, I think this is ready.

tertsdiepraam avatar Sep 05 '22 23:09 tertsdiepraam

sorry, I think I failed as fixing the conflicts :(

sylvestre avatar Oct 03 '22 18:10 sylvestre

small error:


error[B004]: found 2 duplicate entries for crate 'terminal_size'
    ┌─ /github/workspace/Cargo.lock:201:1
    │  
201 │ ╭ terminal_size 0.1.17 registry+https://github.com/rust-lang/crates.io-index
202 │ │ terminal_size 0.2.1 registry+https://github.com/rust-lang/crates.io-index
    │ ╰─────────────────────────────────────────────────────────────────────────^ lock entries

sylvestre avatar Oct 15 '22 18:10 sylvestre

Welp, looks like it's blocked. So I think we'll have to make an exception in deny.

tertsdiepraam avatar Oct 15 '22 19:10 tertsdiepraam

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

github-actions[bot] avatar Oct 17 '22 09:10 github-actions[bot]

needs rebasing again, sorry :(

Once done, if the CI is green, please land it :)

sylvestre avatar Oct 22 '22 08:10 sylvestre