Skip to content

remove HASSHORTSETGROUPS test, use system headers and types instead. #72

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 14, 2020

Conversation

alanpost
Copy link
Contributor

@alanpost alanpost commented Aug 22, 2019

This is the 1.08 work for PR#15, converting the codebase to use uid_t/gid_t and removing the short group test.

@DerDakon has PR #45 which is the same work--only one is needed, and they could we be identical. I was waiting for the 1.07 release before submitting this PR and now we can sort that out.

@alanpost alanpost force-pushed the pr-remove-hasshsgr-h branch from 820dd30 to 537e1b7 Compare August 22, 2019 13:28
@alanpost alanpost force-pushed the pr-remove-hasshsgr-h branch 2 times, most recently from 332702e to bc59fe2 Compare August 22, 2019 14:40
@alanpost alanpost added this to the 1.08 milestone Aug 22, 2019
@alanpost alanpost force-pushed the pr-remove-hasshsgr-h branch 6 times, most recently from 8e3d1dc to abcda33 Compare August 26, 2019 03:48
Copy link
Member

@schmonz schmonz left a comment

Choose a reason for hiding this comment

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

Compared to #45, this PR has more vestiges removed, more sys/types.h included where needed, and a few more int -> uid_t and gid_t.

@alanpost alanpost force-pushed the pr-remove-hasshsgr-h branch 2 times, most recently from 516482b to 55e3cfb Compare August 27, 2019 13:34
Copy link
Member

@DerDakon DerDakon left a comment

Choose a reason for hiding this comment

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

C89 API now that we agreed to introduce them for things we change anyway.

Affects only prot_gid() and prot_uid()

@schmonz
Copy link
Member

schmonz commented Dec 26, 2019

@DerDakon, since we've been dormant a while, can you summarize (for @alanpost, me, and anyone else reading) what's left to address before this looks good to you?

@DerDakon
Copy link
Member

Look one comment before ;) And then this needs a rebase.

@DerDakon DerDakon force-pushed the pr-remove-hasshsgr-h branch from 55e3cfb to ccff0d0 Compare April 11, 2020 12:59
@DerDakon
Copy link
Member

Oops, sorry. I was neither aware that my local branch still pointed to Alans original branch, nor that I have the permissions to override this. Please speak up if we should revert that or if we just want to merge it as it now is. It should address all my previous objections.

@DerDakon DerDakon force-pushed the pr-remove-hasshsgr-h branch from ccff0d0 to a4d96ce Compare April 21, 2020 17:20
@schmonz
Copy link
Member

schmonz commented Apr 24, 2020

Oops, sorry. I was neither aware that my local branch still pointed to Alans original branch, nor that I have the permissions to override this. Please speak up if we should revert that or if we just want to merge it as it now is. It should address all my previous objections.

I'm having trouble finding time and attention with which to review this. But I'm also realizing I don't feel comfortable being the second approver for this when it's based on @alanpost's work and saved on his branch. (And thank you, @DerDakon, for drawing attention to this unintentional result.)

I would be very comfortable adding a +1 iff Alan's comfortable with (1) the state of the code and (2) the state of the commit history. @alanpost, can you spare some round tuits to review (and meta-review)?

@DerDakon DerDakon force-pushed the pr-remove-hasshsgr-h branch from a4d96ce to b4b2a77 Compare May 8, 2020 14:46
@DerDakon
Copy link
Member

DerDakon commented May 8, 2020

I pushed this once more to resolve the issues I caused. The first commit is only rebased, and fixes the merge conflicts. These come from functions that have been changed to C89 style meanwhile. The second commit is mine, doing the C89 conversion for every function changed in the first patch that was not C89 style afterwards.

@DerDakon DerDakon self-requested a review May 8, 2020 20:08
@schmonz
Copy link
Member

schmonz commented May 11, 2020

Thank you for clearing this up. I’m comfortable now that Alan’s original work is clearly credited to him. Let’s give it a few more days before merging, just in case he wants to weigh in.

@DerDakon DerDakon force-pushed the pr-remove-hasshsgr-h branch from b4b2a77 to 0f9cb59 Compare May 11, 2020 21:06
@DerDakon DerDakon force-pushed the pr-remove-hasshsgr-h branch from 0f9cb59 to 7e89111 Compare May 12, 2020 16:23
@DerDakon DerDakon merged commit 7e89111 into notqmail:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants