The value ofbufsize isn't limited. The result length may be limited. IfPATHCCH_ALLOW_LONG_PATHS isn't set inflags, andnew_len >= MAX_PATH (notMAXPATHLEN), the returned error should beHRESULT_FROM_WIN32(ERROR_FILENAME_EXCED_RANGE).

Else ifnew_len is greater thanbufsize, the returned error should beHRESULT_FROM_WIN32(ERROR_INSUFFICIENT_BUFFER).

PATHCCH_ALLOW_LONG_PATHS also determines whether to convert to or from an extended path, depending on whether the current process supports long paths (Python 3.6+ does). But it's okay to omit that behavior forGAMES.

Oh yeah, we shouldn't have references to MAXPATHLEN in here.

I'm not so concerned about error codes. They're only going to be reported to users, and really we ought to be getting the buffer right ourselves before calling.

I think it's fine for this implementation to assumePATHCCH_ALLOW_LONG_PATHS. Don't worry about mimicking every possible behaviour of the original.

Don't worry about mimicking every possible behaviour of the original.

I agree, if getting the correct behavior is excessively complicated with no tangible benefit. What about my other comment on this function? The wayPathCchStripToRoot() is used, whenrelfile is a rooted path, leads to incorrect and inconsistent behavior. If I know I'm combining a rooted path, I may use a smaller buffer because I know that most ofdirname won't be copied -- just the drive part. The current implementation requires using a buffer that's big enough to holddirname,relname and a joining backslash. OTOH, ifrelname has a drive or UNC share and thus replacesdirname, then the length of the passed-indirname is ignored.

Firstly, the suggested change is good.

I'm not particularly worried about the case whererelname would shorten the overall drive. It seems something youcould take advantage of, but generally we don't control either half of the paths that will come in here, which means the caller is going to have to assume worst case of plain-old concatenation.

Maybe just add a comment like/* assuming worst case result length, so our caller should as well */.

The only reasonable alternative (without complicating everything) would involve modifying the result buffer before potentially returning an error, which I think is worse. I'm pretty sure the original API uses temporary buffers internally, but there's no real need to go there.

Yes I would assume the implementation to use something likePathAllocCombine internally

If we really wanted to I could come up with an implementation that behaves closer toPathCchCombineEx. I was simply not sure it was worth the complexity (I kind of hope that at some point microsoft makes these APIs available on the xbox, since there does not appear to be any inherent reason to provide them on every platform except the xbox)

The only reasonable alternative (without complicating everything) would involve modifying the result buffer before potentially returning an error, which I think is worse. I'm pretty sure the original API uses temporary buffers internally, but there's no real need to go there.

It would calculatedir_len based onPathCchSkipRoot() ifrelfile begins with a backslash. Then it's the same initial test based onnew_len = dir_len + file_len + 2, requiringnew_len < bufsize. Copyingdirname would now usewcsncpy(buffer, dirname, dir_len), andPathCchStripToRoot() would not have to be implemented. So you get better behavior AND it's simpler overall.

Hold off on any more changes on this function for a day or two - I'm having some interesting conversations with colleagues right now. Might have a more suitable approach (or at least more information about it).

@zooba, since the check for building winreg and mmap occurs in several places (here, in "PC/config.c", and the respective modules), maybeHAVE_WINDOWS_REG_API andHAVE_WINDOWS_MMAP_API should be defined in "PC/pyconfig.h". And to match that, maybe renameHAVE_WINDOWS_CONSOLE_IO toHAVE_WINDOWS_CONSOLE_API. That's a better name anyway, sinceGenerateConsoleCtrlEvent() is a console API, but not one that's particularly related to I/O.

The mmap API should lose the definitions fairly soon, we just spun that work out to another issue.

For winreg I wouldn't worry aboutpyconfig.h, but the definition could be calculated at the top of the file. Those aren't C API functions, so they can be detected byAttributeError.

None of these feel important enough to block on, but I wouldn't oppose doing them later. Let's call this PR done and let Max focus on the other changes that have been lined up