On Tue, May 10, 2022 at 7:17 AM Phillip Wood [off-list ref] wrote:

On 07/05/2022 17:35, Carlo Marcelo Arenas Belón wrote:

quoted

A reroll for cb/path-owner-check-with-sudo with most of the suggestions
included but planned originally as an RFC, because I am frankly not sure
that I read and addressed all of it, but also because after seeing how
my only chance to get an official Reviewed-by: Junio vanished I also
realized that I wasn't clear enough and careful enough from the
beginning to explain the code correctly and therefore had maybe wasted
more of the review quota this change should require.

Important changes (eventhough most are not affecting the logic)
* Document hopefully better which environments are supported and what
   to do if you want to test it in one that is not (thanks to dscho)
* Removed the arguably premature optimization to try to keep the sudo
   cache warm which was actually buggy, as it was also not needed.
   The CI does passwordless sudo unconditionally and even when running
   it locally it will go so fast that is shouldn't be an issue (thanks
   to Phillip)
* No longer using the ugly and controversial variable name so now it
   will need GIT_TEST_ENABLE_SUDO to be used to enable it on CI (which
   is not done in this series, but a run with it enabled on top of
   seen is available[1])
* Stop the arguing about what is or not a regression worth fixing and
   instead document it as one through a test, which would be easy to
   fix in a follow up since the code was already provided by Junio

I've had a read of the patches and I agree with Junio's comments on the
second patch.

Not sure which comments you are referring to, but
I'd implemented Junio's suggestion and removed the "too clever" (uid_t)-1 in v4.

I'd really like us to avoid sudo in the tests if we can as
it causes a lot of problems.

Even if it might not seem like it, I agree with you both (and dscho)
too, and if I could come out with a way to do so that would be still
secure, I would do it in a pinch, but I can't (at least until now) and
I don't want for that to hold up this fix so will be publishing soon a
v4 that still uses sudo in the tests, I am afraid.

Just to let you know I'm going to be off
the list for the next couple of weeks, so I wont be looking at these
patches in that time.

Thanks for all your help reviewing them and more importantly improving
them, enjoy your time off.

Carlo