Skip to content

Fix: dont silently drop pasted files with newlines#2620

Merged
CatsDeservePets merged 1 commit into
gokcehan:masterfrom
valoq:paste-fix
Jun 25, 2026
Merged

Fix: dont silently drop pasted files with newlines#2620
CatsDeservePets merged 1 commit into
gokcehan:masterfrom
valoq:paste-fix

Conversation

@valoq

@valoq valoq commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

When pasting files with newlines in their name, the operation silently fails instead of throwing an error.

POC:

mkdir -p /tmp/lf-newline-demo/dest
touch "/tmp/lf-newline-demo/$(printf 'report\nDRAFT.txt')"
touch /tmp/lf-newline-demo/keep.txt
lf /tmp/lf-newline-demo

copy and paste in lf

@valoq

valoq commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

@CatsDeservePets Do you think we can get this merged for the release? Some quick tests on macOS may be good, but this shouldn't change anything major and just ensures that copy actions dont fail silently,. which I would consider a major issue for a file manager.

@CatsDeservePets CatsDeservePets merged commit 230ef16 into gokcehan:master Jun 25, 2026
32 checks passed
@valoq

valoq commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Hm, I still keep wondering if this is the right solution.

the error is better then the silent skip, but the error is show during yank, not paste. When I tried the fixed version, it felt kinda worse because paste would just do nothing with no explanation (since I oversaw the error during yank)

At least I think it may be better to unselect the newline file as before and copy the rest of the files, though that still increases the risk of the error going unnoticed, so the user may expect all files to be copied when some are skipped.

Maybe I'm overthinking things though, but I feel this should get another look

@CatsDeservePets

Copy link
Copy Markdown
Collaborator

Hm, I still keep wondering if this is the right solution.

the error is better then the silent skip, but the error is show during yank, not paste. When I tried the fixed version, it felt kinda worse because paste would just do nothing with no explanation (since I oversaw the error during yank)

At least I think it may be better to unselect the newline file as before and copy the rest of the files, though that still increases the risk of the error going unnoticed, so the user may expect all files to be copied when some are skipped.

Maybe I'm overthinking things though, but I feel this should get another look

I had similar thoughts, and I am uncertain as well. Given how niche this problem is, I still think we are spending more time on it than necessary.

@valoq

valoq commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Given how niche this problem is, I still think we are spending more time on it than necessary.

Absolutely, but now I want to do it right :)

@CatsDeservePets

Copy link
Copy Markdown
Collaborator

I'm just a little bit confused why you pushed to merge this PR if the plan was to revert it shortly after and replace it with another approach. Did you not test it before?

@valoq

valoq commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

I'm just a little bit confused why you pushed to merge this PR if the plan was to revert it shortly after and replace it with another approach. Did you not test it before?

I did, but I focused on the error message I had just added and stopped before pasting.
when I tested it again days later (when I added the POC), I just went through the steps and noticed that I didnt read the error and just noticed the not working paste.

The approach in #2630 was the original intention, the breaking of the paste command wasnt intentional. The new approach may not be perfect either (the entire newline topic could be overhauled) but it is much cleaner and just adds the warning

@valoq

valoq commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

@CatsDeservePets before this makes it into the next release, can we either get #2630 merged or just revert this one? It may not be that relevant but would be good for a peace of mind.

I will test future PRs more thoroughly, this week was pretty exhausting though.

@CatsDeservePets

Copy link
Copy Markdown
Collaborator

@CatsDeservePets before this makes it into the next release, can we either get #2630 merged or just revert this one? It may not be that relevant but would be good for a peace of mind.

I commented on this PR. Let's continue the discussion in there.

I will test future PRs more thoroughly, this week was pretty exhausting though.

No biggie. We all do this in our free time and sometimes things just get rough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants