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

cwk_path_is_absolute returns an incorrect result #24

Open
kampeador opened this issue Sep 26, 2021 · 5 comments
Open

cwk_path_is_absolute returns an incorrect result #24

kampeador opened this issue Sep 26, 2021 · 5 comments

Comments

@kampeador
Copy link

Hello.
Thanks for your library and putting a lot of effort into it.

I've made this simple unit test and 5 asserts have failed.

#ifdef _WIN32
  const bool is_unix = false;
  cwk_path_set_style(CWK_STYLE_WINDOWS);
#else
  const bool is_posix = true;
  cwk_path_set_style(CWK_STYLE_UNIX);
#endif

  assert( cwk_path_is_absolute("/") == is_unix  ); // fails
  assert( cwk_path_is_absolute("/dir") == is_unix  ); // fails
  assert( cwk_path_is_absolute("/dir/") == is_unix  ); // fails
  assert( cwk_path_is_absolute("/dir/tmp") == is_unix  ); // fails
  assert( cwk_path_is_absolute("/dir/tmp/") == is_unix  ); // fails
  assert( ! cwk_path_is_absolute("dir") );
  assert( ! cwk_path_is_absolute("dir/") );
  assert( ! cwk_path_is_absolute("dir/tmp") );
  assert( ! cwk_path_is_absolute("dir/tmp/") );
  assert( ! cwk_path_is_absolute("c:") );
  assert( ! cwk_path_is_absolute("c:dir") );
  assert( ! cwk_path_is_absolute("c:dir/") );
  assert( ! cwk_path_is_absolute("c:dir/tmp") );
  assert( ! cwk_path_is_absolute("c:dir/tmp/") );
  assert( cwk_path_is_absolute("c:/") == !is_unix  );
  assert( cwk_path_is_absolute("c:/dir") == !is_unix  );
  assert( cwk_path_is_absolute("c:/dir/") == !is_unix  );
  assert( cwk_path_is_absolute("c:/dir/tmp") == !is_unix  );
  assert( cwk_path_is_absolute("c:/dir/tmp/") == !is_unix  );

Windows also has a PathIsRelativeA/W function, which can be used as reference. However it has some limitations e.g MAX_PATH limit.

@likle
Copy link
Owner

likle commented Sep 26, 2021

Hi @kampeador, thank you very much for your tests! I really appreciate it.

I believe the first five tests fail because cwalk consideres these to be absolute on windows too (please correct me if I am wrong).
PathIsRelative (shlwapi.h) does not seem to consider these to be absolute, and I am not sure why this is the case. The paths start with a slash, and usually that's considered to be equal with a backslash on most APIs. For instance, if you type cd / in a console it will bring you to the root of your current drive. Microsoft even has a few examples here, and the second one clearly shows that paths starting with a (back)slash should be absolute. I ran the following simple test based on those examples provided by Microsoft:

#include <Windows.h>
#include <shlwapi.h>
#include <stdio.h>

int main() {
  const char *paths[] = {
	"C:\\Documents\\Newsletters\\Summer2018.pdf",
	"\\Program Files\\Custom Utilities\\StringFinder.exe",
	"2018\\January.xlsx",
	"..\\Publications\\TravelBrochure.pdf",
	"C:Projects\\apilibrary\\apilibrary.sln",
	"C:\\Projects\\apilibrary\\apilibrary.sln"
  };

  for(int i = 0; i < sizeof(paths) / sizeof(*paths); ++i) {
	const char *result = PathIsRelativeA(paths[i]) ? "relative" : "absolute";
	printf("%s: %s\n", paths[i], result);
  }
}

which shows that C:Projects\apilibrary\apilibrary.sln is considered to be absolute by PathIsRelativeA which is probably not correct as Microsoft puts it:

A relative path from the current directory of the C: drive.

I'd be happy to change things but I am not sure what the reasoning behind this is, on my current knowledge it seems to me as if PathIsRelative is not always working as it should.

@kampeador
Copy link
Author

I guess I've misunderstood terminology related to this library.
Absolute and relative paths have nothing to do in terms of comparison with shlwapi, C++17 filesystem, Qt etc.
They are related to the context of cwalk.
e.g. every cwk_path_get_absolute call must be validated with specific functions like GetFullPathNameW used to translate paths to specific context.

@likle
Copy link
Owner

likle commented Sep 27, 2021

I am not entirely sure whether I undestand you correctly. Could you elaborate on your example? Do you think we need to improve something in the documentation?

@kampeador
Copy link
Author

For example: On Windows "absolute path" = "full path". "/", "/dir", "/dir/tmp" are considered relative in shlwapi, C++17 filesystem, Qt etc because there is an "Apply the current directory" translation stage.

From the link you've posted earlier:

If the path starts with a single component separator, the drive from the current directory is applied. For example, if the file path is \utilities and the current directory is C:\temp, normalization produces C:\utilities.

Drive can be changed dynamically using SetCurrentDirectoryW or _chdir functions.
cwalk is not aware of this global state and treats these relative paths as absolute. A potential solution could be using GetCurrentDirectoryW or _getcwd to supply this information for cwalk, but it will not work.

char buffer[FILENAME_MAX];
cwk_path_get_absolute("C:\temp\data", "\utilities", buffer, sizeof(buffer));
printf("buffer: %s\n", buffer);

will print \utilities instead of C:\utilities. Information is lost because it can be C,D,E or whatewer, which needs to be validated by other functions like GetFullPathNameW or by setting global state etc.

@likle
Copy link
Owner

likle commented Sep 28, 2021

Thanks @kampeador , I understand your issue. I will have to think about that and whether I can do something about it.

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

No branches or pull requests

2 participants