On Tue, May 10, 2022 at 7:17 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > On 07/05/2022 17:35, Carlo Marcelo Arenas Belón wrote: > > 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
