BASE: Ensure folder path when file path set in command line

[antoniou79]

The "soundfont" option is excluded from this, since that is expected to be a file path.

The PR is submitted for consideration as an extension to the command line to allow more leniency with the path arguments, ie. when a file path is set for a folder path argument in command line, it is converted to the appropriate folder path for the file, so that scummvm won't throw an error and exit.

The motivation for the PR was the discussion in this forum thread: https://forums.scummvm.org/viewtopic.php?t=17015

[bluegr]

Since this code is repeated, could we wrap it into a reusable function?

[antoniou79]

I couldn't come up with something pretty for a reusable function.

I did a few iterations on implementing one, but the different cases per path option (isReadable() check or isWriteable check), the custom usage() messages (which should stay outside the function) and the updating of the settings[] value for the option, made the re-usable method complex and ultimately reduced readability of the code, rather than helped.

Any suggestions are of course welcome.

For the record, my monstrosity looked like this:
// Assumes that path.exists() check is true
bool ensureAccessibleDirectoryForPathOption(Common::StringMap &settings, const Common::String optionKeyStr, const Common::FSNode &path, bool ensureWriteable) {
if (path.isDirectory()) {
if ((ensureWriteable && path.isWritable())
|| (!ensureWriteable && path.isReadable())) {
return true;
}
} else if (path.getParent().isDirectory()
&& ((ensureWriteable && path.getParent().isWritable())
|| (!ensureWriteable && path.getParent().isReadable())) ) {
// The path.getParent().isDirectory() check is redundant but included for clarifying
// the purpose of this case, which is to get the folder path part from a file path
settings[optionKeyStr] = path.getParent().getPath().toString(Common::Path::kNativeSeparator);
return true;
}
return false;
}

So that the parsing for eg. the screenshotpath would look like:
DO_LONG_OPTION("screenshotpath")
Common::FSNode path(option);
if (!path.exists()) {
usage("Non-existent screenshot path '%s'", option);
} else if (!ensureAccessibleDirectoryForPathOption(settings, "screenshotpath", path, true)) {
usage("Non-writable screenshot path '%s'", option);
}
END_OPTION

[antoniou79]

Since this code is repeated, could we wrap it into a reusable function?

I went ahead an committed the method above, with some comments to clarify its purpose

[criezy]

Suggested change

	} else if (path.getParent().isDirectory()
	&& ((ensureWriteable && path.getParent().isWritable())
	|| (!ensureWriteable && path.getParent().isReadable())) ) {
	} else if (path.getParent().isDirectory()
	&& ensureAccessibleDirectoryForPathOption(settings, optionKeyStr, path, ensureWritable)) {

I am not sure this makes the code more readable, but you could do a recursive call here to avoid duplicating the same condition.

[criezy]

Also I am wondering if accepting files as argument makes sense for all options. In particular this seems a strange behaviour for writable paths (e.g. screenshots).

So you could have something like

bool ensureAccessibleDirectoryForPathOption(Common::StringMap &settings, const Common::String optionKeyStr, const Common::FSNode &path, bool ensureWriteable, bool acceptFile) {
	if (path.isDirectory()) {
		...
	} else if (acceptFile && ensureAccessibleDirectoryForPathOption(settings, optionKeyStr, path.getParent(), ensureWriteable, false)) {
		...
	}
	return false;
``

[criezy]

Also I am wondering if FSNode::isDirectory() work as expected in the case of a symbolic link?

[antoniou79]

Also I am wondering if FSNode::isDirectory() work as expected in the case of a symbolic link?

I did a few tests on Linux (Ubuntu 22.04, ln and ln -s) and Windows 10 (mklink, mklink /D, mklink /H, mklink /J), with the current version of the code for the PR (I did not implement the recursive version yet).

For links to folders (symbolic or hard) the code works, and isDirectoty() works as expected (returns true). This is on both Linux and Windows.

For links to files (again symbolic or hard) the isDirectory() returns false, and the code enters the second clause. In both OSes it then returns false, but for different reasons. On Linux, it seems that getParent() returns the same FSNode (or a FSNode for the same path as the symbolic link) and the isDirectory() check returns false (as it did before we attempted getParent()). On Windows, getParent() returns an FSNode with empty path, for which isDirectory() is true, but it fails readability and writeability checks, so false is returned.

So the code does (still) support symbolic links to folder paths, but for symbolic links to files, it won't get a valid parent folder path.

This seems acceptable to me (of course more tests/checks by anyone else are welcome), but I should probably add an extra note in the comments that it does not work (does not get a valid parent path) for symbolic links to files and will return false.

[criezy]

In theory a path could be writable and not readable. In the case of screenshots we only need it to be writable, but in the case of savegames we need it to be both writable and readable. I know we do not currently check it is readable, but we could use this change to also add that check.

[sev-]

Thank you!