Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

aesophor
Copy link
Contributor

@aesophor aesophor commented Dec 1, 2019

Previously log cleaner could be broken if users use SetLogDestination() to set a custom base_filename_, as discussed here: #432 (comment)

This PR fixes this bug by checking if a log's filename starts with base_filename_.

@aesophor aesophor changed the title src/logging.cc: encapsulate log cleaner src/logging.cc: encapsulate log cleaner, match logfile with base_filename Dec 1, 2019
@aesophor aesophor force-pushed the encapsulate-log-cleaner branch from 1b56ad7 to 7764e4a Compare December 1, 2019 14:06
@sergiud
Copy link
Contributor

sergiud commented Dec 3, 2019

@cswuyg Could you please review the changes?

@aesophor
Copy link
Contributor Author

aesophor commented Dec 8, 2019

@sergiud I've been using this patch in my own projects for a week and it seems to work well. Tested on both Linux and Windows.

@aesophor
Copy link
Contributor Author

aesophor commented Dec 8, 2019

Please don't merge it yet, I've found a potential bug:

Currently I'm checking if a file is a glog's logfile by checking if its filename starts with base_filename_, but this might also delete non-glog logfiles. I should make sure the entire filename matches the format of a glog's filename.

@aesophor aesophor force-pushed the encapsulate-log-cleaner branch from b27370a to 36fa99b Compare December 9, 2019 00:48
@aesophor aesophor changed the title src/logging.cc: encapsulate log cleaner, match logfile with base_filename src/logging.cc: encapsulate log cleaner, match logfile with <base_filename_><date>-<time>.<pid> Dec 9, 2019
@aesophor
Copy link
Contributor Author

aesophor commented Dec 9, 2019

@sergiud I've revised the patch and re-tested it. LogCleaner::IsLogFromCurrentProject() will now check if the given filename matches the format <base_filename><YYYYMMDD>-<HHMMSS>.<pid>.

@sergiud
Copy link
Contributor

sergiud commented Sep 29, 2020

Please resolve the conflict.

@aesophor aesophor mentioned this pull request Oct 1, 2020
@aesophor
Copy link
Contributor Author

aesophor commented Oct 3, 2020

@sergiud I've resolved the conflict. Thanks!

@sergiud sergiud removed the incomplete label Oct 3, 2020
@sergiud sergiud merged commit c8f8135 into google:master Oct 3, 2020
@sergiud sergiud added this to the 0.5 milestone Mar 30, 2021
@sergiud sergiud mentioned this pull request May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants