The implementation of fs::relative resolves symlinks which is not intended
in ListWalletDir. The replacement does what is required, and listwalletdir RPC
tests are fixed accordingly.
Also, fs::recursive_directory_iterator iteration is fixed to build with boost 1.47.
Based on #14559
56 std::vector<fs::path> paths;
57
58- for (auto it = fs::recursive_directory_iterator(wallet_dir); it != end(it); ++it) {
59+ for (auto it = fs::recursive_directory_iterator(wallet_dir); it != fs::recursive_directory_iterator(); ++it) {
60+ assert(it->path().string().compare(0, base.size(), base) == 0);
61+ const std::string relative = it->path().string().substr(base.size() + 1); // Account for directory separator
fs::path LexicallyRelative(fs::path path, fs::path base) function. Then later we could replace it with standard lexically_relative.
utACK 7879ac6ee8efbefafcecef7a0f7fcbdf888bb957 (last commit only). Only change is adding LexicallyRelative function. There are some other changes I might want to make on top of this:
path.string()[base.string().size()] character is a separator.But I think 7879ac6ee8efbefafcecef7a0f7fcbdf888bb957 is a nice minimal fix that will restore compatibility with previous boost versions.
48@@ -49,15 +49,21 @@ static bool IsBerkeleyBtree(const fs::path& path)
49 return data == 0x00053162 || data == 0x62310500;
50 }
51
52+static fs::path LexicallyRelative(const fs::path& path, const fs::path& base)
53+{
54+ assert(path.string().compare(0, base.string().size(), base.string()) == 0);
I don’t like using assert as error handling here
I previously suggested an exception instead of aborting (https://github.com/bitcoin/bitcoin/pull/14561#pullrequestreview-167964007), but this isn’t runtime error handling. It is checking an assumption the list code is making about the behavior of recursive_directory_iterator. In the new version of this PR that drops the assert, the assumption is no longer documented or checked, and if it’s violated ListWalletDirs will return garbage instead of aborting.
The implementation of fs::relative resolves symlinks which is not intended
in ListWalletDir. The replacement does what is required, and listwalletdir
tests are fixed accordingly.
Also, building with boost 1.47 required 2 changes:
- replace fs::relative with an alternative implementation;
- fix fs::recursive_directory_iterator iteration.
utACK a0daca2 which inlines the lexically relative logic again. I think this is less readable and more fragile than what I suggested, but it’s fine if others like this approach.
we had some discussion about this on IRC. I was fine with having it an utility function, I just believe utility functions need proper error handling and not ‘assert’, that was my only point that caused @promag to change it to this.
Honestly I don’t want to provide the utility function, it wasn’t needed until now, I doubt it will be needed and sadly boost 1.47 doesn’t have it. But if needed this can be extracted and unit tested. Until then this is covered by listwalletdir RPC and soon also exposed in the UI.
I’m also happy to follow up with previous @ryanofsky suggestions. I just think that the goal should be to fix boost build and listwalletdir tests first.
BTW, thanks for your time reviewers.
Honestly I don’t want to provide the utility function, it wasn’t needed until now, I doubt it will be needed
Thank you for being clear, that’s completely acceptable.
FWIW to me, too, it seems to small of a code unit to have much advantage of clarity of factoring it out. Let’s just leave it at this.