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

Explain the reasons of the fork better #151

Closed
P-E-Meunier opened this issue Mar 19, 2023 · 9 comments
Closed

Explain the reasons of the fork better #151

P-E-Meunier opened this issue Mar 19, 2023 · 9 comments

Comments

@P-E-Meunier
Copy link

As the original author of the project, I would have appreciated a little email to explain why you weren't able to contribute your changes back.

Also, if you don't explain what "extra safety guarantees" you added, please remove that line in the readme: either you found safety problems in my code and you should have reported them, or you didn't, and you can't claim my code needed extra safety.

@Eugeny
Copy link
Owner

Eugeny commented Mar 20, 2023

I've submitted my changes as patches over a year ago and got no response: https://nest.pijul.com/pijul/thrussh/discussions/69

extra safety guarantees refers to removing all sources of panicking in the library such as .expect(), .unwrap() and unchecked indexing - it's explained in the readme

@P-E-Meunier
Copy link
Author

I've submitted my changes as patches over a year ago and got no response: https://nest.pijul.com/pijul/thrussh/discussions/69

I see no change submitted in that discussion, only links to diffs. I don't think you would accept PRs to this repo formatted as links to diffs against an unnamed commit. Also, you started this fork before submitting these.

extra safety guarantees refers to removing all sources of panicking in the library such as .expect(), .unwrap() and unchecked indexing - it's explained in the readme

Did you identify places where an .unwrap() or .expect() can cause a safety problem? I believe all unwraps are legit in Thrussh, since the panicking branch is unreachable.

I would suggest reformulating your "extra safety guarantees" as "using more lints (from Clippy)".

@Eugeny
Copy link
Owner

Eugeny commented Mar 20, 2023

I don't think you would accept PRs to this repo formatted as links to diffs against an unnamed commit. Also, you started this fork before submitting these.

This is beyond the point. You've made it unreasonably hard to contribute by rolling your own VCS and failed to communicate back when offered a changeset in alternative way. If you look at the URLs in the post, you can see that I haven't even renamed the repo at the time and have only been using it as code storage.

Did you identify places where an .unwrap() or .expect() can cause a safety problem? I believe all unwraps are legit in Thrussh, since the panicking branch is unreachable. I would suggest reformulating your "extra safety guarantees" as "using more lints (from Clippy)".

I mean things like line 341 where a misbehaving server causes the library to panic, or .unwrap() on file I/O like on line 325 Anyway, changed the wording ✌️

@P-E-Meunier
Copy link
Author

This is beyond the point. You've made it unreasonably hard to contribute by rolling your own VCS and failed to communicate back when offered a changeset in alternative way.

This was exactly on point, actually: one of the reasons I've published Thrussh as open source was to encourage people to use Pijul by contributing to interesting repositories. I must admit I didn't imagine that someone would want to contribute to Thrussh and at the same time find it "unreasonably hard" to install a Rust crate and type two commands (especially when the alternative is something as broken as Git).

Actually, you made it hard to accept your contributions by refusing, for purely ideological reasons, to use the tools used by the community around that library.

I mean things like line 341 where a misbehaving server causes the library to panic, or .unwrap() on file I/O like on line 325

Indeed, that sort of things is usually fixed in the open source community by submitting changes, or at the very least bug reports. No need to go through the "unreasonable" pain of running cargo install for that, you already submitted one, I know you were able to do it.

@Eugeny
Copy link
Owner

Eugeny commented Mar 20, 2023

I must admit I didn't imagine that someone would want to contribute to Thrussh and at the same time find it "unreasonably hard" to install a Rust crate and type two commands (especially when the alternative is something as broken as Git).

I was actually excited to try something other than Git and Mercurial for once, but dang, with user support like this, I wonder why more people aren't using it.

by refusing, for purely ideological reasons, to use the tools used by the community

something as broken as Git

Ironic, isn't it?

I've tried my best to get it to work and failed anyway. So maybe instead of assuming "ideological reasons" behind someone else's actions, you should have focused on making Pijul work.

@Eugeny Eugeny closed this as completed Mar 20, 2023
@P-E-Meunier
Copy link
Author

P-E-Meunier commented Mar 20, 2023

with user support like this, I wonder why more people aren't using it

Maybe because this was never directly reported as an issue in the Pijul or Thrussh repos, nor even mentioned or discussed in the community platforms (Zulip, Discourse)?

Ironic, isn't it?

I don't see why, but maybe you could explain the irony.

@Eugeny
Copy link
Owner

Eugeny commented May 31, 2024

@P-E-Meunier guess it just doesn't get better than straight up lying in an invite-only community where you know you won't get called out: https://lobste.rs/s/bikpmo/russh_rust_ssh_client_server_library

The original motivation for the fork was aggressive (they didn’t talk to us initially) and was motivated by a misunderstanding of whether a feature was present or not.

hostile forks by people just adding micro-features and claim ownership

forking it to [...], claim ownership of my work

“More secure” and “up to date” was in their README at some poin

@P-E-Meunier
Copy link
Author

P-E-Meunier commented May 31, 2024

So, now in addition to being a terrible person for being upset that you never upstreamed your changes, I'm also a liar and conspiring against you.

Did I lie by saying it was in the readme rather than in this issue? I'm so sorry.

Also, why not quote me completely? For example when I wrote the following:

(note that I’m not even complaining about the fork, just about how its hostile nature: never contributing anything back)

@Eugeny
Copy link
Owner

Eugeny commented May 31, 2024

So, now in addition to being a terrible person for being upset that you never upstreamed your changes, I'm also a liar and conspiring against you.

Yes, unless you want to explain why you're spreading literal false information.

Did I lie by saying it was in the readme rather than in this issue? I'm so sorry.

It's literally in neither, at any point of time.

Also, why not quote me completely?

(note that I’m not even complaining about the fork, just about how its hostile nature: never contributing anything back)

Because I didn't want to embarrass you more than necessary, as that's literally in my first response

You've simply ignored the contributions offered, so I just assumed that the project that hasn't had a commit in 6 months at that point in time is unmaintained, forked it, and kept silently working on my project that I needed the library for - until you have opened this issue to cry about getting forked, and only then resumed work on thrussh, 1.5 years since the last update.

You're welcome to pull in all of my changes, in fact, if you do, I'll replace this crate with a placeholder pointing to thrussh.

@Eugeny Eugeny pinned this issue Jun 2, 2024
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

No branches or pull requests

2 participants