On Fri, May 6, 2022 at 2:43 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Carlo Arenas <carenas@xxxxxxxxx> writes: > > > Since I am renaming it anyway as part of this topic with RFC v4, would > > it be a good idea to require both? > > > > I see the "IKNOWHATIAMDOING" not as a normal GIT_TEST flag, but as a > > "here be dragons!" warning, and I later found that I either > > misremembered it being enabled in the CI, or it was dropped with one > > of those refactors we do often there. > > > > My RFC v4 includes a new nice looking GIT_TEST variable as suggested > > by Phillip which I am also enabling in the CI to hopefully make it > > even more clear that this is only meant to run there, but sadly that > > also means that this patch will likely have a conflict when merged > > upwards. > > This must build from the older mainteance tracks like maint-2.30, so > let's keep the changes to absolute minimum makes sense, but I still unsure about the two questions I had above: * would it make sense to make it run ONLY if both variables are set to YES or is it enough to use one (most likely the GIT_TEST one) that we would enable independently in CI? the advantage I see of having both variables is that it is even less likely to even run accidentally in a desktop somewhere and maybe break that test, and also keeps the meaning I wanted to attach to it with that ugly looking flag that no one should ever try to enable in their workstations unless they really know what they are doing. The advantage of ONLY having the GIT_TEST one is that it will be easier to enable in CI and for whoever wants to play with it on their workstation as well, but might still encourage people trying to make it work and wasting their time. * since we have to enable CI for these to be useful, would that be something to be done in an additional patch as part of this topic branch and you will only pull the commits before it to maint to avoid conflicts, or should it be done completely independently as a mini feature branch that depends on this one that will be pulled to seen and merged downwards from it? somehow offtopic but just curious about your process, presume that if we go with a single topic branch adding it instead as 2/4 would break your flow/scripts since the only way to get that merged would be to cherry-pick, right? > I actually think 1/3 and 3/3 are OK. Are there remaining issues in > these two patches (which only are tests)? The versions of them in RFCv4 have more documentation and are cleaner since they mostly include most of the feedback that was collected on them (even if I am still unsure because it is spread around and difficult to track, hence why I was planning an RFC) I don't think there is anything significantly different though but they are and will need another review (which I am hoping would be uncontroversial) > As to 2/3, I think the code is basically already fine, but a > simplification like the following on top would be a good idea. > > * We clear errno before making strtoul() call, so any non-zero > errno must have happeneed in strtoul(), which includes ERANGE. > There is no point checking the returned value env_id; if it is > ULONG_MAX but errno is 0, then the SUDO_UID legitimately is > naming a user whose UID is that special value, and it is not an > indication of an overflow. true, but the apparent check for ULONG_MAX (which should have a comment added) was really a check not to overflow when assigning the value we got into uid_t, by sacrificing an unlikely to be valid ULONG_MAX as an uid. it also has the intentional side effect of breaking this code if uid_t is signed and hopefully triggering a warning in the process since it would be always false, that way whoever has a system where this type is signed will have to carry their own version of the code and we don't have to deal with the portability of it. lastly (since it really made me proud and would be sad to see it go) it ALSO avoids someone trying to sneak a value that would overflow in one of the most common configurations we will run where sizeof(long) > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T (which only exists in a few systems and therefore can't be relied on) to discard values that would overflow the range uid_t has. without it, we would probably find ourselves in the future having to deal with an embarrassing bug that others before us had suffered. Carlo
