From: Phillip Wood Date: Tue, 10 May 2022 14:17:15 GMT Subject: Re: [RFC PATCH v4 0/3] fix `sudo make install` regression in maint Message-ID: In-Reply-To: <20220507163508.78459-1-carenas@gmail.com> Hi Carlo 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 been 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 Philip) > * 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. I'd really like us to avoid sudo in the tests if we can as it causes a lot of problems. 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. Best Wishes Phillip > Lastly I am little concerned by the fact this is going to maint but > has a "weather balloon" of sorts, which might not be wise, since it > might prevent people that might be affected from upgrading if they > have a -Werror policy. > > The effect is minor though, as worst case, if someone has a system > with a signed uid_t then this "feature" wouldn't work for them and > nothing has changed but I think it is worth to consider the alternatives > which are (in my own order of preference) > > * Revert the change to use unsigned long and strtoul() > > This will mean that people running in a 32bit system with an uid bigger > than INT_MAX wouldn't be able to use the feature > > * Move the code out (which is indeed an artificial restriction) so that > we can use intmax_t and strtoimax() instead and a cast to compare the > uid_t. > > This avoids all issues and restrictions but means more code changes > > * Throw away the custom function and expand the API ones to be used > instead as dscho suggested. > > Even more code changes, but maybe less risk as we will be building > on top of battle tested code. > > [1] https://github.com/carenas/git/actions/runs/2286452160 > > Carlo Marcelo Arenas Belón (3): > t: regression git needs safe.directory when using sudo > git-compat-util: avoid failing dir ownership checks if running > privileged > t0034: add negative tests and allow git init to mostly work under sudo > > Documentation/config/safe.txt | 10 ++++ > git-compat-util.h | 49 +++++++++++++++- > t/lib-sudo.sh | 12 ++++ > t/t0034-root-safe-directory.sh | 103 +++++++++++++++++++++++++++++++++ > 4 files changed, 173 insertions(+), 1 deletion(-) > create mode 100644 t/lib-sudo.sh > create mode 100755 t/t0034-root-safe-directory.sh >