Commit af3e3f95 authored by Igor Kushnir's avatar Igor Kushnir
Browse files

GitPlugin::remove: delete files, not trash them

This is the only usage of KIO::trash() in KDevelop's code base. I don't
think it is appropriate here. The trashing occurs only if the file list
contains a file not under version control or contains only directories,
no files. Otherwise, `git rm -r --force` is used, which simply deletes
files. So using KIO::trash() introduces a gratuitous inconsistency.

I noticed this issue by examining my Trash. It contained lots of
directories named "dir" and "emptydir", lots of files named "bar" and
"testfile" removed from /tmp/kdevGit_testdir/. These files and
directories are put to Trash each time test_kdevgit runs.

4b50220a introduced the first call to
KIO::trash(). Later commits that touched this code simply followed suit
or ported to a new API version. The author of the original commit Aleix
Pol Gonzalez explained in the review: "the initial idea was that this
was a way for never removing unrecoverable data. git rm will still have
it in the git history, those that aren't part of it would go to the

The only use of the affected function GitPlugin::remove() (or rather the
function it overloads IBasicVersionControl::remove()) outside of tests
(as found by KDevelop's Show uses), is in KDevelop::removeUrl(). This
single caller invokes KIO::del() if the URL to be removed is not under
version control. The removeUrl() function itself is called safely
enough: after a "Do you really want to delete these %1 items?" prompt OR
in a move operation after successful copying OR in a rename operation
after successful saving-as. So KDevelop deletes files unrecoverably
already, but under certain conditions puts them to Trash instead. Best
to get rid of this surprising inconsistency.
parent 8ec324c8
Pipeline #196082 passed with stage
in 28 minutes and 6 seconds
......@@ -42,6 +42,7 @@
#include <KDirWatch>
#include <KIO/CopyJob>
#include <KIO/DeleteJob>
#include <KLocalizedString>
#include <KMessageBox>
#include <KTextEdit>
......@@ -629,16 +630,16 @@ VcsJob* GitPlugin::remove(const QList<QUrl>& files)
auto trashJob = KIO::trash(otherFiles);
auto deleteJob = KIO::del(otherFiles);
qCDebug(PLUGIN_GIT) << "other files" << otherFiles;
if (fileInfo.isDir()) {
if (isEmptyDirStructure(QDir(file.toLocalFile()))) {
//remove empty folders, git doesn't do that
auto trashJob = KIO::trash(file);
auto deleteJob = KIO::del(file);
qCDebug(PLUGIN_GIT) << "empty folder, removing" << file;
//we already deleted it, don't use git rm on it
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment