Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common/FileUtil: Revert behavior of CreateFullPath(). #11567

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Feb 15, 2023

This was accidentally changed in 7e6436d.

Compare the old code:

bool CreateFullPath(const std::string& fullPath)
{
int panicCounter = 100;
DEBUG_LOG_FMT(COMMON, "CreateFullPath: path {}", fullPath);
if (Exists(fullPath))
{
DEBUG_LOG_FMT(COMMON, "CreateFullPath: path exists {}", fullPath);
return true;
}
size_t position = 0;
while (true)
{
// Find next sub path
position = fullPath.find(DIR_SEP_CHR, position);
// we're done, yay!
if (position == fullPath.npos)
return true;
// Include the '/' so the first call is CreateDir("/") rather than CreateDir("")
std::string const subPath(fullPath.substr(0, position + 1));
if (!IsDirectory(subPath))
File::CreateDir(subPath);
// A safety check
panicCounter--;
if (panicCounter <= 0)
{
ERROR_LOG_FMT(COMMON, "CreateFullPath: directory structure is too deep");
return false;
}
position++;
}
}

to the new code:

bool CreateFullPath(const std::string& fullPath)
{
DEBUG_LOG_FMT(COMMON, "{}: path {}", __func__, fullPath);
std::error_code error;
auto native_path = StringToPath(fullPath);
bool success = fs::create_directories(native_path, error);
// If the path was not created, check if it was a pre-existing directory
if (!success && fs::is_directory(native_path))
success = true;
if (!success)
ERROR_LOG_FMT(COMMON, "{}: failed on {}: {}", __func__, fullPath, error.message());
return success;
}

Specifically the part where the old code returns as soon as it doesn't find any more directory separators.

@AdmiralCurtiss AdmiralCurtiss merged commit 089eab9 into dolphin-emu:master Feb 15, 2023
14 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the create-full-path-behavior-change branch February 15, 2023 02:12
@shuffle2
Copy link
Contributor

sorry, what was the problem here? don't get it

@jordan-woyak
Copy link
Member

sorry, what was the problem here? don't get it

CreateFullPath("dir1/dir2/file") was inadvertently also creating a "file" directory.

@sepalani
Copy link
Contributor

sepalani commented Feb 15, 2023

What about renaming CreateFullPath to CreateParentDirs or similar? AFAICT, the behaviour seems similar to mkdir -p which does just that. CreateFullPath is a confusing name which seems to imply creating the "fullpath" including directories and file.

EDIT: After a second thought maybe CreateDirsFromPath is a better name as I don't think it's strictly identical to mkdir -p neither.

@AdmiralCurtiss
Copy link
Contributor Author

Yeah, probably a good idea to rename that to be more clear.

@shuffle2
Copy link
Contributor

sorry, what was the problem here? don't get it

CreateFullPath("dir1/dir2/file") was inadvertently also creating a "file" directory.

ok, yea....I'd say dolphin should probably be changed to not expect that, but (I guess) it works as-is so can be punted.

@player112972
Copy link

player112972 commented Feb 15, 2023

I'm having a problem updating the emulator. Version 5.0-18661

Screenshot 2023-02-14 20 55 48

@AdmiralCurtiss
Copy link
Contributor Author

This is very much the wrong place for support. Go to the discord or the forums or something.

(but you probably just have to manually update, I'm guessing the issue addressed in this PR broke the updater for those affected versions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants