Why SSH_FX_PERMISSION_DENIED?

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03

[EPERM]
Operation not permitted. An attempt was made to perform an operation limited to processes with
appropriate privileges or to the owner of a file or other resource.

Consult https://elixir.bootlin.com/linux/latest/C/ident/EPERM and look for basically any use therein. There are far more uses of EPERM for “Operation not permitted” rather than simply “file permissions do not allow this operation”.

For example: https://man7.org/linux/man-pages/man2/mkdir.2.html

EPERM  The filesystem containing pathname does not support the creation of directories.

Clients are far more likely to be operating under the assumption that os.ErrPermission is a “do not retry this operation, but handle is still open for further use”, while SSH_FX_FAILURE is so generic that a client cannot really recover from the failure programmatically at all, as it is completely undefined and unknown whether any further operations are possible at that point.

Who is "we"?

This package, and the users of this package. I am specifically excluding any people who are forking this package. They are not “we”—the users of this package—because by using a fork, they are necessarily using a different package.

I think this principle is useful for deciding if one wants to invest the time into implementing something.. But in this case it is already implemented, and IMO is cleaner (for the reason described above).

Part of software development is deleting unnecessary code.

The original form of Server has used a map[string]*os.File since it was first introduced in a merged in September 2015. We have only now 9 years later, seen good cause to convert this into an interface. And this is despite already rolling out a better dynamic server API in RequestServer, because of which we have restricted the focus of Server to be strictly a very simple, thin, and transparent layer over the underlying OS filesystems. And the only reason we’re picking this up, is because Windows is a unique OS among all of the OSes that Golang supports.

When I tell you, “we’re not going to need that”, I mean it. It’s been almost a decade until we even needed the interface for a second implementation. There is no reason to prepare for a future that we’re unlikely to ever need. And “yeah, but I already did the work” does not negate, “and now we have to maintaining it.”

Indeed, in the very PR that introduced the map[string]*os.File an earlier version did implement this as an interface. But this was removed in favor of a simple map[string]*os.File presumably after realizing that Sever can simply use the concrete type, and so we didn’t need the interface. So, that work was undone, because it was unnecessary.

But don't expect me to change this while not giving any clear reasons, thanks 🙏

You are now, as you have been this whole time, under no obligation to do the work of implementing this PR. You volunteered for this task. However, in opposition to your ability to work on this at whatever pace you want all the way up to and including at any time “taking your ball and going home”, the same expectations are not on us as the contributors.

In order for your PR to be effected, I, or another contributor to this package, must sacrifice time out of our lives in order to engage with you, and polish the PR to the point, where we are comfortable with the code enough to merge it into this widely used and adopted package, and failure to respond promptly would be disrespectful to the time and effort you’ve put in. Even that means it comes at the expense of our weekend, or evenings. (Since I certainly don’t have the spare time during my work day to do this volunteer work.)

You must certainly understand that while you might commit this code, and then move on to other projects, the contributors of this repo have volunteered to stick around and support all of the code in this repo long-term. It is not unreasonable for us to push back on overly complex code implementing features that we are not using, and see no immediate utility to in the future.

If this dummyFile type is so useful that it becomes necessary, then we can refactor our code at that point. Until then, we have no use for this code.

But don't expect me to change…

And we are under no obligation to merge your code. I reasonably suggested to you that you should remove it, and even before this more detailed and extensive post, I had provided sufficient explanation as to why. And now almost 2 hours of research in order to pedantically track down references to provide context, and dive through the commit history of this package in order to dig up points to justify my argument, I’m unsure what you ever expected my behavior to be.

I am a volunteer that no matter what else I do stand in representation for the group of all contributors who are volunteering their time. Being directly rude, actively aggressive, or abusive would be wholly inappropriate, and reflect poorly on the whole package’s contributors. So, I must be patient, but insistent. If you interpret this professional courtesy as “passive aggressive”, then I’m sorry.

If you have any feedback I’m happy to listen to it. But unfortunately, as a person who will be responsible for maintaining this code after your PR is merged, I continue to insist that this change be made, whether you find any rational reason behind it or not.