stefankueng / CryptSync

CryptSync is a small utility that synchronizes two folders while encrypting the contents in one folder. That means one of the two folders has all files unencrypted (the files you work with) and the other folder has all the files encrypted.
https://tools.stefankueng.com/CryptSync.html
GNU General Public License v3.0
400 stars 72 forks source link

*** CODE REVIEW REQUEST *** #118

Closed dansyl1 closed 4 months ago

dansyl1 commented 4 months ago

Fixed issues with long path names: 1) Long path names were not correctly watched 2) Sometimes crashed due to buffer overrun. Two alternatives are for review: a) solution with static buffer and b) solution with dynamically-allocated buffer (depending on value of USE_STATIC_BUF).

I will do the code cleanup based on your feedback and keep only the solution you'll select in the next pull request.

Also, I do understand why there is a reference to std::wstring on line 251. It allocates a new std::wstring object just for the call to the .insert method. Could buf_ptr be used directly as argument to the .insert method?

stefankueng commented 4 months ago

looks good! For me, either way is fine: static or dynamic. Or, if we want to handle very large paths without constantly reallocating, I often use an approach like this:

static size_t largeBufSize = 4096;
static auto largeBuf = std::make_unique<wchar_t[]>(largeBufSize);

if (requestedBufSize > largeBufSize)
{
  largeBufSize = requestedBufSize + 1024;
  largeBuf = std::make_unique<wchar_t[]>(largeBufSize);
}

that way the buffer will be increased in size only if necessary, and since the buf is static it will be reused.

dansyl1 commented 4 months ago

Adjusted fix for long path names based on feedback received 1) Long path names were not correctly watched 2) Sometimes crashed due to buffer overrun. Dynamically-allocated buffer is now used. Ready for final code review and merge.