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
It might be good to pull this logic out into a reusable fs::path LexicallyRelative(fs::path path, fs::path base) function. Then later we could replace it with standard lexically_relative.
Done.
utACK ee15451f65c73bc51a7fb2e3fda105488ce16915 (just this commit, not base commits). I think this fix is preferable to the one in #14531 because it avoids getting confused by symlinks and duplicating work already done by the directory iterator.
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.
I will rebase once base is merged.
Rebased.
utACK 8c38f991f8925834c3db390d3c15e42a13c8984c
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 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.
@ryanofsky @ken2812221 please review updated version which inlines the function.
utACK
utACK a0daca2342dbab588db0461ad292d4a4a0d8879c 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.
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.
Squashed.
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.
utACK squashed ed2e18398b3ab657e98e3e1fe135cbf8dd94fda3
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.
Compiles and tests run fine on Debian Jessie PPC (big endian).
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.