Carlo Arenas <carenas@xxxxxxxxx> writes:
>> So unless the refactoring is done to any other end than for refactoring's
>> own sake (which is really not a good reason), I see it as problematic.
>
> I couldn't agree more with that sentiment, but in this case the
> refactoring was done to clean up the recently introduced function
> which was indeed too ugly (some would say on purpose) and replacing it
> with not only better looking code, but also a cleaner interface.
>
> I would think that in this specific case an exemption could be
> granted, but it is also true that while this code is almost a month
> old, it hasn't got the review it will require to be merged so late in
> the release cycle without raising the concerns you both fairly put
> forward.
>
>> > if (euid == ROOT_UID)
>> > - extract_id_from_env("SUDO_UID", &euid);
>> > + return id_from_env_matches("SUDO_UID", st.st_uid);
>>
>> A much shorter, much more obvious patch would look like this:
>>
>> - if (euid == ROOT_UID)
>> + if (st.st_uid != euid && euid == ROOT_UID && )
>> extract_id_from_env("SUDO_UID", &euid);
>>
>> It accomplishes the same goal, but is eminently easier to review. For
>> regression fixes, I much prefer the safety and confidence that comes with
>> that.
>
> will reroll with your suggestion, thanks again for your helpful
> review, and apologies again to everyone for not cleaning it up earlier
> in the cycle.
As long as the syntax error with the "like this" suggestion gets
fixed, I do not mind if you took the above verbatim ;-)
Quite honestly, at this low level of small "the caller asks if it is
safe for the current user to step into the given path to run git"
function, it would be a waste of time to quibble about the
separation of concerns between the helper function that figures out
who the "current user" is, and checking it against the user who owns
the path. It is not like we are talking about a brutal-to-reviewers
250-line function that is called from multiple places after all.
The extract_id_from_env() helper could be inlined the caller and the
issue would disappear (I am not making a suggestion to actually do
so, but an illustration why it does not matter much in the larger
picture for something miniscule like this).
Where the function name and the separation of concerns truly matters
is between the is_path_owned_by*() helper and its callers, as in the
longer term I expect it won't be just "ownership" that we would care
about and once we start checking permission bits and other things for
the path, we'd regret the "owned by" part of the name.
But that is not our immediate concern, so let's take whatever that
is not incorrect and can achieve "concensus" the easiest and move
on.
Thanks for sticking with the larger topic of making the end-user
experience better around here. Very much appreciated.
