Re: mv example fix

Previous thread: correct mxcsr+mxcsr_mask handling (revised) by Philip Guenther on Saturday, December 25, 2010 - 1:02 pm. (7 messages)

Next thread: MicroLinear 6692 PHY for tl(4) -- Olicom 2326 by Loganaden Velvindron on Sunday, December 26, 2010 - 1:40 pm. (2 messages)
From: Ted Unangst
Date: Saturday, December 25, 2010 - 10:39 pm

A 3 line example with 3 bugs.

1.  destination and destination_path should be the same.
2.  source_file doesn't have to be a file.
3.  mv will also unlink a directory, ala rm -d.

Index: mv.1
===================================================================
RCS file: /home/tedu/cvs/src/bin/mv/mv.1,v
retrieving revision 1.26
diff -u -r1.26 mv.1
--- mv.1	3 Sep 2010 09:53:20 -0000	1.26
+++ mv.1	26 Dec 2010 05:35:57 -0000
@@ -117,9 +117,9 @@
 to accomplish the move.
 The effect is equivalent to:
 .Bd -literal -offset indent
-$ rm -f destination_path && \e
-    cp -PRp source_file destination && \e
-    rm -rf source_file
+$ rm -df destination && \e
+    cp -PRp source destination && \e
+    rm -rf source
 .Ed
 .Sh EXIT STATUS
 .Ex -std mv

From: Ingo Schwarze
Date: Sunday, December 26, 2010 - 10:23 am

Hi Ted,


Sure, and both should be "destination_path", because that's the
term used consistently in the DESCRIPTION.

Unambiguous terminology is very important here because "target"
and "destination path" are not the same thing at all, but easily
confused.  At first, when starting to look into your bug report,

Right, and indeed, it is the same as the source operand.

By the way, the DESCRIPTION ought to make it plain as well

Indeed, the code does contain a call to rmdir(2).
However:

  ischwarze@isnote $ df . /tmp
  Filesystem  512-blocks      Used     Avail Capacity  Mounted on
  /dev/wd0g     41280412  31471652   7744740    80%    /home
  /dev/wd0d      2061100        20   1958028     0%    /tmp
  ischwarze@isnote $ touch source
  ischwarze@isnote $ mkdir -p /tmp/target/source
  ischwarze@isnote $ mv source /tmp/target
  mv: rename source to /tmp/target/source: Is a directory
  ischwarze@isnote $ ls -F /tmp/target
  source/

Thus, files do not overwrite directories.

  ischwarze@isnote $ rm -df source /tmp/target/source
  ischwarze@isnote $ mkdir source
  ischwarze@isnote $ touch /tmp/target/source
  ischwarze@isnote $ mv source /tmp/target            
  mv: rename source to /tmp/target/source: Not a directory
  ischwarze@isnote $ ls -F /tmp/target                
  source

Thus, directories do not overwrite files.

  ischwarze@isnote $ rm -df source /tmp/target/source 
  ischwarze@isnote $ mkdir source                     
  ischwarze@isnote $ mkdir -p /tmp/target/source      
  ischwarze@isnote $ touch /tmp/target/source/file
  ischwarze@isnote $ mv source /tmp/target            
  mv: can't remove /tmp/target/source: Directory not empty
  ischwarze@isnote $ ls -F /tmp/target/source         
  file

Thus, the only case of actual rmdir(2) is removing an empty
directory to replace it with a new directory.

Just adding the -d here to the rm statement provokes the
misunderstanding that you can move a file on top of an existing
directory.  Thus, when ...
From: Jason McIntyre
Date: Sunday, December 26, 2010 - 10:38 am

hmm...

	cp - copy files
	rm - remove directory entries
	mv - move files and directories		# with your changes

i'm ok with all approaches, but i fear we are being inconsistent on
three very important utilities about how we describe things. do we
accept that files and directories are the same? do we distinguish?

i'd say your comma would be better placed after "directory" and not

i'm not convinced. it feels like you are removing a valid warning,
and turning it into an example. the author obviously intended that
people watch out for this (or had enough experience of people falling
for it, that they saw fit to warn them). you were caught out because
you nearly missed it in CAVEATS - i think it even less likely that
someone would catch it in EXAMPLES.

so, my instinct is that you "fix" CAVEATS, not move it. i'm happy
to be persuaded otherwise though.

jmc

From: Ingo Schwarze
Date: Sunday, December 26, 2010 - 11:41 am

Hi Jason,


I see the point, and i think it is valid.
Looking at rm(1), right now, we _do_ consistently assume that files
are a special case of directories.  Changing that is probably a bad
idea, and even if we want to change that stance, it is a separate


Seems valid to.

I think moving the description how the utility works to the
DESCRIPTION does make sense, but leaving the example in CAVEATS
as well.

So here is an improved patch:

Yours,
  Ingo


Index: mv.1
===================================================================
RCS file: /cvs/src/bin/mv/mv.1,v
retrieving revision 1.26
diff -u -r1.26 mv.1
--- mv.1	3 Sep 2010 09:53:20 -0000	1.26
+++ mv.1	26 Dec 2010 18:39:33 -0000
@@ -61,18 +61,25 @@
 .Nm
 moves each file named by a
 .Ar source
-operand to a destination specified by the
+operand into the destination specified by the
 .Ar directory
 operand.
 It is an error if the
 .Ar directory
-operand does not exist.
+does not exist.
 The destination path for each
 .Ar source
 operand is the pathname produced by the concatenation of the
 .Ar directory
 operand, a slash, and the final pathname component of the named file.
 .Pp
+In both forms, a
+.Ar source
+operand is skipped with an error message
+when the respective destination path is a non-empy directory,
+or when the source is a non-directory file but the destination path
+is a directory, or vice versa.
+.Pp
 The options are as follows:
 .Bl -tag -width Ds
 .It Fl f
@@ -117,9 +124,9 @@
 to accomplish the move.
 The effect is equivalent to:
 .Bd -literal -offset indent
-$ rm -f destination_path && \e
-    cp -PRp source_file destination && \e
-    rm -rf source_file
+$ rm -df -- destination_path && \e
+    cp -PRp -- source destination_path && \e
+    rm -rf -- source
 .Ed
 .Sh EXIT STATUS
 .Ex -std mv
@@ -162,19 +169,27 @@
 command appeared in
 .At v1 .
 .Sh CAVEATS
-In the second synopsis form
-if the destination path exists,
-the
+In the second synopsis form, incompatible file ...
From: Jason McIntyre
Date: Sunday, December 26, 2010 - 1:25 pm

this seems correct, but it was anyway. what are you trying to change
here?

other than that i think this diff is fine. but a technical ok would be
useful too.


From: Ingo Schwarze
Date: Sunday, December 26, 2010 - 1:54 pm

Hi Jason,


Two very minor issues:

1) The existing text uses the same wording for two different concepts:
First synopsis: moving to a place (like "I go to the bus stop.")
Second synopsis: moving into a container (like "I jump into the pool.")
As the two are technically rather different, i'd like to reflect the
difference at least a bit in the wording.

2) We are talking about one (definite) destination directory, just like
for the first synopsis, were the text also talks about "the destination".
Thus, i don't like switching to the indefinite article, which might
suggest a difference where there is none.


I understand tedu@ already provided that one.

Yours,
  Ingo

From: Jason McIntyre
Date: Sunday, December 26, 2010 - 2:08 pm

i like the simplicity of moving X to Y, regardless of what they are.
pronouns (if that's the right word) are horribly inconsistent anyway, so
i don;t really buy that argument - do you think of moving files *into* a
directory? maybe, but some people move them *to* a directory. i see no
problem with that wording.


right, i see that now too. i'd guess the author wrote naturally that
after talking about "a source operand" that "a destination" would sound
nice (i'm sure it occurred subconsciously). i'd be ok with changing that
to "the destination path", ala first synopsis. that seems more

well, tedu posted the mail to tech, presumably looking for an ok.
another ok would be good. maybe someone dislikes the changes?

jmc

Previous thread: correct mxcsr+mxcsr_mask handling (revised) by Philip Guenther on Saturday, December 25, 2010 - 1:02 pm. (7 messages)

Next thread: MicroLinear 6692 PHY for tl(4) -- Olicom 2326 by Loganaden Velvindron on Sunday, December 26, 2010 - 1:40 pm. (2 messages)