search

shadow-maint / shadow

Upstream shadow tree
Other
292 stars 228 forks source link

src/sulogin.c: Simpler password handling #913

Closed ferivoz closed 7 months ago

ferivoz commented 7 months ago

The pass array can be removed. It is possible to directly work with the result of agetpass, which in turn removes the need to prepare and clean up the array.

And there was one more external variable which was not in use.

alejandro-colomar commented 7 months ago

Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar commented 7 months ago

Thanks @hallyn ! :)

alejandro-colomar commented 7 months ago

@ferivoz Sorry, I introduced some conflict, due to changing the way pw_entry() returns. Can you please rebase? (I would do a manual merge to resolve the conflict, but github won't take it :| )

After you rebase, please show git range-diff 01fad16^..9f956da origin/master..sulogin_pass; (or whatever refs you use).

ferivoz commented 7 months ago

When became shadow so overly bureaucratic and complex? Especially after pushing a change into this before this PR was merged, while knowing exactly that it will create conflicts?

It's up to you how to handle this project. Since this change is not that important, we can just close the pull request.

alejandro-colomar commented 7 months ago

Hi @ferivoz ,

Especially after pushing a change into this before this PR was merged, while knowing exactly that it will create conflicts?

Honestly, I didn't know it. If I knew, I wouldn't have done it, and would have waited for this to be merged before doing the other change. I just forgot that these two touch code that is close to each other.

When became shadow so overly bureaucratic and complex?

I'm very sorry if that's the impression.

I asked for the range-diff, because it makes it a lot easier to review the changes from v1 to v2, and presented you the command to make it easier for you, in case you don't use it often.

Github's interface shows a diff when you force-push, but if you force-push after a rebase, it includes all the changes that have been introduced to master in the meantime, which makes it difficult to see what changes correspond actually to the PR. This is a limitation of GH's interface. Which means you end up having to review the patches from scratch again.

Anyway, seeing that you're not the only one that complained about my reviews, I assume it's my fault, so I'll try to be less bureaucratic with reviews, and do less reviews, in general.

It's up to you how to handle this project. Since this change is not that important, we can just close the pull request.

No, please. Again, I'm sorry.

Have a lovely day, Alex

alejandro-colomar commented 7 months ago

LGTM. Thanks for your work!