Fix crashes when deleting to trash (#1798)

[5hir0kur0]

ISSUE TYPE

• Bug fix: This fixes Ranger crashes when deleting to Trash #1798.

RUNTIME ENVIRONMENT

• Operating system and version: Arch Linux, kernel 5.4.23-1-lts, last full pacman update at: 2020-03-05T08:20:22
• Terminal emulator and version:

rxvt-unicode (urxvt) v9.22 - released: 2016-01-23
options: perl,xft,styles,combining,blink,iso14755,unicode3,encodings=eu+vn+jp+jp-ext+kr+zh+zh-ext,fade,transparent,tint,XIM,frills,selectionscrolling,wheel,slipwheel,cursorBlink,pointerBlank,scrollbars=plain+rxvt+NeXT+xterm

• Python version: Python 3.8.1
• Ranger version/commit: v1.9.3-18-g081e7315 (output of git describe HEAD)
• Locale: en_US.UTF-8

CHECKLIST

• The CONTRIBUTING document has been read [REQUIRED]
• All changes follow the code style [REQUIRED]
  (make test_py outputs is the same before and after the change)
• All new and existing tests pass [REQUIRED]
  (make test_py output is the same before and after the change)
• Changes require config files to be updated
  • Config files have been updated
• Changes require documentation to be updated
  • Documentation has been updated
• Changes require tests to be updated
  • Tests have been updated

DESCRIPTION

The execute() method of the trash command (in ranger/config/commands.py)
used to pass a list of file paths (as strings) to fm.execute_file().
The documentation of the execute_file() method states that the 'files'
parameter must not be a list of strings:

[...]
files: a list of file objects (not strings!)
[...]

So I changed files to be a list of File objects and that seems to fix the issue.

Note: I'm not sure if I need to pass any additional parameters to the File constructor.
Passing just the path seemed to work fine in my tests.
(I'm talking about this line: files = [File(name) for name in file_names]).

MOTIVATION AND CONTEXT

Relevant issue: #1798

TESTING

I ran make test_py as I only changed a python file.

[toonn]

I think we need to use the actual File objects ranger has in memory, not create new ones. They're not singletons. If we create new ones the interface will have outdated information in memory.

[5hir0kur0]

@toonn So you mean I should do something like this?

    def find_existing_files(self, file_names):
        '''
        Find file objects corresponding to the file names that are already in
        memory and load those that are not.
        '''
        result = []
        for name in file_names:
            path = os.path.abspath(os.path.expanduser(name))
            if os.path.isdir(path):
                result.append(self.fm.get_directory(path))
            else:
                dirname = os.path.dirname(path)
                directory = self.fm.get_directory(dirname)
                directory.load_content_if_outdated()
                desired_file = trash.find_file_by_path(directory.files_all, path)
                if desired_file is not None:
                    result.append(desired_file)
                    continue
                # Exit the operation with an error message if the file could
                # not be found.
                self.fm.notify('Error: path does not exist: ' + name, bad=True)
                return None
        return result

    @staticmethod
    def find_file_by_path(files, path):
        '''
        Search a list of File objects for a file matching path.
        Assumes that path is absolute.
        '''
        if files is None:
            return None
        for file_object in files:
            if file_object.path == path:
                return file_object
        return None

Sorry if I misunderstood you; I am not very familiar with ranger's code base.

The problem with my implementation above is that it uses linear search to find files.
For directories, I found the fm.get_directory() method which seems to use a dictionary of loaded directories internally and thus should be quite fast.

But I didn't find a similar method for files, so I just do linear search in the corresponding directory, which could be a performance issue when deleting lots of files.

---

Edit: To clarify: This is for the case when the user supplies the file(s) to be deleted with the command as a string (instead of deleting the selection—in that case I am already using the File objects).

[toonn]

Something like that, yeah. I don't think the linear search can be avoided, the files are sorted but the order depends on the sorting method the user has set. Maybe you could group files by directory, turn them into a set and then go through the directory linearly once, checking the set for membership and remembering those file objects?

[5hir0kur0]

That's a good idea! I've implemented it.

---

There is another problem though: If you trash lots of files you run into the argument list length limitation of the operating system and this actually crashes ranger.
I'm not sure if this is easily fixable though. I think it is this line in rifle.conf:

label trash, has trash-put = trash-put -- "$@"

If there are lots of files, the argument list gets too long, so it should actually be split up into multiple calls to trash-put (similar to the way the command line program xargs handles this case).

While I don't really think this limitation itself is a problem, the fact that it crashes ranger is (IMO).
This can be fixed by wrapping the call to execute_file() in a try-except like so:

try:
    self.fm.execute_file(files, label='trash')
except OSError as e:
    self.fm.notify(str(e), bad=True)

However, I am not sure if this is the right place to put it. Maybe it would be better to have the fm.execute_file() method itself handle this case?

---

Here are the steps to reproduce the crash and the traceback for reference:
Steps:

:cd /tmp
:mkdir temp
:cd temp
:shell touch empty_file{1..10000}
:mark_files all=True val=True
:trash %s
(confirm prompt by pressing y)

Result: Ranger crashes with the following traceback:

ranger version: ranger-master v1.9.3-20-gefa32996
Python version: 3.8.1 (default, Jan 22 2020, 06:38:00) [GCC 9.2.0]
Locale: en_US.UTF-8
Current file: '/tmp/temp/empty_file1'

Traceback (most recent call last):
  File "/home/me/code/ranger/ranger/core/actions.py", line 465, in execute_file
    return execute()
  File "/home/me/code/ranger/ranger/core/actions.py", line 463, in execute
    return self.rifle.execute(filenames, mode, label, flags, None)
  File "/home/me/code/ranger/ranger/ext/rifle.py", line 442, in execute
    process = Popen(cmd, env=self.hook_environment(os.environ))
  File "/usr/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 7] Argument list too long: '/bin/sh'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/me/code/ranger/ranger/core/main.py", line 203, in main
    fm.loop()
  File "/home/me/code/ranger/ranger/core/fm.py", line 383, in loop
    ui.handle_input()
  File "/home/me/code/ranger/ranger/gui/ui.py", line 276, in handle_input
    self.handle_key(key)
  File "/home/me/code/ranger/ranger/gui/ui.py", line 206, in handle_key
    elif not DisplayableContainer.press(self, key):
  File "/home/me/code/ranger/ranger/gui/displayable.py", line 275, in press
    focused_obj.press(key)
  File "/home/me/code/ranger/ranger/gui/widgets/console.py", line 188, in press
    self.type_key(key)
  File "/home/me/code/ranger/ranger/gui/widgets/console.py", line 214, in type_key
    self._answer_question(answer)
  File "/home/me/code/ranger/ranger/gui/widgets/console.py", line 197, in _answer_question
    callback(answer)
  File "/home/me/code/ranger/ranger/config/commands.py", line 800, in _question_callback
    self.fm.execute_file(files, label='trash')
  File "/home/me/code/ranger/ranger/core/actions.py", line 473, in execute_file
    return execute()
  File "/home/me/code/ranger/ranger/core/actions.py", line 463, in execute
    return self.rifle.execute(filenames, mode, label, flags, None)
  File "/home/me/code/ranger/ranger/ext/rifle.py", line 442, in execute
    process = Popen(cmd, env=self.hook_environment(os.environ))
  File "/usr/lib/python3.8/subprocess.py", line 854, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.8/subprocess.py", line 1702, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 7] Argument list too long: '/bin/sh'

[5hir0kur0]

I also "fixed" the crash I mentioned above by displaying an error message instead of crashing.
It does not actually move the files to trash in this case because I don't know how to get rifle to do that (see above).

[kevinhwang91]

I hate the redraw when using trash-put, and never use it happily. So I write a simple command plugin to put the file into trash.
https://github.com/kevinhwang91/dotfiles/blob/master/ranger/plugins/trash_put.py

[kevinhwang91]

Not only trash but also delete will crash. I make a hack patch for solving this.

diff --git a/ranger/gui/widgets/statusbar.py b/ranger/gui/widgets/statusbar.py
index fd44613e..245beb62 100644
--- a/ranger/gui/widgets/statusbar.py
+++ b/ranger/gui/widgets/statusbar.py
@@ -192,11 +192,24 @@ class StatusBar(Widget):  # pylint: disable=too-many-instance-attributes
         directory = target if target.is_directory else \
             target.fm.get_directory(os.path.dirname(target.path))
         if directory.vcs and directory.vcs.track:
-            if directory.vcs.rootvcs.branch:
-                vcsinfo = '({0:s}: {1:s})'.format(
-                    directory.vcs.rootvcs.repotype, directory.vcs.rootvcs.branch)
-            else:
-                vcsinfo = '({0:s})'.format(directory.vcs.rootvcs.repotype)
+            try:
+                if directory.vcs.rootvcs.branch:
+                    vcsinfo = '({0:s}: {1:s})'.format(
+                        directory.vcs.rootvcs.repotype, directory.vcs.rootvcs.branch)
+                else:
+                    vcsinfo = '({0:s})'.format(directory.vcs.rootvcs.repotype)
+            except TypeError:
+                root_path = directory.vcs.rootvcs.path
+                for path, fdir in self.fm.directories.items():
+                    if not fdir.files:
+                        continue
+                    for fobj in fdir.files:
+                        if path.startswith(root_path):
+                            if fobj.is_directory and fobj.vcs:
+                                del fobj.vcs
+                            fobj.vcsstatus = None
+                return
+
             left.add_space()
             left.add(vcsinfo, 'vcsinfo')

[poperigby]

Any progress on this?

[toonn]

Reviews and testing reports welcome.

[poperigby]

It seems to have fixed the issue for me on Arch Linux.

[toonn]

Two small fixes needed but I'll do those myself when testing.

[toonn]

Thank you for the contribution! I'm sorry it took so long to see it through, especially because the code quality was so high!

[da-the-dev]

Has this been fixed? I'm on ranger 1.9.3 and i get the exact same error message

[humanplayer2]

I changed the keybinding in rc.conf inspired by ArchWiKi:

map dT shell gio trash %s