D17816: Support for xattrs on kio copy/move

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191224/5a92311a/attachment-0001.html>