lf icon indicating copy to clipboard operation
lf copied to clipboard

Very serious - Folders being displayed as empty after being renamed

Open rafcon-dev opened this issue 3 years ago • 8 comments

Hi. So, I've found a pretty serious bug that could easily lead to some data loss, as it'd led me had I not backups.

How ot reproduce: Have two folders: folderA - has files inside folderB - is empty

delete folderB rename folderA to folderB now folderB (previsouyl folderA) will appear empty, even though it's not!

I'm guessing this is due to some sort of caching, but it's quite dangerous. Shouldn't the folders refresh every second or something like that?

Edit: Indeed, I just tested lf and dolphin opened side by side. If I create a file in lf, it shows up in dolphin after a second. If I do the same in dolphin, it doesn't show up in lf! This unfortunately makes lf unusable, tbh. If you need help in fixing this one, I can give a hand.

Edit2: I just tested with ranger, and it indeed reacts to a new created file in dolphin, as it should. Midnight Commander doesn't, but if you can refresh the folder with CTRL+R (which sucks btw)

rafcon-dev avatar Aug 20 '22 08:08 rafcon-dev

Read about the period option in the documentation.

lahwaacz avatar Aug 20 '22 08:08 lahwaacz

If you do the "How to reproduce" steps in lf instead of an external file manager, it behaves correctly – i.e. the folderA does not appear empty. So there is no bug.

lahwaacz avatar Aug 20 '22 09:08 lahwaacz

Indeed, the period option should give the expected behaviour. It is very weird that it is not defaulted to 1, I noticed someone just a few days ago with the same issue, this is clearly a usability problem that goes against what's usually expected of a file manager. I've noticed that dolphin does the same thing at times, which also sucks.

Regarding the bug, that's a classic "It works for me!" mate. Here's a gif so I can more clearly explain myself.

lfbug

What's worse is that not even the period option at 1 does anything to solve this.

rafcon-dev avatar Aug 20 '22 11:08 rafcon-dev

The motivation for zero period by default is explained in the FAQ: "Features that have a chance to cause performance slowdown are turned off by default."

Can you show your lfrc?

lahwaacz avatar Aug 20 '22 11:08 lahwaacz

hmm, I mean I'm all for performance, but the main goal of a file manager is to display the files that currently exist in a folder. If it fails at that for whatever reason, it's unfortunately failing it's main goal. It's 2022, we have crappy fully responsive apps written in freaking JS but we can't have a responsive file manager in the terminal? It don't think such a basic thing would be considered "performance bloat". I get it might lag on remote folders, but couldn't it be asynced or something?

As for the lfrc, here it is, nothing special really.

set ignorecase true

set icons true
set shell zsh


set shellopts '-eu'


set ifs "\n"


set previewer ~/.config/lf/preview
set cleaner ~/.config/lf/cleaner


cmd fzf_jump ${{
    res="$(find . | fzf --reverse --header='Jump to location' | sed 's/\\/\\\\/g;s/"/\\"/g')"
    if [ -d "$res" ] ; then
        cmd="cd"
    elif [ -f "$res" ] ; then
        cmd="select"
    else
        exit 0
    fi
    lf -remote "send $id $cmd \"$res\""
}}
map <c-t> :fzf_jump

cmd z %{{
    result="$(zoxide query -- "$1")"
    lf -remote "send ${id} cd '${result}'"
}}

cmd zi ${{
    result="$(zoxide query -i -- "$1")"
    lf -remote "send ${id} cd '${result}'"
}}

map Dd delete

cmd mkdir %{{
	printf "Directory Name: "
	read ans
	mkdir $ans
    	lf -remote "send ${id} cd '${ans}'"
}}

map R reload
set period 1

map c clear
map Md mkdir
map gd cd ~/Downloads

map n jump-prev
map m jump-next

rafcon-dev avatar Aug 20 '22 12:08 rafcon-dev

Hmm, now for me the caching issue sometimes happens and sometimes it doesn't...

lahwaacz avatar Aug 20 '22 12:08 lahwaacz

"Good" to know. There's definitely something devious going on under the surface mate!

rafcon-dev avatar Aug 20 '22 12:08 rafcon-dev

Can you try chaining commands for this action? Something like this:

map r :rename; reload

DusanLesan avatar Aug 27 '22 10:08 DusanLesan

I had the same issues, also if I created new directories or files in them via a self built command in lfrc.

DusanLesans comment helped out, after chaining my commands with reload evereything works as expected.

(set period ... did not help).

EDIT: no, does not help always, happens randomly.

dase78 avatar Dec 24 '22 13:12 dase78

This is caused by the dircache feature, which caches dir data structures by its path: https://github.com/gokcehan/lf/blob/b47cf6d5a525c39db268c2f7b77e2b7497843b17/nav.go#L419-L430

The problem is that when deleting a directory, the corresponding entry is not deleted from the cache. This is the code that performs the delete: https://github.com/gokcehan/lf/blob/b47cf6d5a525c39db268c2f7b77e2b7497843b17/nav.go#L1392

This means that when another directory is renamed to the path of the deleted directory, it will load the stale entry from the cache, causing it to be displayed incorrectly.

Setting the period option won't help either, since it only checks if the contents of a directory have changed, not if the directory itself has moved to a new location.

It looks like deleting the stale entry immediately doesn't work, since lf thinks the deleted directory is still the current file and tries to reload it again. I think it would be easier to just ensure there won't be a stale entry immediately after a rename action:

--- a/nav.go
+++ b/nav.go
@@ -1426,6 +1426,7 @@ func (nav *nav) rename() error {
                return err
        }

+       delete(nav.dirCache, newPath)
        dir := nav.loadDir(filepath.Dir(newPath))

        if dir.loading {

In the meantime, you can also disable directory caching by adding set nodircache to your config file.

joelim-work avatar Feb 04 '23 03:02 joelim-work

In the meantime, you can also disable directory caching by adding set nodircache to your config file.

No, please don't do this!

If you set "nodircache" lf does not remember what you selected the last time and deletes (if you delete) where you are on at the moment. I had this several times now and deleted things I did not want to delete. It seems like a mixture of bug and sluttery (by myself) to me.

dase78 avatar Feb 09 '23 22:02 dase78

If you set "nodircache" lf does not remember what you selected the last time and deletes (if you delete) where you are on at the moment. I had this several times now and deleted things I did not want to delete. It seems like a mixture of bug and sluttery (by myself) to me.

@dase78 Do you have a minimal set of steps to reproduce this issue? I don't actually use set nodircache myself, so I'm not aware of any issues that it might cause.

joelim-work avatar Feb 10 '23 01:02 joelim-work

@joelim-work Yes, I reproduced it now again: set nodircache in ~/.config/lf/lfrc Go to any directory and delete any file which is not in first line in directory (with the builtin delete command). It will delete the first file even if you are asked to confirm the chosen file.

Could you also try, this seems serious?

dase78 avatar Feb 10 '23 07:02 dase78

@dase78 I tried it just now with a config file containing nothing but set nodircache, using a few files named from a to e. But I still couldn't reproduce your issue. Maybe someone else reading this can try for themselves?

https://user-images.githubusercontent.com/50560759/218084691-4250e63d-5cfe-4019-8924-4c61ca364a75.mp4

joelim-work avatar Feb 10 '23 11:02 joelim-work

BTW, this is the code that performs the actual delete. The first thing it does is call nav.currFileOrSelections, which returns a list of selected files, or the file at the current position if there aren't any selected files.

https://github.com/gokcehan/lf/blob/b47cf6d5a525c39db268c2f7b77e2b7497843b17/nav.go#L1377-L1397

If you want to get to the bottom of this, you can try adding some log.Printf statements into the code to help debug the issue.

joelim-work avatar Feb 10 '23 12:02 joelim-work

Oh, and also I am starting to think that this is a separate problem and a new issue should be created, rather than discussing it here.

joelim-work avatar Feb 10 '23 12:02 joelim-work

So I tried again several things, I think I know, what was happening:

I could reproduce my failure but I guess it was, because I had set: map <delete> :delete; reload, so when I pushed , it asked for confirmation for the right file but then deleted the new "reloaded" one. I hope, this makes sense, how I explained.

I took out the "reload" and now everything works as expected with deleting files.

[I don't know, should we report this?] Can you reproduce?

dase78 avatar Feb 10 '23 12:02 dase78

I could reproduce my failure but I guess it was, because I had set: map :delete; reload, so when I pushed , it asked for confirmation for the right file but then deleted the new "reloaded" one. I hope, this makes sense, how I explained.

@dase78 I tried this just now and can confirm that is the problem you're experiencing.

The reload command clears the directory cache (nav.dirCache) and reloads any directories, but also stores the file at the current position (in nav.dirs[len(nav.dirs)-1].files) so that it can be restored afer the reload:

https://github.com/gokcehan/lf/blob/b47cf6d5a525c39db268c2f7b77e2b7497843b17/nav.go#L568-L581

When a directory is reloaded, the current file is initially reset to the first entry. However the original current file prior to the reload will be restored (via the sel function) if the directory cache is enabled:

https://github.com/gokcehan/lf/blob/b47cf6d5a525c39db268c2f7b77e2b7497843b17/app.go#L391-L403

This is why the current file after a reload remains unchanged if directory caching is enabled, and reset to the first entry if directory caching is disabled. I believe this is by design though, so I don't think there's any issue there.

Adding map <delete> :delete; reload will also solve the original issue, because reload clears the directory cache, so there is no stale entry when the directory is renamed. But I think it would be a lot better to actually fix this bug inside the application itself, rather than forcing users to have this kind of workaround in their config file.

joelim-work avatar Feb 11 '23 03:02 joelim-work

I think it's also worth pointing out that this issue also occurs when you rename the original directory to something else instead of deleting it.

The original example given was:

folderA - has files inside folderB - is empty

delete folderB rename folderA to folderB

If you change the step delete folderB to rename folderB to folderC, this issue will also occur, despite the fact that nothing is actually being deleted.

joelim-work avatar Feb 11 '23 03:02 joelim-work

But I think it would be a lot better to actually fix this bug inside the application itself, rather than forcing users to have this kind of workaround in their config file.

Yes, I agree.

And chaining reload to my several commands (deleting, renaming, creating new dir/file, pasting) did not always solve the original issue, but I could not always reproduce it - not updating the info happened qutite randomly. But setting nodircache seems better at the moment.

dase78 avatar Feb 11 '23 07:02 dase78