| bruns | |
| dfaure | |
| ngraham |
| Frameworks |
If the slave didn't pass a modification time (e.g. the http slave doesn't), it was set to -1,
resulting in setting a wrong modification time for the destination file in copyNextFile() later on because that
case wasn't checked.
So only set info.mtime when the slave actually provided a value.
There's no need for further checks later in copyNextFile() (where FileCopyJob::setModificationTime() is called)
because FileCopyJob checks for validity anyway.
BUG: 374420
Run kioclient5 copy "http://kde.org" /tmp/file and check the times with stat.
Before:
$ kioclient5 copy "http://kde.org" /tmp/file
$ LANG=C stat /tmp/file
File: /tmp/file Size: 19655 Blocks: 40 IO Block: 4096 regular file
Device: 39h/57d Inode: 91809 Links: 1
Access: (0644/-rw-r--r--) Uid: ( 1000/ wolfi) Gid: ( 100/ users)
Access: 2019-06-21 11:08:08.000000000 +0200
Modify: 1970-01-01 00:59:59.000000000 +0100
Change: 2019-06-21 11:08:08.288032833 +0200
Birth: -
(note the "Modify" time)
With this patch:
$kioclient5 copy "http://kde.org" /tmp/file
$ LANG=C stat /tmp/file
File: /tmp/file Size: 19655 Blocks: 40 IO Block: 4096 regular file
Device: 39h/57d Inode: 91946 Links: 1
Access: (0644/-rw-r--r--) Uid: ( 1000/ wolfi) Gid: ( 100/ users)
Access: 2019-06-21 11:28:20.369808804 +0200
Modify: 2019-06-21 11:28:20.397808804 +0200
Change: 2019-06-21 11:28:20.397808804 +0200
Birth: -
The result is still correct if the slave did provide a modification time:
$ kioclient5 copy "ftp://upload.kde.org/README" /tmp/file
$ LANG=C stat /tmp/file
File: /tmp/file Size: 594 Blocks: 8 IO Block: 4096 regular file
Device: 39h/57d Inode: 91947 Links: 1
Access: (0644/-rw-r--r--) Uid: ( 1000/ wolfi) Gid: ( 100/ users)
Access: 2019-06-21 11:28:49.000000000 +0200
Modify: 2013-04-27 00:00:00.000000000 +0200
Change: 2019-06-21 11:28:49.529808792 +0200
Birth: -
| Lint Skipped |
| Unit Tests Skipped |
| src/core/copyjob.cpp | ||
|---|---|---|
| 1709 ↗ | (On Diff #60202) | Wouldn't mtime.isNull() or mtime.isValid() be simpler? |
| src/core/copyjob.cpp | ||
|---|---|---|
| 1709 ↗ | (On Diff #60202) | I don't think mtime would be null or invalid in this case, but I'll check. |
| src/core/copyjob.cpp | ||
|---|---|---|
| 1709 ↗ | (On Diff #60202) | Nope. qDebug() << "mtime=" << (*it).mtime << "null:" << (*it).mtime.isNull() << "valid:" << (*it).mtime.isValid(); The output in the "broken" case, i.e. when it was set to -1: mtime= QDateTime(1969-12-31 23:59:59.000 UTC Qt::UTC) null: false valid: true |
Then the bug is on the other side. Whoever is putting that value into a QDateTime, is doing something wrong.
31-Dec-1969 is NOT the normal way to represent an invalid time.
Can you find out who is converting -1 to a QDateTime, and fix that instead?
Ah sorry that was in your commit log.
info.mtime = QDateTime::fromMSecsSinceEpoch(1000 * entry.numberValue(KIO::UDSEntry::UDS_MODIFICATION_TIME, -1), Qt::UTC);
should be changed to:
const auto timeVal = entry.numberValue(KIO::UDSEntry::UDS_MODIFICATION_TIME, -1);
if (timeVal != -1) {
info.mtime = QDateTime::fromMSecsSinceEpoch(1000 * timeVal, Qt::UTC);
}