Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@softins
Copy link
Contributor

@softinssoftins commentedApr 3, 2024•
edited by bedevere-appbot
Loading

Fixes#117467.

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

@softinssoftins requested a review froma team as acode ownerApril 3, 2024 16:48
@ghost
Copy link

ghost commentedApr 3, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

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.

os.chmod(new_file.name,info.st_mode)
try:
os.chown(new_file.name,info.st_uid,info.st_gid)
except:
Copy link
Member

@serhiy-storchakaserhiy-storchakaApr 3, 2024•
edited
Loading

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
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

@softins
Copy link
ContributorAuthor

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
Copy link
Member

!buildbot BSD

@bedevere-bot
Copy link

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

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
Copy link
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 theLib/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
Copy link
Member

!buildbot root

@bedevere-bot
Copy link

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

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

The builders matched are:

  • AMD64 Debian root PR

@softins
Copy link
ContributorAuthor

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 theLib/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.

@softinssoftinsforce-pushed thepreserve-mailbox-owner branch fromc8eb82c toc78971cCompareApril 3, 2024 22:19
@serhiy-storchaka
Copy link
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-storchakaserhiy-storchaka added the needs backport to 3.12only security fixes labelApr 4, 2024
@miss-islington-app
Copy link

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 requestApr 4, 2024
…17510)(cherry picked from commit3f5bcc8)Co-authored-by: Tony Mountifield <tony@mountifield.org>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link

GH-117537 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelApr 4, 2024
serhiy-storchaka added a commit that referenced this pull requestApr 4, 2024
GH-117537)(cherry picked from commit3f5bcc8)Co-authored-by: Tony Mountifield <tony@mountifield.org>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@softins
Copy link
ContributorAuthor

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.

@softinssoftins deleted the preserve-mailbox-owner branchApril 4, 2024 14:35
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
…17510)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-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
[8]ページ先頭
©2009-2025 Movatter.jp