Don't move a file under a directory when renaming

[jelly]

We rename in Files by moving the file to the new filename but when this is a directory mv moves the file to that directory instead. This is quite logical in a shell but not in a GUI, there it is unexpected behaviour.

Closes: #399

[jelly]

Hmm this should be covered. Let's try to cover that in a follow up but let's think about it:

This is about renaming the current dir, which we no longer support, so this is dead code.

[allisonkarlitskaya]

Looks good, thanks!

I guess we get something like "targetname: Is a directory"?

Tests would be nice ;)

[jelly]

Looks good, thanks!

I guess we get something like "targetname: Is a directory"?

We get this: https://github.com/cockpit-project/cockpit-files/pull/404/files#diff-e86755442bc7d21f0414014b3c356b54d6650e8ed9bffcc4f5858d1154df80e1R556

Tests would be nice ;)

I've added a test, see the test/check-application changes. One missing test is rename a file to an existing file. We should imo test that.

[jelly]

Added a test for this scenario which shows the wrong behavior...

> debug: process spawn: ["mv","-T","/home/admin/newfile","/home/admin/existing"]
> debug: send control: {"payload":"stream","spawn":["mv","-T","/home/admin/newfile","/home/admin/existing"],"superuser":"try","err":"message","command":"open","channel":"1:1!17","host":"localhost","flow-control":true}

This just moves newfile into an existing file without warning. Let's fix it in this PR as I already have a test.

[jelly]

Turns, out we likely want just --no-clobber, let's also not write short arguments but write them out so the code is readable.

[jelly]

This is flaky or coreutils changed behaviour. Re-trying once.

[jelly]

Yup, this is broken. Needs work.

[jelly]

[root@archlinux admin]# mv -T  newfile dest && echo 1
mv: cannot overwrite directory 'dest' with non-directory 'newfile'
[root@archlinux admin]# mv -T -n  newfile dest && echo 1
1

Yeah this is great :) It is likely a bug in coreutils as it happens in rawhide as well. Fedora 40 has 9.4 while Arch has 9.5

This is indeed the case see:

https://github.com/coreutils/coreutils/blob/master/NEWS#L123
https://github.com/coreutils/coreutils/blob/master/NEWS#L95

As alternative @allisonkarlitskaya suggested passing -i instead, hoping that this will work with cockpit.spawn and that we can pass y or n via .input() on stderr. This sounds like some nice async work!

[allisonkarlitskaya]

As alternative @allisonkarlitskaya suggested passing -i instead, hoping that this will work with cockpit.spawn and that we can pass y or n via .input() on stderr. This sounds like some nice async work!

If you don't want to do the async work, there's a somewhat easier version:

$ cpf spawn -- mv -i -T a b stderr=message : done ch1 : wait | spy -m cockpit.bridge
...
{
  "exit-status": 1,
  "message": "mv: overwrite 'b'? ",
  "command": "close",
  "channel": "ch1"
}

with the successful case looking like

$ cpf spawn -- mv -i -T a c stderr=message : done ch1 : wait | spy -m cockpit.bridge
...
{
  "exit-status": 0,
  "message": "",
  "command": "close",
  "channel": "ch1"
}

The key here is done. If you don't send it, the channel will hang waiting for the answer. If you do send it, then mv will even give you a nice non-zero exit code.

If you do want to do the async work, that's err=out.

[jelly]

Le JS code:

        cockpit.spawn([
            "mv", "--interactive", "--no-target-directory",
            path.join("/") + "/" + selected.name, newPath
        ],
                      { superuser: "try" }).input()
                .then(() => Dialogs.close())
                .catch(err => {
                    setNameError(err.message);
                });

[jelly]

This needs to be expedited as renaming a folder into a new folder still works which is wrong.

[jelly]

Superseeded by #459