Error in second lf instance when using "paste; clear" to cut/paste files

[karabaja4]

Steps to reproduce:

1. Have the following in your config:

map x cut
map v :paste; clear

2. Open two instances of lf.
3. Go into the instance 1.
4. Cut the file with "x" and paste it somewhere else with "v" in the same instance.

The second instance will show the following error:

sync: unexpected option to copy file(s):

The file is still moved successfully.

[joelim-work]

I can reproduce this too, about half the time. After some investigation, I found that this is because of a timing issue where instance 2 is reading from ~/.local/share/lf/files (the list of cut/copied files) while instance 1 is writing to it.

Consider the code below, which is called when cutting and pasting files:

lf/nav.go

Lines 1483 to 1496 in beedda2

	go nav.moveAsync(app, srcs, dstDir)
	if err := saveFiles(nil, false); err != nil {
	return fmt.Errorf("clearing copy/cut buffer: %s", err)
	}
	if gSingleMode {
	if err := nav.sync(); err != nil {
	return fmt.Errorf("paste: %s", err)
	}
	} else {
	if err := remote("send sync"); err != nil {
	return fmt.Errorf("paste: %s", err)
	}
	}

1. The saveFiles function is called with 'blank' arguments, which clears list of cut/copied files by writing to ~/.local/share/lf/files.
2. The sync command is sent to every lf instance. At this time, the ~/.local/share/lf/files has already been written to so there is no issue here.
3. The clear command is run immediately afterwards (from the user-defined mapping above), and instance 1 opens ~/.local/share/lf/files again for writing, with the file contents truncated.
4. Instance 2 receives the sync command and opens ~/.local/share/lf/files for reading but the contents is invalid because it is empty due to step 3.
5. Instance 1 finishes writing to ~/.local/share/lf/files, but now it is too late because instance 2 has already read it and raised an error.

The error message comes from here:

lf/app.go

Lines 116 to 128 in beedda2

	s := bufio.NewScanner(files)
	s.Scan()
	switch s.Text() {
	case "copy":
	cp = true
	case "move":
	cp = false
	default:
	err = fmt.Errorf("unexpected option to copy file(s): %s", s.Text())
	return
	}

The first line of ~/.local/share/lf/files should be either copy or move, but in this case the file is actually empty because instance 1 is currently writing to it.

---

I guess it's possible to fix this by just returning if the file is blank, for example:

diff --git a/app.go b/app.go
index 4131633..3efc436 100644
--- a/app.go
+++ b/app.go
@@ -115,20 +115,23 @@ func loadFiles() (list []string, cp bool, err error) {
 
 	s := bufio.NewScanner(files)
 
 	s.Scan()
 
 	switch s.Text() {
 	case "copy":
 		cp = true
 	case "move":
 		cp = false
+	case "": // the file could be empty if another lf instance is writing to it
+		cp = false
+		return
 	default:
 		err = fmt.Errorf("unexpected option to copy file(s): %s", s.Text())
 		return
 	}
 
 	for s.Scan() && s.Text() != "" {
 		list = append(list, s.Text())
 	}
 
 	if s.Err() != nil {

Alternative;ly, I think it might be worth revisiting the discussion in #1011, which I believe is where the idea of using :paste; clear started. I'm not sure if keeping the cut/copied files after pasting is a good default, if many users are doing this.

[gokcehan]

@joelim-work The discussion in #1011 is about changing the builtin default behavior to :paste; clear but then it will probably have the same issue since it is not to different than manually setting map p :paste; clear. So the issue is that I mistakenly assumed :paste; clear is a workaround to get the old behavior when #745 is merged. I don't know what to do about this at this point? Maybe we could try reverting back the old paste behavior and find an alternative way to keep the selection (e.g. moving the selection to another file like files.old and then add a new command for something like :paste; resync). I'm not sure if this would have a similar issue as this one when using map p :paste; resync). The workaround of @joelim-work might be good enough for now. The default behavior might also be worth discussing further separately as I don't see anyone else than @laktak so far in these discussions to expect this behavior.

[joelim-work]

@karabaja4 I spent some more time investigating this, it looks like the truncation happens immediately when ~/.local/share/lf/files is opened, but writes to the actual file can be delayed unless it is forcefully synced. I think the following patch is not 100% foolproof, but in practice it works, at least when I tested it:

diff --git a/app.go b/app.go
index 4131633..0c40d6c 100644
--- a/app.go
+++ b/app.go
@@ -157,20 +157,21 @@ func saveFiles(list []string, cp bool) error {
 	if cp {
 		fmt.Fprintln(files, "copy")
 	} else {
 		fmt.Fprintln(files, "move")
 	}
 
 	for _, f := range list {
 		fmt.Fprintln(files, f)
 	}
 
+	files.Sync()
 	return nil
 }
 
 func (app *app) readHistory() error {
 	f, err := os.Open(gHistoryPath)
 	if os.IsNotExist(err) {
 		return nil
 	}
 	if err != nil {
 		return fmt.Errorf("opening history file: %s", err)

I am thinking of submitting this as an actual PR if there are no issues, so it would be nice if you could test it too, just to be sure. I think this patch is cleaner than the one I proposed earlier about ignoring empty files.

---

@gokcehan Regarding the default behavior of paste, TBH I find the idea of keeping the file selection after pasting to be confusing and increases the chance of making unintentional copies, and that it would be better if this behavior is opt-in (i.e. not the default). However, I'm pretty sure that most other file managers do keep file selections, so it really comes down to what you value more - sensible (?) defaults, or conformance with other file managers. I think both approaches can work, for instance if file selections are cleared by default, then it should be possible to somehow map p to backup the file first before executing paste.

[karabaja4]

@joelim-work It works correctly with that patch, or at least I'm unable reproduce the problem anymore described in the original post. Thanks!

[joelim-work]

Thanks, I have submitted a PR: #1480