On Wed, Mar 9, 2022 at 3:29 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Neeraj Singh via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > Including NTSecAPI.h in git-compat-util.h causes build errors in any > > other file that includes winternl.h. That file was included in order to > > get access to the RtlGenRandom cryptographically secure PRNG. This > > change scopes the inclusion of all PRNG headers to just the wrapper.c > > file, which is the only place it is really needed. > > It is true that wrapper.c is the only thing that needs these headers > included as part of its implementation detail of csprng_bytes(), and > I think I very much like this change for that reason. > > Having said that, if it true that including these two header files > in the same file will lead to compilation failure? That sounds like > either (1) they represent two mutually incompatible APIs that will > cause breakage when code that use them, perhaps in two separate > files to avoid compilation failures, are linked together, or (2) > these system header files are simply broken. Or something else? > > > -/* > > - * Including the appropriate header file for RtlGenRandom causes MSVC to see a > > - * redefinition of types in an incompatible way when including headers below. > > - */ > > -#undef HAVE_RTLGENRANDOM > > This comment hints it is more like (1) above? A type used in one > part of the system is defined differently in other parts of the > system? I cannot imagine anything but bad things happen when a > piece of code uses one definition of the type to declare a function, > and another piece of code uses the other definition of the same type > to declare a variable and passes it as a parameter to that function. > > I do not know this patch makes the situation worse, and I am not a > Windows person with boxes to dig deeper to begin with. Hence I do > not mind the change itself, but justifying the change primarily as a > workaround for some obscure header type clashes on a single system > leaves a bad taste. If the first sentence weren't there, I wouldn't > have spent this many minutes wondering if this is a good change ;-) This is (2), these system header files are simply broken. I've been looking deeper into why, but haven't bottomed out yet. Thanks, Neeraj
