SFTP-Storage: Correctly Sync mtime

[msander]

Currently uploading a file to an external SFTP storage does not keep the mtime.
Besides losing this information is bad by itself, fast repeated changing / syncing of a file on the external storage, creates conflicts randomly (maybe if a change happens during the upload?).

Using the provided change, I was not able to reproduce the issue anymore.

[MorrisJobke]

Looks good - but maybe @icewind1991 or so has another look at this.

[kesselb]

Thanks for taking the time to submit a pull request 👍

I had a short look at the code and phpseclib. Here are my thoughts:

1. touch method. I would remove the file_exists check, try-catch block and forward the call to phpseclib's sftp implementation: https://github.com/nextcloud/3rdparty/blob/10acc20198442f3c04bc1abbf9ad540a3afbab40/phpseclib/phpseclib/phpseclib/Net/SFTP.php#L1403-L1452

	/**
	 * {@inheritdoc}
	 */
	public function touch($path, $mtime = null) {
		return $this->getConnection()->touch($this->absPath($path), $mtime);
	}

It seems that the upstream implementation does everything we need.

2. loose mtime on upload. A recent commit phpseclib/phpseclib@ea653e1#diff-f0500f119a16ba4ff6303ba9008b1ce1 added a feature to preserve access and modified time on download/upload to phpseclib's sftp implementation. I guess we need something similar for our SFTPWriteStream and SFTPReadStream wrapper to keep the information. I'm not sure if touch is always called after a write operation thus it's not enough to fix only the touch method.

[msander]

1. loose mtime on upload. A recent commit phpseclib/phpseclib@ea653e1#diff-f0500f119a16ba4ff6303ba9008b1ce1 added a feature to preserve access and modified time on download/upload to phpseclib's sftp implementation. I guess we need something similar for our SFTPWriteStream and SFTPReadStream wrapper to keep the information. I'm not sure if touch is always called after a write operation thus it's not enough to fix only the touch method.

I don't think we need to handle the mtime for streams. The mtime is part of the file status, a stream just handles the content.

[msander]

Thanks for taking the time to submit a pull request

I had a short look at the code and phpseclib. Here are my thoughts:

1. touch method. I would remove the file_exists check, try-catch block and forward the call to phpseclib's sftp implementation: https://github.com/nextcloud/3rdparty/blob/10acc20198442f3c04bc1abbf9ad540a3afbab40/phpseclib/phpseclib/phpseclib/Net/SFTP.php#L1403-L1452

	/**
	 * {@inheritdoc}
	 */
	public function touch($path, $mtime = null) {
		return $this->getConnection()->touch($this->absPath($path), $mtime);
	}

It seems that the upstream implementation does everything we need.

Thank you for your suggestions, I have changed my commit accordingly.

[msander]

Thank you for your suggestions, I have changed my commit accordingly.

Today, this change has resulted in a sync conflict again.
I have changed it on our server back to the old version, which was running in production for over a week without issues:

        public function touch($path, $mtime=null) {
		try {
			if (!$this->file_exists($path)) {	
				$this->getConnection()->put($this->absPath($path), '');	
			} else {	
				$this->getConnection()->touch($this->absPath($path), $mtime);
			}	
		} catch (\Exception $e) {	
			return false;	
		}
		return true;
	}

[rullzer]

Since not many of the devs here use sftp. I think moving this to 21 to have all the things properly reviewed and possibly backported.

@kesselb any idea why your suggestions did 💥 ?

[rullzer]

@kesselb can you have another look here?

[PVince81]

@msander doing just a put might not be enough to have the exact same mtime due to network round trips.
You might need to first do a put with empty file, and then directly touch afterwards.

[blizzz]

what's the current state here?

[skjnldsv]

As there is no feedback since a while I will close this PR. If you are still willing to get this in, please address the potential comments and rebase to latest master. Then, feel free to re-open.