From 9ca0fd52a2eb55bee6e218975b4f44c6a4156315 Mon Sep 17 00:00:00 2001
From: isanae <14251494+isanae@users.noreply.github.com>
Date: Fri, 27 Nov 2020 05:26:51 -0500
Subject: [PATCH] comments, some refactoring in patcher and msbuild

---
 src/core/op.h                |   5 +-
 src/tasks/python.cpp         |   2 +-
 src/tools/git.cpp            |  36 +++++---
 src/tools/git.h              |   2 +-
 src/tools/jom.cpp            |  24 ++++--
 src/tools/msbuild.cpp        | 100 ++++++++++-----------
 src/tools/msbuild.h          |  58 +++++++++++--
 src/tools/patcher.cpp        | 162 ++++++++++++++++++-----------------
 src/tools/process_runner.cpp |  18 +++-
 src/tools/python.cpp         |  33 +++----
 src/tools/tools.cpp          |  30 ++++---
 src/tools/tools.h            |   4 +
 12 files changed, 289 insertions(+), 185 deletions(-)

diff --git a/src/core/op.h b/src/core/op.h
index a996696..f094ac4 100644
--- a/src/core/op.h
+++ b/src/core/op.h
@@ -51,9 +51,12 @@ void create_directories(const context& cx, const fs::path& p, flags f=noflags);
 
 // deletes the given directory, recursive
 //
-// if deletion fails because of access denied, attemps to remove the readonly
+// if deletion fails because of access denied, attempts to remove the readonly
 // flag on all files and tries again; this happens with some archives like 7z
 //
+// if the directory is controlled by git, prefer git_wrap::delete_directory(),
+// which checks for uncommitted changes before
+//
 void delete_directory(const context& cx, const fs::path& p, flags f=noflags);
 
 // deletes the given file
diff --git a/src/tasks/python.cpp b/src/tasks/python.cpp
index 013b873..1561262 100644
--- a/src/tasks/python.cpp
+++ b/src/tasks/python.cpp
@@ -245,7 +245,7 @@ msbuild python::create_msbuild_tool(msbuild::ops o)
 		.targets({
 			"python", "pythonw", "python3dll", "select", "pyexpat",
 			"unicodedata", "_queue", "_bz2", "_ssl", "_overlapped"})
-		.parameters({
+		.properties({
 			"bz2Dir=" + path_to_utf8(bzip2::source_path()),
 			"zlibDir=" + path_to_utf8(zlib::source_path()),
 			"opensslIncludeDir=" + path_to_utf8(openssl::include_path()),
diff --git a/src/tools/git.cpp b/src/tools/git.cpp
index 80eb9a5..cc30ef1 100644
--- a/src/tools/git.cpp
+++ b/src/tools/git.cpp
@@ -7,8 +7,8 @@
 namespace mob::details
 {
 
-const std::string default_github_url_pattern = "git@github.com:{}/{}";
-
+// calls f() with each .ts file in the root, recursive
+//
 template <class F>
 void for_each_ts(const fs::path& root, F&& f)
 {
@@ -26,22 +26,28 @@ void for_each_ts(const fs::path& root, F&& f)
 	}
 }
 
+// returns a github url for the given org and git file
+//
 std::string make_url(
 	const std::string& org, const std::string& git_file,
 	const std::string& url_pattern)
 {
+	const std::string default_github_url_pattern = "git@github.com:{}/{}";
+
 	const std::string pattern = url_pattern.empty() ?
-		details::default_github_url_pattern : url_pattern;
+		default_github_url_pattern : url_pattern;
 
 	return fmt::format(pattern, org, git_file);
 }
 
 
+// creates a basic git process, used by all the functions below
+//
 [[nodiscard]] process make_process()
 {
 	static env e = this_env::get()
-		.set("GCM_INTERACTIVE", "never")
-		.set("GIT_TERMINAL_PROMPT", "0");
+		.set("GCM_INTERACTIVE", "never")  // disables credentials UI
+		.set("GIT_TERMINAL_PROMPT", "0"); // disables all prompts
 
 	return std::move(process()
 		.binary(git_wrap::binary())
@@ -281,7 +287,7 @@ std::string make_url(
 		.cwd(root);
 }
 
-[[nodiscard]] process git_file(const fs::path& root)
+[[nodiscard]] process remote_url(const fs::path& root)
 {
 	return make_process()
 		.stdout_flags(process::keep_in_string)
@@ -416,15 +422,17 @@ void git_wrap::revert_ts()
 	{
 		const auto rp = fs::relative(p, root_);
 
-		if (!is_tracked(rp))
+		if (is_tracked(rp))
+		{
+			run(details::revert(root_, p));
+		}
+		else
 		{
 			cx().debug(context::generic,
 				"won't try to revert ts file '{}', not tracked", rp);
 
 			return;
 		}
-
-		run(details::revert(root_, p));
 	});
 }
 
@@ -496,8 +504,10 @@ void git_wrap::add_submodule(
 
 std::string git_wrap::git_file()
 {
-	auto p = details::git_file(root_);
+	auto p = details::remote_url(root_);
 	run(p);
+
+	// contains the remote url, get the last component
 	const std::string out = p.stdout_string();
 
 	const auto last_slash = out.find_last_of("/");
@@ -522,6 +532,9 @@ void git_wrap::delete_directory(const context& cx, const fs::path& dir)
 {
 	git_wrap g(dir);
 
+	// make sure there are no uncommitted or stashed changes to avoid losing
+	// data
+
 	if (!conf().global().get<bool>("ignore_uncommitted"))
 	{
 		if (g.has_uncommitted_changes())
@@ -749,8 +762,7 @@ static std::unique_ptr<git_submodule_adder> g_sa_instance;
 static std::mutex g_sa_instance_mutex;
 
 git_submodule_adder::git_submodule_adder() :
-	instrumentable("submodule_adder",
-		{"add_submodule_wait", "add_submodule"}),
+	instrumentable("submodule_adder", {"add_submodule_wait", "add_submodule"}),
 	cx_("submodule_adder"), quit_(false)
 {
 	run();
diff --git a/src/tools/git.h b/src/tools/git.h
index 00ff6cd..d997f34 100644
--- a/src/tools/git.h
+++ b/src/tools/git.h
@@ -81,7 +81,7 @@ public:
 	//
 	bool has_remote(const std::string& name);
 
-	// adds a remote from github
+	// adds a remote from github, no-op if it already exists
 	//
 	// remote_name:  name of the new remote
 	//
diff --git a/src/tools/jom.cpp b/src/tools/jom.cpp
index cc82d0a..75a40d7 100644
--- a/src/tools/jom.cpp
+++ b/src/tools/jom.cpp
@@ -55,9 +55,15 @@ void jom::do_run()
 {
 	process p;
 
+	// jom doesn't handle sigint well, it just continues, so kill it on
+	// interruption
 	auto pflags = process::terminate_on_interrupt;
+
 	if (flags_ & allow_failure)
 	{
+		// tasks will set allow_failure for the first couple of runs of jom,
+		// which often fails because of the /J multi-process flag, so don't log
+		// errors in that case
 		p.stderr_level(context::level::trace);
 		pflags |= process::allow_failure;
 	}
@@ -67,19 +73,21 @@ void jom::do_run()
 		.cwd(cwd_)
 		.stderr_filter([](process::filter& f)
 		{
+			// initial log line, can't get rid of it, /L or /NOLOGO don't seem
+			// to work
 			if (f.line.find("empower your cores") != std::string::npos)
 				f.lv = context::level::trace;
 		})
-		.arg("/C", process::log_quiet)
-		.arg("/S", process::log_quiet)
-		.arg("/L", process::log_quiet)
-		.arg("/D", process::log_dump)
-		.arg("/P", process::log_dump)
-		.arg("/W", process::log_dump)
-		.arg("/K");
+		.arg("/C", process::log_quiet)  // silent
+		.arg("/S", process::log_quiet)  // silent
+		.arg("/L", process::log_quiet)  // silent, jom likes to spew crap
+		.arg("/D", process::log_dump)   // verbose stuff
+		.arg("/P", process::log_dump)	// verbose stuff
+		.arg("/W", process::log_dump)	// verbose stuff
+		.arg("/K");                     // don't stop on errors
 
 	if (flags_ & single_job)
-		p.arg("/J", "1");
+		p.arg("/J", "1");               // single-process
 
 	for (auto&& def : def_)
 		p.arg(def);
diff --git a/src/tools/msbuild.cpp b/src/tools/msbuild.cpp
index 68aa418..83ae9a3 100644
--- a/src/tools/msbuild.cpp
+++ b/src/tools/msbuild.cpp
@@ -6,16 +6,6 @@
 namespace mob
 {
 
-void error_filter(process::filter& f)
-{
-	// ": error C2065"
-	// ": error MSB1009"
-	static std::regex re(": error [A-Z]");
-	if (std::regex_search(f.line.begin(), f.line.end(), re))
-		f.lv = context::level::error;
-}
-
-
 msbuild::msbuild(ops o) :
 	basic_process_runner("msbuild"),
 	op_(o), config_("Release"), arch_(arch::def), flags_(noflags)
@@ -39,9 +29,9 @@ msbuild& msbuild::targets(const std::vector<std::string>& names)
 	return *this;
 }
 
-msbuild& msbuild::parameters(const std::vector<std::string>& params)
+msbuild& msbuild::properties(const std::vector<std::string>& props)
 {
-	params_ = params;
+	props_ = props;
 	return *this;
 }
 
@@ -105,51 +95,59 @@ void msbuild::do_run()
 
 void msbuild::do_build()
 {
-	do_run(targets_);
+	run_for_targets(targets_);
 }
 
-void msbuild::do_run(const std::vector<std::string>& targets)
+std::string msbuild::platform_property() const
+{
+	if (!platform_.empty())
+		return platform_;
+
+	switch (arch_)
+	{
+		case arch::x86:
+			return "Win32";
+
+		case arch::x64:
+			return "x64";
+
+		case arch::dont_care:
+		default:
+			cx().bail_out(context::generic, "msbuild::do_run(): bad arch");
+	}
+}
+
+void msbuild::run_for_targets(const std::vector<std::string>& targets)
 {
 	// 14.2 to v142
 	const auto toolset = "v" + replace_all(vs::toolset(), ".", "");
 
-	std::string plat;
-
-	if (platform_.empty())
-	{
-		switch (arch_)
-		{
-			case arch::x86:
-				plat = "Win32";
-				break;
-
-			case arch::x64:
-				plat = "x64";
-				break;
-
-			case arch::dont_care:
-			default:
-				cx().bail_out(context::generic, "msbuild::do_run(): bad arch");
-		}
-	}
-	else
-	{
-		plat = platform_;
-	}
-
-	auto pflags = process::noflags;
 	process p;
 
 	if (is_set(flags_, allow_failure))
 	{
-		p.stderr_level(context::level::trace);
-		pflags |= process::allow_failure;
+		// make sure errors are not displayed and mob doesn't bail out
+		p
+			.stderr_level(context::level::trace)
+			.flags(process::allow_failure);
 	}
 	else
 	{
-		p.stdout_filter([&](auto& f){ error_filter(f); });
+		p.stdout_filter([&](auto& f)
+		{
+			// ": error C2065"
+			// ": error MSB1009"
+			static std::regex re(": error [A-Z]");
+
+			// ghetto attempt at showing errors on the console, since stdout
+			// has all the compiler output
+			if (std::regex_search(f.line.begin(), f.line.end(), re))
+				f.lv = context::level::error;
+		});
 	}
 
+	// msbuild will use the console's encoding, so by invoking `chcp 65001`
+	// (the utf8 "codepage"), stdout and stderr are utf8
 	p
 		.binary(binary())
 		.chcp(65001)
@@ -159,6 +157,7 @@ void msbuild::do_run(const std::vector<std::string>& targets)
 
 	if (!is_set(flags_, single_job))
 	{
+		// multi-process
 		p
 			.arg("-maxCpuCount")
 			.arg("-property:UseMultiToolTask=true")
@@ -169,25 +168,28 @@ void msbuild::do_run(const std::vector<std::string>& targets)
 		.arg("-property:Configuration=", config_, process::quote)
 		.arg("-property:PlatformToolset=" + toolset)
 		.arg("-property:WindowsTargetPlatformVersion=" + vs::sdk())
-		.arg("-property:Platform=", plat, process::quote)
-		.arg("-property:RunCodeAnalysis=false")
+		.arg("-property:Platform=", platform_property(), process::quote)
 		.arg("-verbosity:minimal", process::log_quiet)
 		.arg("-consoleLoggerParameters:ErrorsOnly", process::log_quiet);
 
+	// some projects have code analysis turned on and can fail on preview
+	// versions, make sure it's never run
+	p.arg("-property:RunCodeAnalysis=false");
+
+	// targets
 	if (!targets.empty())
 		p.arg("-target:" + mob::join(targets, ";"));
 
-	for (auto&& param : params_)
-		p.arg("-property:" + param);
+	// properties
+	for (auto&& prop : props_)
+		p.arg("-property:" + prop);
 
 	env e = env::vs(arch_);
-
 	for (auto&& path : prepend_path_)
 		e.prepend_path(path);
 
 	p
 		.arg(sln_)
-		.flags(pflags)
 		.cwd(sln_.parent_path())
 		.env(e);
 
@@ -197,7 +199,7 @@ void msbuild::do_run(const std::vector<std::string>& targets)
 void msbuild::do_clean()
 {
 	flags_ |= allow_failure;
-	do_run(map(targets_, [&](auto&& t){ return t + ":Clean"; }));
+	run_for_targets(map(targets_, [&](auto&& t){ return t + ":Clean"; }));
 }
 
 }	// namespace
diff --git a/src/tools/msbuild.h b/src/tools/msbuild.h
index b317b11..4067082 100644
--- a/src/tools/msbuild.h
+++ b/src/tools/msbuild.h
@@ -3,9 +3,18 @@
 namespace mob
 {
 
+// tool that runs `msbuild`, see the jom tool for explanations on errors with
+// parallel builds and the single_job/allow_failure flags, the same thing
+// happens with msbuild
+//
 class msbuild : public basic_process_runner
 {
 public:
+	// path to the msbuild binary
+	//
+	static fs::path binary();
+
+
 	enum flags_t
 	{
 		noflags        = 0x00,
@@ -13,6 +22,8 @@ public:
 		allow_failure  = 0x02
 	};
 
+	// what run() should do
+	//
 	enum ops
 	{
 		build = 1,
@@ -21,17 +32,43 @@ public:
 
 	msbuild(ops o=build);
 
-	static fs::path binary();
-
+	// .sln file
+	//
 	msbuild& solution(const fs::path& sln);
+
+	// adds a "-target:string` for each string given
+	//
 	msbuild& targets(const std::vector<std::string>& names);
-	msbuild& parameters(const std::vector<std::string>& params);
+
+	// adds a "-property:string" for every string given
+	//
+	msbuild& properties(const std::vector<std::string>& props);
+
+	// sets "-property:Configuration=s"
+	//
 	msbuild& config(const std::string& s);
+
+	// sets "-property:Platform=s"; if not set, uses architecture() to figure
+	// it out
+	//
 	msbuild& platform(const std::string& s);
+
+	// used by
+	//  1) the vsvars environment variables
+	//  2) the "-property:Platform" property if platform() wasn't called
+	//
 	msbuild& architecture(arch a);
+
+	// flags
 	msbuild& flags(flags_t f);
+
+	// can be called multiple times, the given paths will be prepended to PATH
+	// before invoking msbuild
+	//
 	msbuild& prepend_path(const fs::path& p);
 
+	// exit code
+	//
 	int result() const;
 
 protected:
@@ -41,16 +78,27 @@ private:
 	ops op_;
 	fs::path sln_;
 	std::vector<std::string> targets_;
-	std::vector<std::string> params_;
+	std::vector<std::string> props_;
 	std::string config_;
 	std::string platform_;
 	arch arch_;
 	flags_t flags_;
 	std::vector<fs::path> prepend_path_;
 
+	// runs msbuild with ":Clean" for each target given in targets(), giving
+	// something like "-target:modorganizer:Clean"
+	//
 	void do_clean();
+
+	// runs msbuild
+	//
 	void do_build();
-	void do_run(const std::vector<std::string>& targets);
+
+	// called by both do_clean() and do_build
+	//
+	void run_for_targets(const std::vector<std::string>& targets);
+
+	std::string platform_property() const;
 };
 
 MOB_ENUM_OPERATORS(msbuild::flags_t);
diff --git a/src/tools/patcher.cpp b/src/tools/patcher.cpp
index 0f8fd5e..8f49c13 100644
--- a/src/tools/patcher.cpp
+++ b/src/tools/patcher.cpp
@@ -37,72 +37,80 @@ patcher& patcher::root(const fs::path& dir)
 
 void patcher::do_run()
 {
-	const fs::path patches_root = conf().path().patches() / task_;
+	// patches should be in mob/patches/task-name, but not all tasks need
+	// patches
+	const fs::path root = conf().path().patches() / task_;
 
-	if (!fs::exists(patches_root))
+	if (!fs::exists(root))
 	{
 		cx().trace(context::generic,
-			"patch directory {} doesn't exist, assuming no patches",
-			patches_root);
+			"patch directory {} doesn't exist, assuming no patches", root);
 
 		return;
 	}
 
-	if (file_.empty())
-	{
-		const fs::path patches =
-			patches_root / (prebuilt_ ? "prebuilt" : "sources");
 
-		cx().trace(context::generic, "looking for patches in {}", patches);
-
-		if (!fs::exists(patches))
-		{
-			cx().trace(context::generic,
-				"patch directory {} doesn't exist, assuming no patches",
-				patches);
-
-			return;
-		}
-
-		for (auto e : fs::directory_iterator(patches))
-		{
-			if (!e.is_regular_file())
-			{
-				cx().trace(context::generic,
-					"skipping {}, not a file", e.path());
-
-				continue;
-			}
-
-			const auto p = e.path();
-
-			if (p.extension() == ".manual_patch")
-			{
-				cx().trace(context::generic,
-					"skipping manual patch {}", e.path());
-
-				continue;
-			}
-			else if (p.extension() != ".patch")
-			{
-				cx().warning(context::generic,
-					"file with unknown extension {}", p);
-
-				continue;
-			}
-
-			do_patch(p);
-		}
-	}
-	else
+	if (!file_.empty())
 	{
+		// patcher tool is being run for a manual patch
 		cx().trace(context::generic, "doing manual patch from {}", file_);
-		do_patch(patches_root / file_);
+		do_patch(root / file_);
+		return;
+	}
+
+	// patcher tool is being by the task for auto patching, figure out the
+	// directory to use depending on whether it's a prebuilt
+	const fs::path patches = root / (prebuilt_ ? "prebuilt" : "sources");
+	cx().trace(context::generic, "looking for patches in {}", patches);
+
+	if (!fs::exists(patches))
+	{
+		cx().trace(context::generic,
+			"patch directory {} doesn't exist, assuming no patches",
+			patches);
+
+		return;
+	}
+
+
+	// for each path file
+	for (auto e : fs::directory_iterator(patches))
+	{
+		if (!e.is_regular_file())
+		{
+			cx().trace(context::generic, "skipping {}, not a file", e.path());
+			continue;
+		}
+
+		const auto p = e.path();
+
+		if (p.extension() == ".manual_patch")
+		{
+			cx().trace(context::generic, "skipping manual patch {}", e.path());
+			continue;
+		}
+		else if (p.extension() != ".patch")
+		{
+			cx().warning(context::generic, "file with unknown extension {}", p);
+			continue;
+		}
+
+		do_patch(p);
 	}
 }
 
-void patcher::do_patch(const fs::path& patch_file)
+void patcher::do_patch(const fs::path& patch)
 {
+	// there's no way to figure out if patch failure is because 1) the patch
+	// file is incorrect, or 2) the patch has already been applied
+	//
+	// an incorrect patch file would probably mean that the source has changed
+	// and the patch must be updated or removed if it's not required anymore
+	//
+	// so patching is a two step process: check if the patch has already been
+	// applied, and apply it if it hasn't
+
+	// use by both the check and apply processes
 	const auto base = process()
 		.binary(binary())
 		.arg("--read-only", "ignore")
@@ -110,50 +118,50 @@ void patcher::do_patch(const fs::path& patch_file)
 		.arg("--directory", root_)
 		.arg("--quiet", process::log_quiet);
 
+	// process to reverse the path: the only way to check if a patch has been
+	// applied is actually to try to reverse it and check if there was an error
+	//
+	// this uses --dry-run because if the file was already patched, it shouldn't
+	// actually be reversed
 	auto check = process(base)
 		.flags(process::allow_failure)
 		.arg("--dry-run")
-		.arg("--force")
-		.arg("--reverse")
-		.arg("--input", patch_file);
+		.arg("--force")    // no prompts
+		.arg("--reverse")  // swaps old and new files
+		.arg("--input", patch);
 
+	// process to apply the patch
 	auto apply = process(base)
-		.arg("--forward")
-		.arg("--batch")
-		.arg("--input", patch_file);
+		.arg("--forward")  // don't try to reverse the patch if it fails
+		.arg("--batch")    // no prompts
+		.arg("--input", patch);
 
-	cx().trace(context::generic, "trying to patch using {}", patch_file);
+	cx().trace(context::generic, "trying to patch using {}", patch);
 
 	{
-		// check
-
-		cx().trace(context::generic,
-			"checking if already patched");
-
+		// check, returns 0 when the patch would have been reversed correctly,
+		// 1 if not, anything else on error
+		cx().trace(context::generic, "checking if already patched");
 		const auto ret = execute_and_join(check);
 
 		if (ret == 0)
 		{
-			cx().trace(context::generic,
-				"patch {} already applied", patch_file);
-
+			// reversing the patch would succeed, so the patch has already been
+			// applied
+			cx().trace(context::generic, "patch {} already applied", patch);
 			return;
 		}
-		else if (ret == 1)
-		{
-			cx().trace(context::generic,
-				"looks like the patch is needed");
-		}
-		else
-		{
+
+		// anything other than 0 or 1 is a hard error
+		if (ret != 1)
 			cx().bail_out(context::generic, "patch returned {}", ret);
-		}
+
+		cx().trace(context::generic, "looks like the patch is needed");
 	}
 
 	{
 		// apply
-
-		cx().trace(context::generic, "applying patch {}", patch_file);
+		cx().trace(context::generic, "applying patch {}", patch);
 		execute_and_join(apply);
 	}
 }
diff --git a/src/tools/process_runner.cpp b/src/tools/process_runner.cpp
index daeeae8..fe37ac5 100644
--- a/src/tools/process_runner.cpp
+++ b/src/tools/process_runner.cpp
@@ -11,8 +11,8 @@ basic_process_runner::basic_process_runner(std::string name)
 {
 }
 
+// anchors
 basic_process_runner::basic_process_runner(basic_process_runner&& r) = default;
-
 basic_process_runner::~basic_process_runner() = default;
 
 void basic_process_runner::do_interrupt()
@@ -23,10 +23,18 @@ void basic_process_runner::do_interrupt()
 
 int basic_process_runner::execute_and_join(process& p)
 {
+	// remember the process for do_interrupt()
 	p_ = &p;
+
+	// use the process' name for this tool
 	set_name(p.name());
+
+	// use this tool's log context for the process
 	p.set_context(&cx());
+
+	// run, remember the code because the process object might be destroyed
 	code_ = p.run_and_join();
+
 	return code_;
 }
 
@@ -41,15 +49,19 @@ process_runner::process_runner(process& p)
 {
 }
 
+// anchor
 process_runner::~process_runner() = default;
 
 void process_runner::do_run()
 {
+	// use the process' name for this tool
 	set_name(p_.name());
+
+	// use this tool's log context for the process
 	p_.set_context(&cx());
 
-	p_.run();
-	p_.join();
+	// run
+	p_.run_and_join();
 }
 
 int process_runner::result() const
diff --git a/src/tools/python.cpp b/src/tools/python.cpp
index ee18c8c..78ee309 100644
--- a/src/tools/python.cpp
+++ b/src/tools/python.cpp
@@ -25,6 +25,11 @@ python& python::arg(const std::string& s)
 
 void python::do_run()
 {
+	// python is a bit finicky for utf8, so:
+	//  1) chcp changes the codepage to utf8
+	//  2) `-X utf8` and the PYTHONUTF8 environment variable are set, which
+	//     is probably redundant
+
 	auto p = process()
 		.binary(tasks::python::python_exe())
 		.chcp(65001)
@@ -32,20 +37,20 @@ void python::do_run()
 		.stderr_encoding(encodings::utf8)
 		.stderr_filter([&](process::filter& f)
 		{
+			// filter out crap from setuptools
 			if (f.line.find("zip_safe flag not set") != std::string::npos)
 				f.lv = context::level::trace;
 			else if (f.line.find("module references __file__") != std::string::npos)
 				f.lv = context::level::trace;
 		})
-		.arg("-X", "utf8");
+		.arg("-X", "utf8");  // forces utf8
 
 	for (auto&& a : args_)
 		p.arg(a);
 
 	p
 		.cwd(root_)
-		.env(this_env::get()
-			.set("PYTHONUTF8", "1"));
+		.env(this_env::get().set("PYTHONUTF8", "1"));  // forces utf8
 
 	execute_and_join(p);
 }
@@ -98,17 +103,16 @@ void pip::do_run()
 void pip::do_ensure()
 {
 	// ensure
-	//
-	// this spits out two warnings about not being on PATH and suggests to add
-	// --no-warn-script-location, but that's not actually a valid command
-	// line parameter for `ensurepip` and it fails, unlike the `install`
-	// commands below
-	//
-	// so just filter it out
-
 	execute_and_join(process()
 		.stderr_filter([](auto&& f)
 		{
+			// this spits out two warnings about not being on PATH and suggests
+			// to add --no-warn-script-location, but that's not actually a valid
+			// parameter for `ensurepip` and it fails, unlike the `install`
+			// commands below
+			//
+			// so just filter it out
+
 			if (f.line.find("which is not on PATH") != -1)
 				f.lv = context::level::debug;
 			else if (f.line.find("Consider adding this directory"))
@@ -154,9 +158,7 @@ void pip::do_install()
 	else if (!file_.empty())
 		p.arg(file_);
 
-	p
-		.env(this_env::get()
-			.set("PYTHONUTF8", "1"));
+	p.env(this_env::get().set("PYTHONUTF8", "1"));
 
 	execute_and_join(p);
 }
@@ -175,8 +177,7 @@ void pip::do_download()
 		.arg("--no-deps")
 		.arg("-d", conf().path().cache())
 		.arg(package_ + "==" + version_)
-		.env(this_env::get()
-			.set("PYTHONUTF8", "1")));
+		.env(this_env::get().set("PYTHONUTF8", "1")));
 }
 
 }	// namespace
diff --git a/src/tools/tools.cpp b/src/tools/tools.cpp
index cec7d77..078fcd7 100644
--- a/src/tools/tools.cpp
+++ b/src/tools/tools.cpp
@@ -37,6 +37,7 @@ void tool::run(context& cx)
 {
 	cx_ = &cx;
 
+	// tell the context this tool is running, used for logs
 	cx_->set_tool(this);
 	guard g([&]{ cx_->set_tool(nullptr); });
 
@@ -45,12 +46,12 @@ void tool::run(context& cx)
 
 void tool::interrupt()
 {
-	if (!interrupted_)
-	{
-		cx().debug(context::interruption, "interrupting {}", name_);
-		interrupted_ = true;
-		do_interrupt();
-	}
+	if (interrupted_)
+		return;
+
+	cx().debug(context::interruption, "interrupting {}", name_);
+	interrupted_ = true;
+	do_interrupt();
 }
 
 void tool::do_interrupt()
@@ -173,6 +174,8 @@ void vs::do_run()
 
 void vs::do_upgrade()
 {
+	// assume the project is already upgraded if UpgradeLog.htm exists, because
+	// upgrading is slow even if it's not necessary
 	if (fs::exists(sln_.parent_path() / "UpgradeLog.htm"))
 	{
 		cx().debug(context::generic, "project already upgraded");
@@ -324,8 +327,7 @@ void transifex::do_config()
 		.arg("config")
 		.arg("mapping-remote")
 		.arg(url_)
-		.env(this_env::get()
-			.set("TX_TOKEN", key_))
+		.env(this_env::get().set("TX_TOKEN", key_))
 		.cwd(root_));
 }
 
@@ -341,8 +343,7 @@ void transifex::do_pull()
 		.arg("--parallel")
 		.arg("--no-interactive")
 		.arg("--minimum-perc", min_)
-		.env(this_env::get()
-			.set("TX_TOKEN", key_))
+		.env(this_env::get().set("TX_TOKEN", key_))
 		.cwd(root_);
 
 	if (force_)
@@ -391,7 +392,10 @@ fs::path lrelease::qm_file() const
 	if (sources_.empty())
 		cx().bail_out(context::generic, "lrelease: no sources");
 
+	// source files are something like "fr.ts", get "fr" and use the project
+	// name to make something like "modorganizer_fr.qm"
 	const auto lang = trim_copy(path_to_utf8(sources_[0].stem()));
+
 	if (lang.empty())
 	{
 		cx().bail_out(context::generic,
@@ -403,6 +407,7 @@ fs::path lrelease::qm_file() const
 
 void lrelease::do_run()
 {
+	// that's the output file
 	const auto qm = qm_file();
 
 	auto p = process()
@@ -417,11 +422,12 @@ void lrelease::do_run()
 		});
 
 
+	// input .ts files
 	for (auto&& s : sources_)
 		p.arg(s);
 
-	p
-		.arg("-qm", (out_ / qm));
+	// output .qm file
+	p.arg("-qm", (out_ / qm));
 
 	execute_and_join(p);
 }
diff --git a/src/tools/tools.h b/src/tools/tools.h
index 5ef76e0..5d99120 100644
--- a/src/tools/tools.h
+++ b/src/tools/tools.h
@@ -663,6 +663,8 @@ private:
 class pip : public basic_process_runner
 {
 public:
+	// what run() should do
+	//
 	enum ops
 	{
 		// installs pip if needed and updates it
@@ -734,6 +736,8 @@ public:
 	static fs::path binary();
 
 
+	// what run() should do
+	//
 	enum ops
 	{
 		// runs `ini`, initializes an empty directory to use transifex