dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> jobtest.cpp:535
> + xattrwriter.start(command, arguments);
> + xattrwriter.waitForFinished(-1);
> +
QVERIFY(....) around all calls to waitForFinished
(repeats)
> jobtest.cpp:537
> +
> + QHash<QString, QString> attrs;
> + attrs["user.name with space"] = "value with spaces";
This entire QHash is duplicated as is between both methods, right?
Extract it into a function that returns the QHash (by value).
> jobtest.cpp:556
> + xattrwriter.waitForStarted();
> + QVERIFY(xattrwriter.state() == QProcess::Running);
> + xattrwriter.waitForFinished(-1);
Whenever you see QVERIFY(a==b) it should in fact be QCOMPARE(a, b) so that we
can see the value on failure (this isn't gtest).
If needed, cast to int.
[repeats]
> jobtest.cpp:660
> + job->setUiDelegate(nullptr);
> + job->exec();
> + compareXattr(src, dest, command); // Our test
always use QVERIFY2(job->exec(), qPrintable(job->errorString()));
(repeats)
> jobtest.cpp:752-764
> + QString command = QStandardPaths::findExecutable("setfattr");
> + if (command.isEmpty()) {
> + command = QStandardPaths::findExecutable("setextattr");
> + if (command.isEmpty()) {
> + command = QStandardPaths::findExecutable("xattr");
> + }
> + }
Please extract a function from those 12 lines, which are duplicated.
> jobtest.cpp:766
> +
> + QUrl u = QUrl::fromLocalFile(src);
> + QString dest(_dest);
const
> jobtest.cpp:768
> + QString dest(_dest);
> + QUrl d = QUrl::fromLocalFile(dest);
> +
const
> cochise wrote in copyjob.cpp:1119
> I tried various ways to call this as a subjob, but all of them leads to
> breakage of non xattr related tests. Maybe some major changes to the class
> are needed.
>
> But the call can be asynchronous with little effort. All tests still pass if
> `start()` is called, instead of `exec()`.
That sounds racy; the subjob might not be finished by the time the main job is
finished, if you just "start and forget".
I cannot accept this commit with an exec() in here. The easy and modern way to
make this async is just a connect and a lambda (which contains the rest of the
code here).
I have to wonder though: do we have a fast way to determine whether we actually
need the additional subjob? (to avoid making this slower on systems -- or files
-- without xattr)
Hmm, or a much bigger architectural question: why doesn't kio_file copy the
xattr as part of FileProtocol::copy(), as it already does with e.g. permissions
and ACLs, instead of doing this in a separate job?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17816
To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika,
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella,
kde-frameworks-devel, LeGast00n, GB_2, michaelh
