Re: [PATCH v5 1/5] wrapper: move inclusion of CSPRNG headers the wrapper.c file

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On 2022-03-09 at 23:03:14, Neeraj Singh via GitGitGadget wrote:
> From: Neeraj Singh <neerajsi@microsoft.com>
> 
> 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.

We generally prefer to do system includes in git-compat-util.h because
it allows us to paper over platform incompatibilities in one place and
to deal with the various ordering problems that can happen on certain
systems.

It may be that Windows needs additional help here; I don't know, because
I don't use Windows.  I personally find it unsavoury that Windows ships
with multiple incompatible header files like this, since such problems
are typically avoided by suitable include guards, whose utility has been
well known for several decades.  However, if that's the case, let's move
only the Windows changes there, and leave the Unix systems, which lack
this problem, alone.

It would also be helpful to explain the problem that Windows has in more
detail here, including any references to documentation that explains
this incompatibility, so those of us who are not Windows users can more
accurately reason about why we need to be so careful when including
header files there and why this is the best solution (and not, say,
providing our own include guards in a compat file).
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]