Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impl fs::remove_file #66

Merged
merged 4 commits into from
Nov 16, 2021
Merged

Conversation

matthiasbeyer
Copy link
Contributor

Rel: #48

Suggestions welcome 😆

Copy link

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@matthiasbeyer
Copy link
Contributor Author

Fixed the formatting and updated the PR.

@Darksonn
Copy link

This seems to have a conflict with your other PR.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
No flags provided here, as AT_REMOVEDIR is the only flag that is
supported right now and we unlink a file here, so no need to specify
this flag.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer
Copy link
Contributor Author

Do you rebase-merge this or squash-merge?

@Darksonn
Copy link

It would get squashed.

@matthiasbeyer
Copy link
Contributor Author

Okay, that's sad, because it effectively destroys history and replicability.

@Darksonn Darksonn merged commit 1a09881 into tokio-rs:master Nov 16, 2021
@matthiasbeyer matthiasbeyer deleted the impl-remove-file branch November 16, 2021 15:49
@Darksonn
Copy link

All commits include the id of the PR, so you can view the history on the PR.

@matthiasbeyer
Copy link
Contributor Author

Still you destroy information. If I want to revert just one patch of a PR because it was faulty, I have to jump through hoops to get that one patch and then revert it! Also, attribution is basically destroyed because the commit message is rewritten completely (which means trailers are removed, so attribution is gone)! I don't know whether you rewrote the commit message by hand or whether github just "did its thing here", either way it destroyed history without providing any benefit over a simple solid merge commit.

@Darksonn
Copy link

If you want to continue this discussion, feel free to jump on our discord.

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.

2 participants