[Git] Be safer about removing files
Needs RevisionPublic

Authored by ngraham on Nov 20 2019, 10:06 PM.

Details

Reviewers
meven
Group Reviewers
Dolphin
Summary

Right now, performing a Git remove" operation on the file destroys it, passing
the --force option, which is dangerous because it can destroy your files in the case
where you accidentally stage a file when then want it un-staged. Because the file was
not yet added to the history, it's not recoverable. And because the file was was deleted
using git, it's not put in the trash or made an undoable operation.

This patch fixes that by replacing the --force argument with --cached, which uses
a safe behavior by default and keeps the file on disk rather than destroying it.

As a result, removed files must be manually deleted, moved to the trash, etc. Arguably
this is saner, more user-friendly behavior in general, especially for the kind of person
who prefers to use a GUI tool to interact with git.

BUG: 414342
FIXED-IN: 19.12.0

Test Plan

Create a new file
right-click on that file > Git add
right-click on that file > Git remove

Before: file is destroyed!

After: file is un-staged and remains there

Diff Detail

Repository
R449 Plugins for Dolphin
Branch
safer-git-rm (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19006
Build 19024: arc lint + arc unit
ngraham requested review of this revision.Nov 20 2019, 10:06 PM
ngraham created this revision.
ngraham edited the summary of this revision. (Show Details)Nov 20 2019, 10:10 PM
zachus added a subscriber: zachus.Nov 20 2019, 10:23 PM

this is a huge usability step forward. people do not want to accidentally suffer file loss, which is quick to "achieve" with Dolphin's git-menu.

This cache business is absolutely the way to go! :-)

It means that after the "git rm" we can't do a "git checkout" to a branch that existed before deleting the file. A common case.
It seems like it'll make life harder for all users.

IMHO we shouldn't do anything, but if we insist that we should, I would suggest.

don't do --force or --cached

just have the git rm fail, and show a prompt if it does saying to revert first.

Here's another idea: intelligently detect whether the file is in the history already. If not, add --cached so it doesn't bet blown away. If so, just do the rm normally.

What about a dialog that asks the user what to do (--cache, --force, neither) and tell the user what will happen to the file?

zachus added a comment.Dec 2 2019, 8:15 PM
tell the user what will happen to the file?

users MUST be made aware of the danger of irreversible file loss. call the option "nuke my file" if need be. make it abundantly clear even to newbies that they potentially run into FILE LOSS right there and then.

meven requested changes to this revision.Mar 20 2020, 11:28 AM
meven added a subscriber: meven.

I am putting this a request review to get it out of my to review, since there is obviously some work to do:

What about a dialog that asks the user what to do (--cache, --force, neither) and tell the user what will happen to the file?

Seems to me the best option. This implies checking if we have file concerned first.

This revision now requires changes to proceed.Mar 20 2020, 11:28 AM