On 11/5/14 5:10 PM, Adam Brightwell wrote:
> Attached is two separate patches to address previous
> comments/recommendations:
>
> * superuser-cleanup-shortcuts_11-5-2014.patch
Seeing that the regression tests had to be changed in this patch
indicates that there is a change of functionality, even though this
patch was previously discussed as merely an internal cleanup. So either
there is a user-facing change, which would need to be documented (or at
least mentioned, discussed, and dismissed as minor), or there is a fault
in the tests, which should be fixed first independently.
Which makes me wonder whether the other changes are indeed without
effect or just not covered by tests.
> * has_privilege-cleanup_11-5-2014.patch
I don't really see the merit of this patch. A "cleanup" patch that adds
more code than it removes isn't really a cleanup. If you want to make
the APIs consistent, then let's make a patch for that. If you want to
make the error messages consistent, then let's make a patch for that.
There is other work going on replacing these role attributes with
something more general, so maybe this cleanup should be done as part of
that other work.