jarun / nnn

n³ The unorthodox terminal file manager
BSD 2-Clause "Simplified" License
19.1k stars 760 forks source link

Can't batch rename files which names start with dash '-' #1818

Closed Oxore closed 7 months ago

Oxore commented 7 months ago

Environment details (Put x in the checkbox along with the information)

If the issue can be reproduced on master, log it.

Yeah, okay, let's go formal, here is the log:

Click to expand ``` ln 8729: VERSION=4.9 ln 8513: g_tmpfpath=/tmp ln 8514: tmpfplen=5 ln 8762: home=/home/oxore ln 8461: cfgpath=/home/oxore/.config ln 8467: cfgpath=/home/oxore/.config/nnn ln 8495: selpath=/home/oxore/.config/nnn/.selection ln 8770: opener=xdg-open ln 8880: getenv(envs[ENV_VISUAL])=nvim ln 8881: getenv(envs[ENV_EDITOR])=nvim ln 8882: editor=nvim ln 8886: pager=/usr/bin/less ln 8890: shell=/bin/zsh ln 8892: getenv("PWD")=/tmp/a ln 2248: COLORS=256 ln 2249: COLOR_PAIRS=65536 ln 5744: __func__=dentfill ln 6011: ts2.tv_nsec - ts1.tv_nsec=19356 ln 6615: __func__=redraw ln 6630: path=/tmp/a ln 2411: status=0 ln 2485: pid=14522 ln 1631: pbuf=-file ln 1631: pbuf=-file ln 2411: status=0 ln 2485: pid=14523 ln 2776: count=1 ln 2777: lines=1 ln 2411: status=123 ln 2485: pid=14591 ln 5744: __func__=dentfill ln 6011: ts2.tv_nsec - ts1.tv_nsec=19847 ln 6615: __func__=redraw ln 6630: path=/tmp/a ```

Exact steps to reproduce the issue

Let's say I have a file with the name starting with a dash. I've created one with the following command:

touch -- -file

Then I fire up nnn, hit r to invoke batch rename, which opens up my lovely nvim, remove the dash from the file's name, hit save and exit aaand... nothing changed: I see the file is still there named as "-file". If I quit NNN, then I am faced with the following message in the terminal:

mv: invalid option -- 'l'
Try 'mv --help' for more information.

It looks to me that the double dash -- options separator may be useful in the mv invocation of batch rename function. If I add it in the source code like this:

diff --git a/src/nnn.c b/src/nnn.c
index 6792d503..111870b4 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -2730,3 +2730,3 @@ static bool batch_rename(void)
        static const char batchrenamecmd[] = "paste -d'\n' %s %s | "SED" 'N; /^\\(.*\\)\\n\\1$/!p;d' | "
-                                            "tr '\n' '\\0' | xargs -0 -n2 sh -c 'mv -i \"$0\" \"$@\" <"
+                                            "tr '\n' '\\0' | xargs -0 -n2 sh -c 'mv -i -- \"$0\" \"$@\" <"
                                             " /dev/tty'";

then it just werks, no problem, any file starting with a dash gets renamed correctly. But this is Linux, I mean, this is mv from coreutils package, and NNN supports a plethora of other stuff like OSX, *BSD, Haiku and others. Do they all have double dash options separator for the mv command? I don't know. It probably needs some testing.

KlzXS commented 7 months ago

Do they all have double dash options separator for the mv command?

-- as a marker for the end of options is a POSIX guideline, so it should be widely supported. Not to mention that it's been standard practice for many years. Of course, individual programs may not respect it, but mv most certainly will.

The same would go for cp and rm as well. And all the uses inside plugins. @jarun how vigilant do you think we should be with this? At least core nnn I'd say, but what about plugins?

N-R-K commented 7 months ago

how vigilant do you think we should be with this?

The fix looks simple. So there's no point in not doing the correct thing, IMO.

jarun commented 7 months ago

Yes, please raise the PR. Also check for other places in the code where it may be an issue.

We should fix this in plugins too.

Oxore commented 7 months ago

I found that a similar functionality exists in the cpmv_rename function. It uses similar shell pipeline defined in patterns[P_CPMVRNM] that does not use the double dash separator (--) too, but it mvs and cpies files without a problem even if the name starts with a dash. This and the fact, that I don't fully understand neither of these two shell pipelines, takes away the last bit of any confidence I had to create a PR with the simple the double dash separator fix.

KlzXS commented 7 months ago

cpmv_rename() and batch_rename() (when dealing with selection) use absolute paths. You don't get this problem with absolute paths as the first character is by definition /.

So the fix is not strictly necessary in those cases, but it doesn't hurt them either. Just smack on a -- everywhere you see cp, mv or rm. Other shell programs might also benefit from the same treatment but leave those for now.

I don't fully understand neither of these two shell pipelines

Also, fair point. Those pipelines have zero documentation to their name other than "they do what they do". I should write something for that when I get some more time.

KlzXS commented 7 months ago

See #1820