dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > jobtest.cpp:118 > + } else { > + qWarning() << "Xatr command not foud."; > + } typo in Xatr, and in "foud". I suggest: qWarning() << "Neither getfattr, getextattr nor xattr was found"; Although I have to wonder why this looks for all three, to then only use getfattr and skip tests in all other cases.... > jobtest.cpp:480 > + attrs["user.name with space"] = "value with spaces"; > + return attrs; > +} this method has weird indentation > jobtest.cpp:736 > + } > + setXattr(filePath); > copyLocalFile(filePath, dest); I fail to understand the code here. This is the same call as two lines above, line 734, which is inside an if(). This one should be removed? > jobtest.cpp:793 > copyLocalSymlink(filePath, dest, QStringLiteral("relative")); > + > QFile::remove(filePath); please revert no-op changes > jobtest.cpp:805 > const QString dest = otherTmpDir() + "testlink_copied"; > + > createTestSymlink(filePath, QFile::encodeName(homeTmpDir())); no-op (just empty lines) > jobtest.cpp:808 > copyLocalSymlink(filePath, dest, homeTmpDir()); > + > QFile::remove(filePath); empty line > file_unix.cpp:164 > +#endif > + QList<QByteArray> m_keyList = keylist.split('\0'); > + if (m_keyList.last().isEmpty()) m_keyList.removeLast(); // the last item > may be empty Can this *ever* return an empty list, because keylist was empty? (Then last() will assert on the next line) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh