The value of bufsize isn't limited. The result length may be limited. If PATHCCH_ALLOW_LONG_PATHS isn't set in flags, and new_len >= MAX_PATH (not MAXPATHLEN), the returned error should be HRESULT_FROM_WIN32(ERROR_FILENAME_EXCED_RANGE).

Else if new_len is greater than bufsize, the returned error should be HRESULT_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 for GAMES.

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 assume PATHCCH_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 way PathCchStripToRoot() is used, when relfile 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 of dirname won't be copied -- just the drive part. The current implementation requires using a buffer that's big enough to hold dirname, relname and a joining backslash. OTOH, if relname has a drive or UNC share and thus replaces dirname, then the length of the passed-in dirname is ignored.

Firstly, the suggested change is good.

I'm not particularly worried about the case where relname would shorten the overall drive. It seems something you could 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 like PathAllocCombine internally

If we really wanted to I could come up with an implementation that behaves closer to PathCchCombineEx. 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 calculate dir_len based on PathCchSkipRoot() if relfile begins with a backslash. Then it's the same initial test based on new_len = dir_len + file_len + 2, requiring new_len < bufsize. Copying dirname would now use wcsncpy(buffer, dirname, dir_len), and PathCchStripToRoot() 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).

What if dirname is "G:\eggs" and relfile is the relative rooted path "\spam"? This should be joined as "G:\spam", not as "G:\eggs\spam". relfile could also be a relative drive path such as "T:spam", which should join as "T:spam" because it's a different drive, not as "G:\eggs\T:spam". Code that's meant for POSIX doesn't handle such cases. Maybe that's good enough.

I mostly selected this implementation for it's simplicity. On the xbox you should probably simply work with absolute paths, so I considered it as good enough for now. That said a better implementation would certainly not hurt.

On all platforms you should work with absolute paths :)

You're likely in control of your development process well enough to ensure it, but we need to account for broader expectations than that when it comes into core.

Hm I guess on Windows/Linux we do use paths relative to the binary we ship. On Xbox basically every application has their "own" filesystem, so you really use absolute paths everywhere. I guess that might be the reason why they do not provide the API there.

Anyway I know to little about WIndows file path concatenation to come up with a better implementation.

The POSIX code is okay as long as it's always a simple relative path such as "eggs\foo.py". One minor annoyance is if dirname ends in a forward slash, such as "T:/spam/". It will add SEP to join it as "T:/spam/\eggs\foo.py". Such a path still works, so it's not a technical problem.

I added an implementation which should be able to handle more of these cases on the xbox.