From f08930c9c9ce91007c2f576c25d3f874abb96a00 Mon Sep 17 00:00:00 2001 From: Adrian Kummerlaender Date: Sat, 6 Feb 2016 22:20:46 +0100 Subject: Preserve pre-change file content as tracking state Relying on `diff` for storing the pre-change content did not work out as it opens the file descriptors to soon and only reads the first block of pre-change file content until the remaining content is actually required. This obviously led to problems when tracking actually changing files. --- src/tracking/change_tracker.cc | 66 ++++++++++++++++++++++-------------------- src/tracking/change_tracker.h | 7 ++--- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/tracking/change_tracker.cc b/src/tracking/change_tracker.cc index ca07214..9416576 100644 --- a/src/tracking/change_tracker.cc +++ b/src/tracking/change_tracker.cc @@ -8,7 +8,7 @@ namespace { // constants for increasing pair access readability constexpr unsigned int EMPLACE_SUCCESS = 1; constexpr unsigned int FILE_NAME = 0; -constexpr unsigned int DIFF_PROCESS = 1; +constexpr unsigned int FILE_CONTENT = 1; boost::process::context getDefaultContext() { boost::process::context context; @@ -20,6 +20,15 @@ boost::process::context getDefaultContext() { return context; } +// Build `diff` command to be used to display the tracked changes in a +// human readable fashion. +// +// As the original file contents are read by this library and forwarded +// to `diff` via its standard input the command has to be structured as +// follows: +// +// diff -u --label $file_path - $file_path +// std::string getDiffCommand( const std::string& diff_cmd, const std::string& full_path) { return diff_cmd + " --label " + full_path + " - " + full_path; @@ -39,11 +48,19 @@ ChangeTracker::ChangeTracker(utility::Logger* logger): ChangeTracker::~ChangeTracker() { for ( auto&& tracked : this->children_ ) { - std::get(tracked)->get_stdin().close(); + boost::process::child diffProcess{ + boost::process::launch_shell( + getDiffCommand(this->diff_cmd_, std::get(tracked)), + getDefaultContext() + ) + }; - this->logger_->forward(std::get(tracked)->get_stdout()); + diffProcess.get_stdin() << std::get(tracked)->rdbuf(); + diffProcess.get_stdin().close(); - std::get(tracked)->wait(); + this->logger_->forward(diffProcess.get_stdout()); + + diffProcess.wait(); } } @@ -55,27 +72,18 @@ bool ChangeTracker::is_tracked(const std::string& file_path) const { // Begins tracking changes to a file reachable by a given path // -// The actual tracking is performed by a `diff` instance that is -// spawned by this method and managed by this class. -// As `diff` is called as a new subprocess and because there is no -// straight forward way for checking / enforcing if it has already -// completed reading the initial contents of the file the command -// is structured in the following sequence: -// -// diff -p - $file_path -// -// This means that reading the final file contents is delegated to -// `diff` while the initial file contents are read by this method -// and written to `diff`'s standard input. +// _Tracking_ consists of adding the full file path to the `children_` +// map and reading the full pre-change file contents into the mapped +// `std::stringstream` instance. // -// If the `-` and `$file_path` arguments were exchanged there would -// be no way to: -// - be sure the initial contents are read before the changing -// syscall that triggered this tracking in the first place is -// performed -// - be sure that the initial file contents are read completly -// as `diff` seemingly only reads the first block of the first -// file provided if the second file argument is standard input +// This seems to be the most workable of various tested approaches +// to providing `diff` with proper input in all circumstances. Leaving +// the intermediate preservation of the original file content to +// `diff` sadly failed randomly as it is _too intelligent_ in actually +// reading the file from permanent storage. e.g. it opens all relevant +// file descriptors at launch and only reads the first block of the +// file contents until more is required. This leads to problems when +// the tracked file is modified after `diff` has been spawned. // bool ChangeTracker::track(const std::string& file_path) { const std::string full_file_path{ @@ -86,12 +94,7 @@ bool ChangeTracker::track(const std::string& file_path) { auto result = this->children_.emplace( full_file_path, - std::make_unique( - boost::process::launch_shell( - getDiffCommand(this->diff_cmd_, full_file_path), - getDefaultContext() - ) - ) + std::make_unique() ); guard.unlock(); @@ -102,7 +105,8 @@ bool ChangeTracker::track(const std::string& file_path) { ); if ( file.is_open() ) { - std::get(*result.first)->get_stdin() << file.rdbuf(); + *std::get(*result.first) << file.rdbuf(); + std::get(*result.first)->sync(); } return true; diff --git a/src/tracking/change_tracker.h b/src/tracking/change_tracker.h index e694b56..05f4021 100644 --- a/src/tracking/change_tracker.h +++ b/src/tracking/change_tracker.h @@ -1,10 +1,9 @@ #ifndef CHANGE_SRC_TRACKING_CHANGE_TRACKER_H_ #define CHANGE_SRC_TRACKING_CHANGE_TRACKER_H_ -#include #include - -#include +#include +#include #include "utility/logger.h" @@ -26,7 +25,7 @@ class ChangeTracker { std::mutex write_mutex_; std::unordered_map< - std::string, std::unique_ptr + std::string, std::unique_ptr > children_; }; -- cgit v1.2.3