Skip to content

Conversation

@greatroar
Copy link
Contributor

Doing this loses potentially useful information for the client. They should instead check with errors.Is(err, fs.ErrNotExist), etc.; the StatusPacket type has an Is method for this already.

Doing this loses potentially useful information for the client. They
should instead check with errors.Is(err, fs.ErrNotExist), etc.; the
StatusPacket type has an Is method for this already.
@puellanivis
Copy link
Collaborator

puellanivis commented Jun 1, 2025

Hm… 🤔 I can’t see any reason against this. It’s also for the dev-v2 branch, and so the potential change of the behavior would be isolated to a future breaking change.

And as noted, unlike io.EOF that gets a lot of regular == io.EOF checks in the standard library and elsewhere, these errors really should be using errors.Is(err, fs.ErrNotExist) and errors.Is(err, fs.ErrPermission) as called out in the os.IsNotExist and os.IsPermission.

The openssh-portable sftp-server does not return any more useful information in the error message for these, but other servers might (like ours).

@greatroar
Copy link
Contributor Author

IIRC, the translation to os.ErrNotExist was actually a breaking change introduced in a minor revision, before errors.Is was a thing.

@puellanivis
Copy link
Collaborator

It seems was introduced before this package even released as v1.0.0.

Regardless, we have the better option now, and v2 should support the new way rather than keep an old way.

@puellanivis puellanivis merged commit 2d30c45 into pkg:dev-v2 Jun 2, 2025
5 checks passed
@greatroar greatroar deleted the dev-v2-errors branch June 3, 2025 09:28
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