FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-94518: Rename `group*` to `extra_group*` to avoid confusion by arhadthedev · Pull Request #101054 · python/cpython · GitHub

/ cpython Public

gh-94518: Rename group* to extra_group* to avoid confusion - #101054

Merged
gpshead merged 7 commits into
python:mainfrom
arhadthedev:_posixsubprocess-rename-groups
Jan 26, 2023
ConversationCommits (7)ChecksFiles changed
Merged

gh-94518: Rename group* to extra_group* to avoid confusion#101054
gpshead merged 7 commits into
python:mainfrom
arhadthedev:_posixsubprocess-rename-groups

Conversation

arhadthedev commented Jan 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This PR addresses #94687 (comment):

Todo: replace groups_size/groups parameter names with extra_group_size/extra_groups across the whole file to avoid confusion of passers-by. It's necessary to mitigate a misleading name of setgroups that supposes replacement of not supplimentary group affinities, but all ones. The mislead results in seeing the omission of setgroups call for groups_size == 0 as a bug (like we should reset the inherited affinities but do nothing instead).

I'll do it in a separate PR because both this PR and gh-94519 are already big enough to clobber them with extra details.

Edit: this PR also unifies terminology by further renaming groups_list parameter and num_groups variable into extra_groups_packed and extra_group_size.

cc @gpshead

arhadthedev requested a review from gpshead as a code owner January 15, 2023 09:07
bedevere-bot mentioned this pull request Jan 15, 2023
Closed
arhadthedev marked this pull request as draft January 15, 2023 09:17

arhadthedev commented Jan 15, 2023

Copy link
Copy Markdown
Member Author

Converted to draft because num_groups and groups_list seem to require renaming too.

arhadthedev marked this pull request as ready for review January 15, 2023 09:53
arhadthedev marked this pull request as draft January 15, 2023 10:33

arhadthedev commented Jan 15, 2023

Copy link
Copy Markdown
Member Author

Converted to draft again to rerun tests because spurious PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\a\\cpython\\cpython\\build\\test_python_6372�\\test_python_worker_6400�' struck again on Windows x64.

arhadthedev marked this pull request as ready for review January 15, 2023 11:34

arhadthedev commented Jan 21, 2023

Copy link
Copy Markdown
Member Author

Removed spuriously added technical _packed suffix from a Python signature of fork_exec. It's relevant inside C code only (to distinguish PyObject and gid_t * variants of extra_groups).

However, a new extra_groups argument name retains because fork_exec arguments are positional-only.

gpshead merged commit 73245d0 into python:main Jan 26, 2023
arhadthedev deleted the _posixsubprocess-rename-groups branch January 26, 2023 07:05
mdboom pushed a commit to mdboom/cpython that referenced this pull request Jan 31, 2023
pythongh-94518: Rename group* to extra_group* to avoid confusion (p…
add6967 
…ython#101054)

* Rename `group*` to `extra_group*` to avoid confusion
* Rename `num_groups` into `extra_group_size`
* Rename `groups_list` to `extra_groups_packed`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

3 participants

Back | FazBrowse Home | New Git URL