Skip to content

Conversation

@BigPapa314
Copy link
Contributor

Fixes #7296

@sylvestre
Copy link
Contributor

Could you please add a test to make sure we don't regress? Thanks

@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

1 similar comment
@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@RenjiSann
Copy link
Collaborator

I know the compiler is smart enough to understand that the Skippedenum is just a boolean, but does it really need to be a dedicated type ? Why not rather use a simple boolean ?

@cakebaker
Copy link
Contributor

cakebaker commented Feb 24, 2025

I'm not a fan of using a boolean, as it often makes the code harder to read.

Maybe it helps to think about what the function does:

  • it copies a file,
  • or it skips a file,
  • or it fails

Thus, we have three potential outcomes: two success cases and one error case. My suggestion is to explicitly express the success cases as an enum, something like:

enum SomeName {
    Copied,
    Skipped,
}

@BigPapa314
Copy link
Contributor Author

I'm not a fan of using a boolean, as it often makes the code harder to read.

Maybe it helps to think about what the function does:

* it copies a file,

* or it skips a file,

* or it fails

Thus, we have three potential outcomes: two success cases and one error case. My suggestion is to explicitly express the success cases as an enum, something like:

enum SomeName {
    Copied,
    Skipped,
}

I also like self explaining variables and return types. And I agree that:

enum Skipped {
    No,
    Yes
} 

is a bad name. I reused the enum Preserve logic.

What do you think about:

enum PerformedAction {
    Copied,
    Skipped
} 

@RenjiSann
Copy link
Collaborator

Sounds good to me 👍

@RenjiSann
Copy link
Collaborator

You can fixup the rename commit ?

@BigPapa314 BigPapa314 force-pushed the cp--fix-don't-show-msg-with-cp--i--v---update=older-a-b-if-a-is-older-than-b branch from 1297a76 to f6c70f3 Compare February 24, 2025 20:08
@RenjiSann
Copy link
Collaborator

I mean your third commit should be stashed in the first or second one.
The Skipped enum you create in your first commit don't exist anymore in the third one, so there's no reason it should appear in the commit history of the main branch

@BigPapa314 BigPapa314 force-pushed the cp--fix-don't-show-msg-with-cp--i--v---update=older-a-b-if-a-is-older-than-b branch from f6c70f3 to 63fe4fb Compare February 24, 2025 21:24
@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@RenjiSann
Copy link
Collaborator

LGTM 👍

@cakebaker cakebaker merged commit 15eaeae into uutils:main Feb 26, 2025
64 of 65 checks passed
@cakebaker
Copy link
Contributor

@BigPapa314 thanks for your PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cp: don't show msg with cp -i -v --update=older a b if a is older than b

4 participants