Add shutil.chown to allow to use user and group name (and not only uid/gid)

[sandrotosi]

BPO	12191
Nosy	@rhettinger, @gpshead, @ncoghlan, @pitrou, @vstinner, @ericvsmith, @giampaolo, @ezio-melotti, @merwok, @bitdancer, @sandrotosi, @akheron
Files	shutil_chown-default.patch shutil_chown-default-v2.patch shutil_chown-default-v3.patch shutil_chown-default-v4.patch shutil_chown-default-v5.patch shutil_chown-default-v6.patch shutil_chown-default-v7.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/sandrotosi'
closed_at = <Date 2011-08-22.21:30:00.982>
created_at = <Date 2011-05-26.20:14:09.878>
labels = ['type-feature', 'library']
title = 'Add shutil.chown to allow to use user and group name (and not only uid/gid)'
updated_at = <Date 2011-09-02.16:15:08.069>
user = 'https://github.com/sandrotosi'

bugs.python.org fields:

activity = <Date 2011-09-02.16:15:08.069>
actor = 'eric.araujo'
assignee = 'sandro.tosi'
closed = True
closed_date = <Date 2011-08-22.21:30:00.982>
closer = 'sandro.tosi'
components = ['Library (Lib)']
creation = <Date 2011-05-26.20:14:09.878>
creator = 'sandro.tosi'
dependencies = []
files = ['22132', '22200', '22372', '22540', '22841', '22864', '22949']
hgrepos = []
issue_num = 12191
keywords = ['patch']
message_count = 30.0
messages = ['137004', '137008', '137011', '137084', '137174', '137195', '137320', '137452', '138388', '138671', '138904', '139603', '140491', '141665', '141686', '141699', '141743', '141749', '141753', '141784', '141792', '142480', '142751', '142752', '142753', '142754', '142757', '142764', '142770', '143411']
nosy_count = 14.0
nosy_names = ['rhettinger', 'gregory.p.smith', 'ncoghlan', 'pitrou', 'vstinner', 'eric.smith', 'giampaolo.rodola', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'sandro.tosi', 'neologix', 'python-dev', 'petri.lehtinen']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue12191'
versions = ['Python 3.3']

[sandrotosi]

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!

[ericvsmith]

As a new feature, it can't be added to 2.7.

[neologix]

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.

[merwok]

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.

[gpshead]

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.

[merwok]

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.

[sandrotosi]

@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.

[merwok]

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 :)

[sandrotosi]

Here's the updated patch following review on Rietveld

[vstinner]

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

[merwok]

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.

[sandrotosi]

I've applied some of the suggestions Victor made (and also refresh to be cleanly applicable on default).

[sandrotosi]

Sorry for the late reply: I've applied Éric comments made on Rietveld.

[merwok]

You should replace the v5 file (or even remove all files, for clarity) with the actual output of hg diff, not hg status ;-)

[sandrotosi]

Gaaah, sorry about that: I've just uploaded the correct fifth version.

[pitrou]

I wonder if we should raise LookupError for unknown uids/gids. Do we have other APIs with similar semantics?

[sandrotosi]

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.

[bitdancer]

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.

[sandrotosi]

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?)

[merwok]

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.

[sandrotosi]

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).

[sandrotosi]

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.

[python-dev]

New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default':
bpo-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

[sandrotosi]

At last, it's in :) thanks a lot to all the people that helped me in the process!

[vstinner]

New changeset d1fd0f0f8e68 by Sandro Tosi in branch 'default'

You may add shutil.chmod to the "What's New in Python 3.3?" document.

[ezio-melotti]

Raymond, are you still taking care of the whatsnew?
Do you want people to update it when they add something new?

[python-dev]

New changeset 5d317e38da44 by Sandro Tosi in branch 'default':
bpo-12191: fix build failures, explicitly passing group argument when I want to test it
http://hg.python.org/cpython/rev/5d317e38da44

[merwok]

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.

[python-dev]

New changeset f2cb733c9a37 by Sandro Tosi in branch 'default':
bpo-12191: added entry in What's New (+ small editing on shutil section)
http://hg.python.org/cpython/rev/f2cb733c9a37

[merwok]

I have added 'chown' in shutil.__all__.