Bug-871: mv builtins cannot move on cross-mountpoint, cp obscure error

[phidebian]

when -r is not given on src dir.

• src/lib/libcmd/cp.c If src is a dir and -r was not given, warn with a message like -r not specified; omitting directory

  This fix bogus error message like cp: warning: t: directory -- copying as plain file cp: t: /tmp/t read error [Is a directory]

  This is not the exact purpose of the bug-871 submission, but it is in the same vein.

• src/lib/libcmd/mv.c The builtin mv is unable to deal with cross-mounted file system. The code still has a -x option for 'may be' cross-mounted file system but it doesn't works.

  The effect of $ /opt/ast/bin/mv bin /dev/shm mv: bin: /dev/shm/bin read error [Is a directory]

  Is a failure on xdev mv, and a left over regular file (not dir) named. '/tmp/bin'

  Then $ /opt/ast/bin/cp -r bin /dev/shm cp: /dev/shm/bin: not a directory -- bin ignored Since /tmp/bin is a regular file we got the above error.

  The code is pretty obscure and wrong, to avoid upsetting this instable design, I propose a workaround instead of a fix.

  The idea is to detect xdev mounted target dst and do a cp -r src... dst If all goes well do rm -rf src...

NOTE : The bug-871 report the problem with /tmp or /dev/shm but it is not related to those FS, using another disc (FS) fail the same way.

The workaround.
in b_mv() we try to figure out if some src... dev don't match the dts dev, if so we craft an argc argv and call b_cp() with argv[0]="XM", i.e a signature not used by b_cp() which can be called with the signature "cp", "ln", "mv".

In b_cp() we detect the "XM" signature, and restore a valid "cp" signature, this signature is used by cmdinit() to set error_info.id="cp", i.e the basename of argv[0], as side effect, used for error reporting in "cp".
After the cmdinit() we keep a backup of the cmd because the behavior of the code depend on switch (error_info.id[0]), and since our XM "cp" is actually a run on "mv" behalf, we set error_info.id to "mv" that is important for accurate error reporting, and now the switc become switch(cmd[0]) to still work as a cp but with an mv error report.

Down the code, while looping on src... if the cmd[0]=='c' i.e cp mode, and src is a dir and -r was not given, we produce the more conventional error message
"-r not specified; omitting directory %s",src

[McDutchie]

At long last, I'm getting around to looking at this PR.

Clever concept, that workaround. You're literally translating a cross-device mv to a cp -r followed by a rm -rfd.

Two of the regression tests were failing on my system, and failure output was very messy. I've pushed a cleanup commit for them to this PR.

A few notes:

• The timestamps aren't preserved when moving a hierarchy across file systems. Should be easy to fix by changing cp -r to cp -a.
• rm -rfd is redundant as -r necessarily implies -d, so rm -rf should be fine.
• The workaround forces the rm builtin (rm.o) to be linked into the statically compiled ksh binary due to the b_rm call, though rm is not part of the current default selection in data/builtins.c. So if we end up using this workaround, we might as well add it there and expose it to the user.
• Bug: if any option is given, like mv -v somedir somewhere/else, then it will be treated as cross-device no matter whether it is or not. The probe code does not skip options, so it considers -v a directory that is not on the same device because it is not found.
• Bug: since argv[0] is changed to "cp", it will now parse cp options, not mv options, which may result in incorrect behaviour if one of the cp-specific options is given.
• The cross-device mv regression tests are unfortunately not viable, because it is completely system-dependent whether /tmp is on a different file system from the source directory. On my macOS system, it's on the same file system. Also, many build systems extract the source directory in /tmp in the first place.

Some of these issues are easy to fix, others perhaps not so much.

Many thanks for the PR, but this is going to need some more thought and testing before merging.

[McDutchie]

I found another way, but I couldn't have done it without your PR.

The check for cross-device arguments can be done in b_mv() itself, which allows the code to be more concise and efficient. It can simply switch from 'mv' to 'cp -a' mode by setting the appropriate flags.

Then, the only thing left to do is remove the source files after copying them. Using b_rm for this is actually a great idea -- it would be silly to do otherwise as it would be code duplication, and b_rm already contains all the appropriate error handling etc. But if we make a small special case for argc==-1 in b_rm that sets the appropriate flags for -rfu, we can call b_rm(-1,argv,context) to delete the files efficiently.

I'm pretty much completely rewriting your PR I'm afraid, so I'll make a commit of my own instead of merging the PR, because i wouldn't want you to have to take the git blame for my mistakes. :-) However, I'll definitely give you credit as a co-author because you came up with the general idea, without which I wouldn't have been able to do this.