Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: Rename needs more doc clarity #6887

Closed
gopherbot opened this issue Dec 3, 2013 · 18 comments
Closed

os: Rename needs more doc clarity #6887

gopherbot opened this issue Dec 3, 2013 · 18 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2013

by xunfin:

documentation and maybe implementation issue 

text (ie http://golang.org/pkg/os/#Rename ) is too vague

eg naively expect os.Rename("C:/a/b/c/oldname.go","newname.go") to
create/rename the file at "C:a/b/c/newname.go"

in WinXP the file ends up in working directory eg with go file in C:/x/y/z/rename.go ,
'go run rename.go' places files renamed using commands of the type
'os.Rename("C:/d/e/.../i/j/oldname.go","newname.go")' into
C:/x/y/z/newname.go

Specifically Windows users may be caught out by this - commandline windows (eg cmd.exe)
rename function cannot change either drive or path eg usage given in WinXP cmd.exe via
"rename help"

::(quote) RENAME [drive:][path]filename1 filename2. Note that you cannot specify a new
drive or path for your destination file.

See also  issue #6401 - its not clear if os.Rename is supposed to function as an
"os.Move" or not

I'm assuming the current behaviour of os.Rename is due to the parameters not being
checked for suitability.

In the short term can the docs be modified to say something like: "Both strings
'oldname' and 'newname' must be supplied as full file paths to avoid unintended
behavour"

Which operating system are you using?
:XP SP3
@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Dec 4, 2013

Comment 1:

Labels changed: added os-windows, documentation, removed priority-triage.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 2:

xunfin,
Rename uses MoveFile Windows API under the covers. So whatever Windows MoveFile does is
what you get. That seems most reasonable and less surprising to me.
I don't see what else Rename documentation can say, given that we don't actually
implement that function ourselves, but use OS for the service.
Please, make a new doc suggestion for a consideration. Thank you.
Alex

Status changed to WaitingForReply.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 3:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 4, 2013

Comment 5 by xunfin:

It seems that this and issue #6401 could be case checked using something like : 
import (
    "errors"
    "os"
    "path/filepath"
)
func Rename2(oldname, newname string) error {
    olddir, _ := filepath.Split(oldname)
    newdir, newfile := filepath.Split(newname)
    // 1. arguments supplied in form ("oldname_fullpath","newname_filename"")
    if newdir == "" {
        return os.Rename(oldname, filepath.Join(filepath.Dir(olddir), newfile))
    }
    // 2. otherwise assume both are full paths
    if olddir == newdir {
        return os.Rename(oldname, newname)
    } else {
        return &os.PathError{"rename", newname, errors.New("path for new name must match path given for old name")}
    }
}
I don't think it is go practice to second guess the user's intentions so maybe the
double-use of the second parameter may not be acceptable  - depending on what is the
intended form of the path string supplied to "newname" either the parts marked 1. or 2.
in the comments could be deleted..
(pkg filepath is used above because the equivalent pkg path function fail to work
properly using windows type paths..)
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 6:

Why are we talking about issue #6401 here? I even more confused now. Can we start again,
please? Can you describe your problem again? Thank you.
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 4, 2013

Comment 7 by xunfin:

Alex (added more before seeing your reply)
re:MovefileAPI
Not sure - I don't know what the behaviour is on ther OSes -  its not clear that the
side effects of using MoveFileAPI on Windows are actually intended - can someone
clarify? Is 'rename' supposed to be synonymous with 'move' - I would have expected a
separate function 'move' - to fix that would require more work and a gofix issue..
As it stands the suggestion made above for should do for the docs,  ie add to the bottom
"Strings 'oldname' and 'newname' must be supplied as full file paths"
I'm assuming that the behaviour in windows when 'newname' us supplied with a just a new
filename is not intended and would not be consistent across other OSs - so not to
attempt to document it.
However I would prefer to error check. See above for more details.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 4, 2013

Comment 8 by xunfin:

Alex - issue #6401 is about a problem on Plan 9 - this (my) post is about an issue on
Windows - the two are similar issues 
There are two problems
1.The function acts as a move - but is named "rename" - rename on windows machines would
not be expected to work like this at all
2.The documentation does not explain what form of path for "newname" to use - I've given
a line of text to add to the documentation to fix that.
I don't actually know if the function is supposed to act like a move or not..
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 4, 2013

Comment 9 by xunfin:

ok. I see how the above could be confusing - summary
Simple fix is to update the docs to explain that 
os.Rename("C:\a\b\oldname.go", "newname.go")
is not the correct way to get the file at "C:\a\b\newname.go" , but 
os.Rename("C:\a\b\oldname.go", "C:\a\b\newname.go")
is correct.
Some grammatical variant on
:"Strings 'oldname' and 'newname' should be supplied as full file paths"
should be enough
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Dec 4, 2013

Comment 10:

re #5, we can't change the behavior of the API due to the Go 1 API guarantee.
the Rename API is modeled after the same named API on Unix which moves files.
It has nothing to do with the REN(AME) command in DOS.
Viewed that way, the API is clear, you give two path, and it will try to rename the
first to the 2nd. And if you give a relative path, the correct thing will happen, i.e.
the file path is treated relative to the current working directory.
To clear things up, I propose we change both names of the argument to contain
the word "path", and also mention OS-specific restrictions, like this:
// Rename renames a file. OS-specific restrictions might apply.
func Rename(oldpath, newpath string) error
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 4, 2013

Comment 11 by xunfin:

re: 10 ok thanks - that makes sense..
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 12:

minux, so what is your final proposal for the doc? Are we going to mention absolute /
relative paths?
Alex
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Dec 4, 2013

Comment 13:

my final proposal is:
// Rename renames (moves) a file. OS-specific restrictions might apply.
func Rename(oldpath, newpath string) error
three separate changes:
1. mention "move" to clarify things up.
2. use {old,new}path instead of {old,new}name, which makes it clear what relative path
would do here.
3. mention "OS-specific restrictions might apply". I'm open to suggestions on this one,
because
  I don't think we can list all the OS-specific restrictions without refering to the implementations.
  And people using it should be aware that we can't make a version that works the same across all
  supported platforms (such an API is not usable IMO, and it will also changing the current behavior).
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 14:

SGTM. You want to sent a CL?
xunfin, do you agree too?
Alex
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Dec 4, 2013

Comment 15:

Sure, https://golang.org/cl/36930044/

Owner changed to @minux.

Status changed to Started.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 5, 2013

Comment 16 by xunfin:

#14 Yes that helps a lot. Agree with changes 1 & 2
Not sure what to do about 3
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 5, 2013

Comment 17:

It has been decided already https://golang.org/cl/36930044/.
Alex
@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Dec 10, 2013

Comment 18:

This issue was closed by revision aa0ae75.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.