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

LogCleaner: Fix relative paths and add a new test #754

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

aesophor
Copy link
Contributor

@aesophor aesophor commented Dec 7, 2021

Overview

Currently, if the user sets the log destination using a relative path (e.g., "testdir/"), then the log cleaner will scan "." for logs instead of "testdir/". This PR fixes the problem and adds a new test.

Root Cause

  • The second parameter pos of string::find_last_of indicates the position at which the search is to finish.
  • We should pass string::npos instead of 0 at line 1333.

glog/src/logging.cc

Lines 1333 to 1341 in 3362cc6

size_t pos = base_filename.find_last_of(possible_dir_delim, 0,
sizeof(possible_dir_delim));
if (pos != string::npos) {
string dir = base_filename.substr(0, pos + 1);
dirs.push_back(dir);
} else {
dirs.push_back(".");
}
}

Fix

  • Pass string::npos instead of 0 at line 1333.
  • A new test is added (modified from src/cleanup_with_prefix_unittest.cc)
    • In ${TEST_DIR}, create a subdirectory named "test_subdir"
    • After executing the unit test, it expects a single log file in ${TEST_DIR}/test_subdir
    • In ${TEST_DIR}, remove the "test_subdir" directory

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #754 (b791102) into master (ee6faf1) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
+ Coverage   72.65%   72.74%   +0.09%     
==========================================
  Files          17       17              
  Lines        3211     3211              
==========================================
+ Hits         2333     2336       +3     
+ Misses        878      875       -3     
Impacted Files Coverage Δ
src/logging.cc 73.91% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee6faf1...b791102. Read the comment docs.

@aesophor aesophor force-pushed the fix-relative-paths branch 2 times, most recently from 387bca3 to 9b4df1c Compare December 7, 2021 05:48
@sergiud
Copy link
Collaborator

sergiud commented Dec 7, 2021

[...] the log cleaner will not behave as intended.

What does this mean exactly, i.e., what is the intended behavior?

src/logging.cc Outdated Show resolved Hide resolved
@aesophor
Copy link
Contributor Author

aesophor commented Dec 7, 2021

[...] the log cleaner will not behave as intended.

What does this mean exactly, i.e., what is the intended behavior?

I've updated the description of this PR.

if the user sets the log destination using a relative path (e.g., "testdir/"), then the log cleaner will scan "." for logs instead of "testdir/"

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also rebase onto current master and squash all the commits?

CMakeLists.txt Outdated Show resolved Hide resolved
@aesophor
Copy link
Contributor Author

aesophor commented Dec 7, 2021

I've squashed all the commits into one, and renamed cleanup_with_prefix to cleanup_with_absolute_prefix. Thanks!

@sergiud sergiud added the bug label Dec 9, 2021
@sergiud sergiud added this to the 0.6 milestone Dec 9, 2021
@sergiud
Copy link
Collaborator

sergiud commented Dec 9, 2021

Thanks!

@sergiud sergiud merged commit ccbda2d into google:master Dec 9, 2021
@aesophor aesophor deleted the fix-relative-paths branch December 12, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants