bpo-34260: fix docstring of shutil.copy2 #8523

csernazs · 2018-07-28T12:56:04Z

Fix the docstring of copy2, as it does not copy file ownership (user and
group), just only mode bits, mtime, atime and flags.

The original text was confusing to developers as it suggested that this
command is the same as 'cp -p', but according to cp(1), '-p' copies file
ownership as well.

https://bugs.python.org/issue34260

the-knights-who-say-ni · 2018-07-28T12:56:06Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

You can check yourself
to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

BoboTiG

Thanks! This seems OK for me.

csernazs · 2018-07-29T20:34:09Z

I've signed CLA, could you remove the CLA not signed label?
https://bugs.python.org/user?@template=clacheck&github_names=csernazs

vstinner

I dislike when docstring and doc in Doc/library/shutil.rst are different:
https://docs.python.org/dev/library/shutil.html#shutil.copy2

"Identical to copy() except that copy2() also attempts to preserve all file metadata."

This documentation is wrong: the owner is not copied.

vstinner · 2018-07-31T10:03:49Z

Lib/shutil.py

@@ -384,8 +384,8 @@ def copy(src, dst, *, follow_symlinks=True):
    return dst

 def copy2(src, dst, *, follow_symlinks=True):
-    """Copy data and all stat info ("cp -p src dst"). Return the file's
-    destination.
+    """Copy data and stat info (mode bits, mtime, atime, flags). Return the file's


I'm confused by "mode bits", I would prefer to only write "mode". I don't know what are "flags". I'm unable to find "st_flags" in os.stat_result.

I suggest to either give the full list of all copied attributes, or document which ones are not copied. But this documentation seems to be half baked.

I've changed the rst. I think it is written well mostly, so only the text 'all' has been replaced to 'more', which is correct now I believe.

'mode bits' has been replaced everywhere to 'mode' as you suggested. Also, I decided to write the concrete list of what metadata is copied by copystat, and copy2() refers to it to avoid unnecessary duplicating of the docstring.

st_flags is actually returned by stat() on some systems (*bsd and osx I believe), so I kept it there (I think it is not in stat_info's repr()), with the notation that flags is only copied when os.chflags() is available and st_flags is returned by stat(). More info about chflags: https://www.freebsd.org/cgi/man.cgi?query=chflags

bedevere-bot · 2018-07-31T10:03:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

csernazs · 2018-07-31T19:35:04Z

I have made the requested changes; please review again.

Please see my comments in a reply to your comment above.

bedevere-bot · 2018-07-31T19:35:06Z

Thanks for making the requested changes!

@vstinner, @pablogsal: please review the changes made to this pull request.

vstinner · 2018-07-31T22:25:14Z

Misc/NEWS.d/next/Library/2018-07-28-14-05-43.bpo-34260.SR5XMW.rst

@@ -0,0 +1,2 @@
+Fix docstring of ``shutil.copy2()`` to make it more specific about which


We don't document changes on the documentation. I would suggest to remove the NEWS entry.

vstinner · 2018-07-31T22:25:57Z

Lib/shutil.py

@@ -384,8 +394,10 @@ def copy(src, dst, *, follow_symlinks=True):
    return dst

 def copy2(src, dst, *, follow_symlinks=True):
-    """Copy data and all stat info ("cp -p src dst"). Return the file's
-    destination.
+    """Copy data and some metadata. Return the file's destination.


I suggest to remove "some", the next sentence is enough.

vstinner · 2018-07-31T22:26:21Z

Lib/shutil.py

@@ -366,7 +376,7 @@ def lookup(name):
    _copyxattr(src, dst, follow_symlinks=follow)

 def copy(src, dst, *, follow_symlinks=True):
-    """Copy data and mode bits ("cp src dst"). Return the file's destination.
+    """Copy data and mode ("cp src dst"). Return the file's destination.


I'm sorry, I didn't notice that the existing doc already used "mode bits". Please leave this doc unchanged ;-)

vstinner · 2018-07-31T22:27:50Z

Doc/library/shutil.rst

@@ -177,7 +177,7 @@ Directory and files operations
 .. function:: copy2(src, dst, *, follow_symlinks=True)

   Identical to :func:`~shutil.copy` except that :func:`copy2`
-   also attempts to preserve all file metadata.
+   also attempts to preserve more file metadata.


I suggest to remove "more".

Problem is that shutil.copy() still copies the mode, so it is not true that the difference is that "copy2 attempts to preserve file metadata", as copy also preserves metadata.
Anyway, I removed the 'more' word as you suggested.

csernazs · 2018-08-01T06:14:18Z

I have made the requested changes; please review again.

Could you please also add the "skip news" label to this?
Also, the devguide would need to be updated at some point in the future as it says nothing about "no news file is needed for documentation-only changes".

bedevere-bot · 2018-08-01T06:14:20Z

Thanks for making the requested changes!

@pablogsal, @vstinner: please review the changes made to this pull request.

vstinner · 2018-08-01T16:15:26Z

@giampaolo, @gpshead: would you mind to review this PR?

vstinner · 2018-10-22T14:12:10Z

I close/reopen the issue to re-trigger CI jobs. GitHub is sick today, https://status.github.com/messages

bedevere-bot · 2018-10-22T18:37:29Z

Thanks for making the requested changes!

@gpshead, @vstinner, @pablogsal: please review the changes made to this pull request.

vstinner · 2018-10-22T20:57:55Z

"AppVeyor build failed (...) Console: Log is empty." thank you AppVeyor...

vstinner · 2018-10-22T20:58:32Z

I rescheduled the AppVeyor job. If AppVeyor doesn't pass, I cannot merge the PR.

miss-islington · 2018-10-23T10:09:53Z

Thanks @csernazs for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2018-10-23T10:09:57Z

Sorry, @csernazs and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4f399be0e70d8b5516b6213568b7665765bb3114 3.7

vstinner · 2018-10-23T10:11:01Z

@csernazs: Oh, the backport failed. Can you please try to backport the change using " cherry_picker 4f399be 3.7" (see previous comment) or using "git cherry-pick -x 4f399be"?

IMHO this change also deserves to be backported to 2.7 and 3.6 as well.

…GH-8523) Fix the documentation of copy2, as it does not copy file ownership (user and group), only mode, mtime, atime and flags. The original text was confusing to developers as it suggested that this command is the same as 'cp -p', but according to cp(1), '-p' copies file ownership as well. Clarify which metadata is copied by shutil.copystat in its docstring.. (cherry picked from commit 4f399be) Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>

bedevere-bot · 2018-10-23T21:57:05Z

GH-10065 is a backport of this pull request to the 3.7 branch.

… (GH-10065) Fix the documentation of copy2, as it does not copy file ownership (user and group), only mode, mtime, atime and flags. The original text was confusing to developers as it suggested that this command is the same as 'cp -p', but according to cp(1), '-p' copies file ownership as well. Clarify which metadata is copied by shutil.copystat in its docstring. (cherry picked from commit 4f399be)

… (GH-10068) Fix the documentation of copy2, as it does not copy file ownership (user and group), only mode, mtime, atime and flags. The original text was confusing to developers as it suggested that this command is the same as 'cp -p', but according to cp(1), '-p' copies file ownership as well. Clarify which metadata is copied by shutil.copystat in its docstring. (cherry picked from commit 4f399be)

…GH-8523) Fix the documentation of copy2, as it does not copy file ownership (user and group), only mode, mtime, atime and flags. The original text was confusing to developers as it suggested that this command is the same as 'cp -p', but according to cp(1), '-p' copies file ownership as well. Clarify which metadata is copied by shutil.copystat in its docstring.. (cherry picked from commit 4f399be) Co-authored-by: Zsolt Cserna <cserna.zsolt@gmail.com>

… (GH-10071) Fix the documentation of copy2, as it does not copy file ownership (user and group), only mode, mtime, atime and flags. The original text was confusing to developers as it suggested that this command is the same as 'cp -p', but according to cp(1), '-p' copies file ownership as well. Clarify which metadata is copied by shutil.copystat in its docstring. (cherry picked from commit 4f399be)

the-knights-who-say-ni added the CLA not signed label Jul 28, 2018

bedevere-bot added the awaiting review label Jul 28, 2018

csernazs force-pushed the fix-issue-34260 branch from c4fe127 to 4d1c346 Compare July 28, 2018 13:07

pablogsal added the docs Documentation in the Doc dir label Jul 28, 2018

BoboTiG reviewed Jul 28, 2018

View reviewed changes

BoboTiG approved these changes Jul 28, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 28, 2018

pablogsal approved these changes Jul 28, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 28, 2018

pablogsal requested a review from giampaolo July 28, 2018 16:48

Mariatta removed the CLA not signed label Jul 30, 2018

the-knights-who-say-ni added the CLA signed label Jul 30, 2018

vstinner previously requested changes Jul 31, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jul 31, 2018

csernazs force-pushed the fix-issue-34260 branch from 4d1c346 to a89bc46 Compare July 31, 2018 19:24

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 31, 2018

vstinner reviewed Jul 31, 2018

View reviewed changes

csernazs force-pushed the fix-issue-34260 branch from a89bc46 to 691cd9a Compare August 1, 2018 06:11

vstinner approved these changes Oct 22, 2018

View reviewed changes

vstinner closed this Oct 22, 2018

vstinner reopened this Oct 22, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 22, 2018

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 22, 2018

vstinner merged commit 4f399be into python:master Oct 23, 2018

bedevere-bot removed the awaiting merge label Oct 23, 2018

miss-islington assigned vstinner Oct 23, 2018

csernazs mentioned this pull request Oct 23, 2018

[3.7] bpo-34260, shutil: fix copy2 and copystat documentation (GH-8523) #10065

Merged

bedevere-bot removed the needs backport to 3.7 label Oct 23, 2018

		@@ -0,0 +1,2 @@
		Fix docstring of ``shutil.copy2()`` to make it more specific about which

Uh oh!

bpo-34260: fix docstring of shutil.copy2 #8523

bpo-34260: fix docstring of shutil.copy2 #8523

Uh oh!

Conversation

csernazs commented Jul 28, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jul 28, 2018

Uh oh!

BoboTiG left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csernazs commented Jul 29, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

csernazs Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 31, 2018

Uh oh!

csernazs commented Jul 31, 2018

Uh oh!

bedevere-bot commented Jul 31, 2018

Uh oh!

vstinner Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

vstinner Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

csernazs Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

csernazs commented Aug 1, 2018

Uh oh!

bedevere-bot commented Aug 1, 2018

Uh oh!

vstinner commented Aug 1, 2018

Uh oh!

vstinner commented Oct 22, 2018

Uh oh!

bedevere-bot commented Oct 22, 2018

Uh oh!

vstinner commented Oct 22, 2018

Uh oh!

vstinner commented Oct 22, 2018

Uh oh!

miss-islington commented Oct 23, 2018

Uh oh!

miss-islington commented Oct 23, 2018

Uh oh!

vstinner commented Oct 23, 2018

Uh oh!

bedevere-bot commented Oct 23, 2018

Uh oh!

Uh oh!

csernazs commented Jul 28, 2018 •

edited by bedevere-bot

Loading

BoboTiG left a comment •

edited

Loading

csernazs Jul 31, 2018 •

edited

Loading