Skip to content

gh-117467: Add preserving of mailbox owner on flush#117510

Merged
serhiy-storchaka merged 4 commits into
python:mainfrom
softins:preserve-mailbox-owner
Apr 4, 2024
Merged

gh-117467: Add preserving of mailbox owner on flush#117510
serhiy-storchaka merged 4 commits into
python:mainfrom
softins:preserve-mailbox-owner

Conversation

@softins

@softins softins commented Apr 3, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Fixes #117467.

Copy owner and group from the old file to the new when rewriting a mailbox. Ignore any failure from os.chown(), which may be platform-dependent.

@softins
softins requested a review from a team as a code owner April 3, 2024 16:48
@ghost

ghost commented Apr 3, 2024 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app Bot mentioned this pull request Apr 3, 2024
Closed
@bedevere-app

bedevere-app Bot commented Apr 3, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

serhiy-storchaka
serhiy-storchaka reviewed Apr 3, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a user visible change, it needs a NEWS entry. You can read above how to create it.

Could you please also add a test? It should create a mailbox, change its user and group, modify it, and check that its user and group are preserved. This test can only be run as root, but it is better than nothing.

Comment thread Lib/mailbox.py Outdated
os.chmod(new_file.name, info.st_mode)
try:
os.chown(new_file.name, info.st_uid, info.st_gid)
except:

@serhiy-storchaka serhiy-storchaka Apr 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently ignoring arbitrary exceptions is never a good idea. KeyboardInterrupt, MemoryError and RecursionError can be raised by virtually any code.

It is better to make the exception check more specific, for example: (PermissionError, AttributeError).

@bedevere-app

bedevere-app Bot commented Apr 3, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@softins

softins commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

Since it is a user visible change, it needs a NEWS entry. You can read above how to create it.

Done. Hope it's ok.

Could you please also add a test? It should create a mailbox, change its user and group, modify it, and check that its user and group are preserved. This test can only be run as root, but it is better than nothing.

As a novice contributor, I will have to work out how to add a test. Is there any tutorial?

@serhiy-storchaka

serhiy-storchaka commented Apr 3, 2024

Copy link
Copy Markdown
Member

!buildbot BSD

@bedevere-bot

bedevere-bot commented Apr 3, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit c8eb82c 🤖

The command will test the builders whose names match following regular expression: BSD

The builders matched are:

  • AMD64 FreeBSD PR
  • AMD64 FreeBSD14 PR
  • AMD64 FreeBSD15 PR

@serhiy-storchaka

serhiy-storchaka commented Apr 3, 2024

Copy link
Copy Markdown
Member

As a novice contributor, I will have to work out how to add a test. Is there any tutorial?

https://devguide.python.org/testing/run-write-tests/ may help.

Usually you find an appropriate file in the Lib/test/ directory and add a test similar to other tests in this file. But a test required for this PR is not usual, so I wrote it for you.

@serhiy-storchaka

serhiy-storchaka commented Apr 3, 2024

Copy link
Copy Markdown
Member

!buildbot root

@bedevere-bot

bedevere-bot commented Apr 3, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit c8eb82c 🤖

The command will test the builders whose names match following regular expression: root

The builders matched are:

  • AMD64 Debian root PR

@softins

softins commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

As a novice contributor, I will have to work out how to add a test. Is there any tutorial?

https://devguide.python.org/testing/run-write-tests/ may help.

Usually you find an appropriate file in the Lib/test/ directory and add a test similar to other tests in this file. But a test required for this PR is not usual, so I wrote it for you.

That is brilliant, thank you very much indeed! Although I've programmed for decades, Python is still relatively new to me.

@softins
softins force-pushed the preserve-mailbox-owner branch from c8eb82c to c78971c Compare April 3, 2024 22:19
@serhiy-storchaka
serhiy-storchaka merged commit 3f5bcc8 into python:main Apr 4, 2024
@serhiy-storchaka

serhiy-storchaka commented Apr 4, 2024

Copy link
Copy Markdown
Member

Thank you for your contribution @softins.

We usually do not use rebase and force push. It makes iterative reviewing more difficult. All consequent changes are just stacked one over other during review, and squashed into a single changeset right before merging.

@serhiy-storchaka serhiy-storchaka added the needs backport to 3.12 only security fixes label Apr 4, 2024
@miss-islington-app

miss-islington-app Bot commented Apr 4, 2024

Copy link
Copy Markdown

Thanks @softins for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 4, 2024
@softins @serhiy-storchaka @miss-islington
pythongh-117467: Add preserving of mailbox owner on flush (pythonGH-1…
1a2c115 
…17510)

(cherry picked from commit 3f5bcc8)

Co-authored-by: Tony Mountifield <tony@mountifield.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app

bedevere-app Bot commented Apr 4, 2024

Copy link
Copy Markdown

GH-117537 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Apr 4, 2024
serhiy-storchaka added a commit that referenced this pull request Apr 4, 2024
@miss-islington @softins @serhiy-storchaka
[3.12] gh-117467: Add preserving of mailbox owner on flush (GH-117510) (
14aef56 
GH-117537)

(cherry picked from commit 3f5bcc8)

Co-authored-by: Tony Mountifield <tony@mountifield.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@softins

softins commented Apr 4, 2024

Copy link
Copy Markdown
Contributor Author

We usually do not use rebase and force push. It makes iterative reviewing more difficult. All consequent changes are just stacked one over other during review, and squashed into a single changeset right before merging.

Ok, thank you. I'll remember that for any future contributions.

@softins
softins deleted the preserve-mailbox-owner branch April 4, 2024 14:35
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@softins @serhiy-storchaka @diegorusso
pythongh-117467: Add preserving of mailbox owner on flush (pythonGH-1…
3936899 
…17510)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

mailbox.mbox.flush() loses mailbox owner

3 participants

@softins @serhiy-storchaka @bedevere-bot