Please monitor pull requests carefully

[samclaus]

Hello, I am currently working on a big fork of this repo because the company I work for needs a good Golang SFTP library and, quite frankly, this package is really not up-to-par.

In my fork I'm already addressing several ease of use issues, like #277 and many others mention, as well as numerous bugs, such as LinkPath and TargetPath being in the wrong order for SSH_FXP_SYMLINK packets.

Additionally, the code in the repo is very inconsistent from one file to another and looking at the blame I can see that many people worked on it but that just illustrates how important it is to have a single person mediating commits on the basis of style and structure.

Another issue is performance: reading over all the packet marshaling code reveals that a ton of extra allocations and copies are made even after people went to all the trouble to manually write marshalling functions for every packet type for the sake of speed.

Not even internal errors are consistent:

// This whole function is ugly and contains extra copies
func (p *sshFxpDataPacket) UnmarshalBinary(b []byte) error {
	var err error
	if p.ID, b, err = unmarshalUint32Safe(b); err != nil {
		return err
	} else if p.Length, b, err = unmarshalUint32Safe(b); err != nil {
		return err
	} else if uint32(len(b)) < p.Length {
		return errors.New("truncated packet") // should be errShortPacket
	}

	p.Data = make([]byte, p.Length)
	copy(p.Data, b)
	return nil
}

Sorry to go on a bit of a tirade but as I write this I have already spent way more time peeling over the SFTP spec and this repository and trying to understand the spaghetti than it would have taken me to just rewrite the package from scratch.

I understand that maintaining a package like this is a public service--it takes your precious time and effort for everyone else's benefit--and that is something I truly respect. However, I don't even really want to make a pull request once I'm done if I know carefully written code is just going to get trampled in the future by people new to Golang or people too lazy to read and think carefully before writing.

I am also willing to help maintain this repo and review PRs, etc. if that is desired. I will make a large pull request by the end of next week, if I can be assured that the package will be maintained properly. Otherwise I will just give my fork to my company and it will compete with this repo.

[samclaus]

All of these commits may even be before the repo was moved to maintenance, as I am not really aware of the timeline. These complaints may not even be properly directed, I just want assurance that if I push in big improvements it will not be in vain. Thank you for reading.

[eikenb]

Thanks for letting me know about the possible influx of PRs and your work to improve the library. It is a bit of a mess and any improvements are more than welcome.

I'll be happy to look over pull requests but please keep them small and targeting specific things. I don't have much time for this project right now, but can spare some here and there and if the PRs are small I can fit them in. If they are to big (requiring a substantial amount of time spent at once) I probably won't have time.

Thanks.

[eikenb]

Another thought as it sounds like you might have time... If you'd like to review and chime in on PR #285 and #290, that'd be a big help as well. They both seem like they might be OK to merge in but I just don't have the time ATM.

[samclaus]

Absolutely, I will take a look tomorrow. Thank you for responding so quickly.