search

shadow-maint / shadow

Upstream shadow tree
Other
304 stars 232 forks source link

Refactor pw_entry() and src/sulogin.c #911

Closed alejandro-colomar closed 9 months ago

alejandro-colomar commented 9 months ago

Cc: @hallyn , @ferivoz

https://github.com/shadow-maint/shadow/pull/908

alejandro-colomar commented 9 months ago

This PR would make this comment obsolete: https://github.com/shadow-maint/shadow/commit/9a7f5c6b16ef9d1ca8637a3dcb5e27cd1c5dc372#diff-cfc1a71a8d276e6874357906177011d16928d127f4f639d4ea14050c85f817b4R197.

Edit: this comment was meant for a different PR. Ignore it.

hallyn commented 9 months ago

Yeah it looks good to me. Looked closely this morning but you've added a few commits, so want to look again.

alejandro-colomar commented 9 months ago

(It's she, AFAIK :)

I didn't really coordinate. Was waiting for her to comment on her own PR, but since I had some time, I wrote this code.

While reviewing her PR, I also thought refactoring before would make more sense than after, as it would have helped review the patch itself (since it depends on too many assumptions on the caller side). I didn't say anything because she offered refactoring afterwards, but as you also suggested that it would make sense to do it together, I prepared this, in case she wanted to rebase after this.

@ferivoz whatever you prefer. I can rebase after your patch, or you can rebase after these (and even merge them to your own PR if that's easier for you).

alejandro-colomar commented 9 months ago

Yeah it looks good to me. Looked closely this morning but you've added a few commits, so want to look again.

Ahh, sorry, I didn't add range-diffs because since it was still a draft, I didn't expect you would've looked at it yet. Next time I'll try to post range-diffs since earlier versions, if that helps you.

hallyn commented 9 months ago

(It's she, AFAIK :)

Sorry!

Ok so let's merge this as is and then you two can proceed as you see fit :)