classification
Title: Add shutil.chown to allow to use user and group name (and not only uid/gid)
Type: enhancement Stage: committed/rejected
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: sandro.tosi Nosy List: eric.araujo, eric.smith, ezio.melotti, giampaolo.rodola, gregory.p.smith, haypo, ncoghlan, neologix, petri.lehtinen, pitrou, python-dev, r.david.murray, rhettinger, sandro.tosi
Priority: low Keywords: patch

Created on 2011-05-26 20:14 by sandro.tosi, last changed 2011-09-02 16:15 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
shutil_chown-default.patch sandro.tosi, 2011-05-26 20:14 review
shutil_chown-default-v2.patch sandro.tosi, 2011-05-30 19:15 review
shutil_chown-default-v3.patch sandro.tosi, 2011-06-15 18:51 review
shutil_chown-default-v4.patch sandro.tosi, 2011-07-01 20:52 review
shutil_chown-default-v5.patch sandro.tosi, 2011-08-05 20:34 review
shutil_chown-default-v6.patch sandro.tosi, 2011-08-08 16:26 review
shutil_chown-default-v7.patch sandro.tosi, 2011-08-19 18:01 review
Messages (30)
msg137004 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-05-26 20:14
Hi, following up the discussion on http://mail.python.org/pipermail/python-dev/2011-May/111642.html here'a patch to introduce shutil.chown() along with doc, tests and a change in os doc to link to the new function (I also took the occasion to rewrap os.chown() doc text). As of now only the one-file function is implemented, let's see at a later time what's about for a recursive one.

The patch was prepared against default, but it probably makes sense to backport it to 2.7?

Any comment/suggestion is welcome!
msg137008 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-05-26 20:45
As a new feature, it can't be added to 2.7.
msg137011 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-26 22:20
I didn't test, but after skimming through the code I think that if an invalid user name/group name is provided, os.chown is passed None, which will fail with ValueError.
It would be better to raise an explicit error like "no such user" / "no such group".
That's something you could add to the tests.

Also, you could initialize _user to -1 instead of None, and replace
    if user is None:
        _user = -1
    else:
        [...]
with
    if user is not None:
        [...]

Same goes for _group.
msg137084 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-05-27 16:53
As a general rule, rewrapping is not done in patches, it can make review less easy.  The committer can do it, sometimes in a second commit.

I commented on the review page.
msg137174 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-05-29 09:13
I added comments in the code review.

this patch is looking good once the comments are addressed.  thanks for your contribution!

As for talk of support for recursion... thats what os.walk() is for.  it doesn't belong as part of any particular individual function itself.
msg137195 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-05-29 16:31
The python-dev discussion talked about chowntree vs. os.walk:
http://mail.python.org/pipermail/python-dev/2011-May/111667.html
http://mail.python.org/pipermail/python-dev/2011-May/111673.html
http://mail.python.org/pipermail/python-dev/2011-May/111674.html

I find those arguments of convenience and precedent convincing.
msg137320 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-05-30 19:15
@Eric: ok, I was just asking given the "unusual" situation 2.7 is (i.e. a very long support series), but it's perfectly fine not to backport the feature.

@Charles-François: I changed a bit the logic: I check for 'None' at first, since it signals that we don't want to change the uid/gid, and then I recheck for None after _get_uid/gid, and only there raise a ValueError (is there a better exception for cases like this?) - what do you think about it?

@Éric: re rewrap: I kinda find that almost every core dev has his opinion on this :) some don't want to rewrap, others are fine if you're touching that paragraph, and so on. Anyhow, to be on the safe-side, I un-rewrapped the first paragraph of os.chown().

@Gregory: I'd like to first concentrate on the "single path" function, then we'll look at the "tree" version.

Thanks a lot to everyone for the review/comment/inspiration: attached you can find the updated patch. If something still needs a fix, I'd be happy to work on it.
msg137452 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-01 18:04
> I was just asking given the "unusual" situation 2.7 is (i.e. a very
> long support series)
Support means bug fixes.  Long-term means that the bugfix period (before going into security mode) is extended, not that it can get new features: that’s exclusively for 3.3.

> rewrap: I kinda find that almost every core dev has his opinion on this :)
Heh :)
msg138388 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-06-15 18:51
Here's the updated patch following review on Rietveld
msg138671 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-19 22:57
shutil_chown-default-v3.patch:
 - os.chown() is only available on Unix, shutil.chown() should not be defined if os.chown() doesn't exist (require to add a @unittest.skipUnless decorator to the test)
 - tests: is it possible that shutil.chown() exists whereas pwd or grp module is missing? if yes, you should split the test into two parts. it looks like pwd and grp are avaiable on Unix.
 - os.chown(path, -1, -1) is accepted, why not accepting shutil.chown(path) (shutil.chown(path, None, None))?
 - style: i don't like _user name, i suggest uid and gid (or user_id and group_id) instead of _user and _group... or you can reuse user/group variables
 - style: "else: \n if not isinstance(user, int):" can be written "elif not isinstance(user, int):" to avoid an useless level of indentation
 - tests: you may only call os.getuid() and os.getgid() once
msg138904 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-24 10:51
haypo, you missed this comment from gps on Rietveld:

> As for the _user = user stuff, the reason that is being done is so
> that the original user and or group value can be included in the
> raised exception error message.  That is actually a nice thing to do.
> 
> otherwise, yes, i'd normally just reassign the parameters.
>
> At this point I believe this patch is ready to commit.  I'd reword
> the docstring to make it simpler but that is a minor detail and
> something that is easy to change in a future commit.

I agree the patch is ready.
msg139603 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-07-01 20:52
I've applied some of the suggestions Victor made (and also refresh to be cleanly applicable on default).
msg140491 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-07-16 10:42
Sorry for the late reply: I've applied Éric comments made on Rietveld.
msg141665 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-05 16:25
You should replace the v5 file (or even remove all files, for clarity) with the actual output of hg diff, not hg status ;-)
msg141686 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-05 20:36
Gaaah, sorry about that: I've just uploaded the correct fifth version.
msg141699 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-06 00:42
I wonder if we should raise LookupError for unknown uids/gids. Do we have other APIs with similar semantics?
msg141743 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-07 13:43
grepping over the stdlib, LookupError is only used in codecs exception; also the documentation of LE is "The base class for the exceptions that are raised when a key or index used on a mapping or sequence is invalid: IndexError, KeyError. This can be raised directly by codecs.lookup()." so I guess it's not the right exception of this case.
msg141749 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-08-07 22:12
Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly".  Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.
msg141753 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-07 22:26
On Mon, Aug 8, 2011 at 00:12, R. David Murray <report@bugs.python.org> wrote:
>
> R. David Murray <rdmurray@bitdance.com> added the comment:
>
> Well, you could consider codecs an example in the stdlib of using it, and the doc could be changed to something like "stdlib modules that look up information in specialized data sources may raise this error directly".  Whether this is a good idea or not, I'm not sure, but it feels more logical than ValueError.

Sure, I was just showing my line of reasoning :) I'm ok either way: if
you feel LookupError is best, I'll change shutil.chown() code and also
the doc for LE (about that, is it ok a single patch or 2 separate?)
msg141784 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-08 14:46
LookupError sounds good.  About the latest patch: I wonder if using !a instead of !r in the format strings for exceptions would be more helpful (maybe you’ve seen a recent python-dev subthread about that).  I don’t like seeing escapes for perfectly common characters like ß or é, but OTOH escapes help disambiguating different characters that look the same.  Apart from these two points, this is good to go.

A doc addition for LookupError would be an independent changeset; it has nothing to do with adding shutil.chown.
msg141792 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-08 16:26
after a review from Ezio (thanks!) we've come out with this updated patch; main changes are in the test suite, where now it's checked that chown() succeed. about !r/!a I've left !r; and changed the 2 ValueError in LookupError (the first, in case no arguments are passed, it's still there).
msg142480 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-19 18:01
version 7 here we come :) I've addressed Eric's commenta on rietveld (all but one, I need input from him) and also updated the tests for new helper functions.
msg142751 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-22 21:29
New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default':
#12191: add shutil.chown() to change user and/or group owner of a given path also specifying their names.
http://hg.python.org/cpython/rev/d1fd0f0f8e68
msg142752 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2011-08-22 21:30
At last, it's in :) thanks a lot to all the people that helped me in the process!
msg142753 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-08-22 21:33
> New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default'

You may add shutil.chmod to the "What's New in Python 3.3?" document.
msg142754 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-08-22 21:38
Raymond, are you still taking care of the whatsnew?
Do you want people to update it when they add something new?
msg142757 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-22 21:56
New changeset 5d317e38da44 by Sandro Tosi in branch 'default':
#12191: fix build failures, explicitly passing group argument when I want to test it
http://hg.python.org/cpython/rev/5d317e38da44
msg142764 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-22 22:18
Ezio: I think that Raymond is in charge of the 3.3 whatsnew too.  The rules in a comment at the top of the file still apply: there are already around 30 notes to the file.
msg142770 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-22 22:59
New changeset f2cb733c9a37 by Sandro Tosi in branch 'default':
#12191: added entry in What's New (+ small editing on shutil section)
http://hg.python.org/cpython/rev/f2cb733c9a37
msg143411 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-02 16:15
I have added 'chown' in shutil.__all__.
History
Date User Action Args
2011-09-02 16:15:08eric.araujosetmessages: + msg143411
2011-08-22 22:59:04python-devsetmessages: + msg142770
2011-08-22 22:18:42eric.araujosetmessages: + msg142764
2011-08-22 21:56:11python-devsetmessages: + msg142757
2011-08-22 21:38:51ezio.melottisetnosy: + rhettinger
messages: + msg142754
2011-08-22 21:33:45hayposetmessages: + msg142753
2011-08-22 21:30:00sandro.tosisetstatus: open -> closed
resolution: fixed
messages: + msg142752

stage: patch review -> committed/rejected
2011-08-22 21:29:03python-devsetnosy: + python-dev
messages: + msg142751
2011-08-19 18:01:28sandro.tosisetfiles: + shutil_chown-default-v7.patch

messages: + msg142480
2011-08-08 16:26:43sandro.tosisetfiles: + shutil_chown-default-v6.patch

messages: + msg141792
2011-08-08 14:46:35eric.araujosetmessages: + msg141784
2011-08-07 22:26:29sandro.tosisetmessages: + msg141753
2011-08-07 22:12:34r.david.murraysetnosy: + r.david.murray
messages: + msg141749
2011-08-07 13:43:15sandro.tosisetmessages: + msg141743
2011-08-06 00:42:59pitrousetnosy: + pitrou
messages: + msg141699
2011-08-05 20:36:28sandro.tosisetnosy: + ezio.melotti
messages: + msg141686
2011-08-05 20:35:10sandro.tosisetfiles: - shutil_chown-default-v5.patch
2011-08-05 20:34:47sandro.tosisetfiles: + shutil_chown-default-v5.patch
2011-08-05 16:25:02eric.araujosetmessages: + msg141665
2011-07-16 10:42:10sandro.tosisetfiles: + shutil_chown-default-v5.patch

messages: + msg140491
2011-07-01 20:52:17sandro.tosisetfiles: + shutil_chown-default-v4.patch

messages: + msg139603
2011-06-24 10:51:37eric.araujosetmessages: + msg138904
2011-06-19 22:57:15hayposetnosy: + haypo
messages: + msg138671
2011-06-15 18:51:38sandro.tosisetfiles: + shutil_chown-default-v3.patch

messages: + msg138388
2011-06-01 18:04:59eric.araujosetmessages: + msg137452
2011-05-30 19:15:16sandro.tosisetfiles: + shutil_chown-default-v2.patch

messages: + msg137320
2011-05-29 16:31:20eric.araujosetmessages: + msg137195
2011-05-29 09:13:30gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg137174
2011-05-27 16:53:31eric.araujosetnosy: + eric.araujo

messages: + msg137084
title: Shutil - add chown() in order to allow to use user and group name (and not only uid/gid) -> Add shutil.chown to allow to use user and group name (and not only uid/gid)
2011-05-27 11:44:36giampaolo.rodolasetnosy: + giampaolo.rodola
2011-05-27 05:42:27petri.lehtinensetnosy: + petri.lehtinen
2011-05-27 05:27:17ncoghlansetnosy: + ncoghlan
2011-05-26 22:20:05neologixsetnosy: + neologix
messages: + msg137011
2011-05-26 20:45:21eric.smithsetnosy: + eric.smith
messages: + msg137008
2011-05-26 20:14:19sandro.tosisetversions: + Python 3.3
2011-05-26 20:14:09sandro.tosicreate