From 51a7852aba245f784ccab0c887457e462a5283cd Mon Sep 17 00:00:00 2001
From: isanae <14251494+isanae@users.noreply.github.com>
Date: Fri, 27 Nov 2020 04:06:48 -0500
Subject: [PATCH] comments refactored do_download() into use_existing() and
 try_download()

---
 src/cmd/git.cpp          |   5 +-
 src/tools/downloader.cpp | 100 +++++++++-------
 src/tools/extractor.cpp  |  95 ++++++++++-----
 src/tools/git.cpp        |  44 +++----
 src/tools/git.h          | 253 ++++++++++++++++++++++++++++++++++++---
 src/tools/tools.h        |  13 +-
 6 files changed, 396 insertions(+), 114 deletions(-)

diff --git a/src/cmd/git.cpp b/src/cmd/git.cpp
index e36fefe..063ce9a 100644
--- a/src/cmd/git.cpp
+++ b/src/cmd/git.cpp
@@ -176,8 +176,11 @@ void git_command::do_set_remotes()
 void git_command::do_set_remotes(const fs::path& r)
 {
 	u8cout << "setting up " << path_to_utf8(r.filename()) << "\n";
+
 	git_wrap(r).set_credentials(username_, email_);
-	git_wrap(r).set_remote(username_, key_, nopush_, push_default_);
+
+	git_wrap(r).set_origin_and_upstream_remotes(
+		username_, key_, nopush_, push_default_);
 }
 
 void git_command::do_add_remote()
diff --git a/src/tools/downloader.cpp b/src/tools/downloader.cpp
index 51666ac..ca0c2da 100644
--- a/src/tools/downloader.cpp
+++ b/src/tools/downloader.cpp
@@ -60,51 +60,24 @@ void downloader::do_download()
 	dl_.reset(new curl_downloader(&cx()));
 
 	cx().trace(context::net, "looking for already downloaded files");
-
-	if (!file_.empty())
+	if (use_existing())
 	{
-		if (try_picking(file_))
-			return;
+		cx().trace(context::bypass, "using {}", file_);
+		return;
 	}
-	else
-	{
-		for (auto&& u : urls_)
-		{
-			const auto file = path_for_url(u);
-
-			if (try_picking(file))
-			{
-				file_ = file;
-				return;
-			}
-		}
-	}
-
 
 	cx().trace(context::net, "no cached downloads were found, will try:");
 	for (auto&& u : urls_)
 		cx().trace(context::net, "  . {}", u);
 
-
 	// try them in order
 	for (auto&& u : urls_)
 	{
-		if (file_.empty())
-			file_ = path_for_url(u);
-
-		cx().trace(context::net, "trying {} into {}", u, file_);
-
-		dl_->start(u, file_);
-		cx().trace(context::net, "waiting for download");
-		dl_->join();
-
-		if (dl_->ok())
+		if (try_download(u))
 		{
-			cx().trace(context::net, "file {} downloaded", file_);
+			// done
 			return;
 		}
-
-		cx().debug(context::net, "download failed");
 	}
 
 	if (interrupted())
@@ -117,15 +90,37 @@ void downloader::do_download()
 	cx().bail_out(context::net, "all urls failed to download");
 }
 
+bool downloader::try_download(const mob::url& u)
+{
+	// when file() wasn't called, the output file is created from the url
+	if (file_.empty())
+		file_ = path_for_url(u);
+
+	// downloading
+	cx().trace(context::net, "trying {} into {}", u, file_);
+	dl_->start(u, file_);
+
+	cx().trace(context::net, "waiting for download");
+	dl_->join();
+
+	if (dl_->ok())
+	{
+		// done
+		cx().trace(context::net, "file {} downloaded", file_);
+		return true;
+	}
+
+	cx().debug(context::net, "download failed");
+	return false;
+}
+
 void downloader::do_clean()
 {
-	if (!file_.empty())
-	{
-		cx().debug(context::redownload, "deleting {}", file_);
-		op::delete_file(cx(), file_, op::optional);
-	}
-	else
+	if (file_.empty())
 	{
+		// file() wasn't called, delete all the files that would be created
+		// depending on the urls given
+
 		for (auto&& u : urls_)
 		{
 			const auto file = path_for_url(u);
@@ -134,6 +129,12 @@ void downloader::do_clean()
 			op::delete_file(cx(), file, op::optional);
 		}
 	}
+	else
+	{
+		// delete the given output file
+		cx().debug(context::redownload, "deleting {}", file_);
+		op::delete_file(cx(), file_, op::optional);
+	}
 }
 
 void downloader::do_interrupt()
@@ -142,16 +143,28 @@ void downloader::do_interrupt()
 		dl_->interrupt();
 }
 
-bool downloader::try_picking(const fs::path& file)
+bool downloader::use_existing()
 {
-	if (fs::exists(file))
+	if (file_.empty())
 	{
-		cx().trace(context::bypass, "picking {}", file_);
-		return true;
+		// check if one of the files that would be created by a url exists
+		for (auto&& u : urls_)
+		{
+			const auto file = path_for_url(u);
+
+			if (fs::exists(file))
+			{
+				// take it
+				file_ = file;
+				return true;
+			}
+		}
 	}
 	else
 	{
-		cx().trace(context::net, "no {}", file);
+		// file() was called, check if it exists
+		if (fs::exists(file_))
+			return true;
 	}
 
 	return false;
@@ -183,6 +196,7 @@ fs::path downloader::path_for_url(const mob::url& u) const
 		filename = u.filename();
 	}
 
+	// downloaded files go in the cache, typically build/downloads/
 	return conf().path().cache() / filename;
 }
 
diff --git a/src/tools/extractor.cpp b/src/tools/extractor.cpp
index fc9a051..b47f931 100644
--- a/src/tools/extractor.cpp
+++ b/src/tools/extractor.cpp
@@ -31,8 +31,10 @@ void extractor::do_run()
 {
 	interruption_file ifile(cx(), where_, "extractor");
 
+	// check interruption file from last run
 	if (ifile.exists())
 	{
+		// resume the extraction, will overwrite
 		cx().debug(context::generic,
 			"previous extraction was interrupted; resuming");
 	}
@@ -40,11 +42,14 @@ void extractor::do_run()
 	{
 		if (conf().global().reextract())
 		{
+			// output already exists, no interruption file, but the user wants
+			// to re-extract
 			cx().debug(context::reextract, "deleting {}", where_);
 			op::delete_directory(cx(), where_, op::optional);
 		}
 		else
 		{
+			// output already exists, no interruption file, assume it's fine
 			cx().debug(context::bypass, "directory {} already exists", where_);
 			return;
 		}
@@ -52,14 +57,23 @@ void extractor::do_run()
 
 	cx().debug(context::generic, "extracting {} into {}", file_, where_);
 
+	op::create_directories(cx(), where_);
+
+	// will be left on disk on crashes or interruptions
 	ifile.create();
 
-	op::create_directories(cx(), where_);
+	// deletes the directory in the destructor if in case of hard failure, but
+	// not interruptions so extraction is resumed later
 	directory_deleter delete_output(cx(), where_);
 
-	// the -spe from 7z is supposed to figure out if there's a folder in the
-	// archive with the same name as the target and extract its content to
-	// avoid duplicating the folder
+	// some archives have a top-level directory, others have files directly in
+	// it, and it sucks to have special cases that know about individual
+	// third parties, so this tries to figure out whether to move the files
+	// after extraction
+	//
+	// now, the -spe flag from 7z is supposed to figure out if there's a folder
+	// in the archive with the same name as the target and extract its content
+	// to avoid duplicating the folder
 	//
 	// however, it fails miserably if there are files along with that folder,
 	// which is the case for openssl:
@@ -68,52 +82,66 @@ void extractor::do_run()
 	//   +- openssl-1.1.1d/
 	//   +- pax_global_header
 	//
-	// that pax_global_header makes 7z fail with "unspecified error"
+	// that pax_global_header makes 7z fail with "unspecified error", so -spe
+	// just can't be used at all
 	//
 	// so the handling of a duplicate directory is done manually in
-	// check_duplicate_directory() below
+	// check_duplicate_directory() below, unfortunately
 
 	if (file_.u8string().ends_with(u8".tar.gz"))
 	{
+		// tar in gz, must be piped, 7z can't do it in one step
+
 		cx().trace(context::generic, "this is a tar.gz, piping");
 
+		// untar
 		auto extract_tar = process()
 			.binary(binary())
-			.arg("x")
-			.arg("-so", file_);
+			.arg("x")     // extract
+			.arg("-so")   // output to stdout
+			.arg(file_);  // input file
 
+		// decompress
 		auto extract_gz = process()
 			.binary(binary())
-			.arg("x")
-			.arg("-aoa")
-			.arg("-si")
-			.arg("-ttar")
-			.arg("-o", where_, process::nospace);
+			.arg("x")     // extract
+			.arg("-aoa")  // overwrite all without prompt
+			.arg("-si")   // read from stdin
+			.arg("-ttar") // type is tar
+			.arg("-o", where_, process::nospace);  // output file
 
 		auto piped = process::pipe(extract_tar, extract_gz);
 		execute_and_join(piped);
 	}
 	else
 	{
+		// not tar, just extract directly
+
 		execute_and_join(process()
 			.binary(binary())
-			.arg("x")
-			.arg("-aoa")
-			.arg("-bd")
-			.arg("-bb0")
-			.arg("-o", where_, process::nospace)
-			.arg(file_));
+			.arg("x")     // extract
+			.arg("-aoa")  // overwrite all without prompt
+			.arg("-bd")   // no progress indicator
+			.arg("-bb0")  // disable log
+			.arg("-o", where_, process::nospace)  // output file
+			.arg(file_)); // input file
 	}
 
-	check_duplicate_directory(ifile.file());
+	// moves files up if necessary
+	check_for_top_level_directory(ifile.file());
 
+	// success or interruption, don't delete the directory
 	delete_output.cancel();
 
 	if (!interrupted())
+	{
+		// extraction finished and not interrupted, everything worked, so remove
+		// the interruption file
 		ifile.remove();
+	}
 }
 
-void extractor::check_duplicate_directory(const fs::path& ifile)
+void extractor::check_for_top_level_directory(const fs::path& ifile)
 {
 	const auto dir_name = where_.filename();
 
@@ -149,7 +177,7 @@ void extractor::check_duplicate_directory(const fs::path& ifile)
 		if (!fs::is_regular_file(e.path()))
 		{
 			// don't know what to do with archives that have the
-			// same directory _and_ other directories
+			// same directory _and_ other directories, bail out for now
 			cx().bail_out(context::generic,
 				"check_duplicate_directory: {} is yet another directory",
 				e.path());
@@ -198,14 +226,19 @@ void archiver::create_from_glob(
 
 	auto p = process()
 		.binary(extractor::binary())
-		.arg("a")
-		.arg(out)
-		.arg("-r")
-		.arg("-mx=5")
-		.arg(glob);
+		.arg("a")      // add to archive
+		.arg(out)      // output file
+		.arg("-r")     // recursive
+		.arg("-mx=5")  // normal compression level
+		.arg(glob);    // input file
 
 	for (auto&& i : ignore)
+	{
+		// x: exclude
+		// r: recurse
+		// !: filename or glob
 		p.arg("-xr!", i, process::nospace);
+	}
 
 	p.run();
 	p.join();
@@ -218,6 +251,8 @@ void archiver::create_from_files(
 	std::string list_file_text;
 	std::error_code ec;
 
+	// make each file relative to files_root, convert to utf8 and put in
+	// list_file_text separated by newlines
 	for (auto&& f : files)
 	{
 		fs::path rf = fs::relative(f, files_root, ec);
@@ -232,6 +267,8 @@ void archiver::create_from_files(
 	}
 
 	const auto list_file = make_temp_file();
+
+	// always delete the list file when done
 	guard g([&]
 	{
 		if (fs::exists(list_file))
@@ -246,8 +283,8 @@ void archiver::create_from_files(
 
 	auto p = process()
 		.binary(extractor::binary())
-		.arg("a")
-		.arg(out)
+		.arg("a")    // add to archive
+		.arg(out)    // output file
 		.arg("@", list_file, process::nospace)
 		.cwd(files_root);
 
diff --git a/src/tools/git.cpp b/src/tools/git.cpp
index 8563246..80eb9a5 100644
--- a/src/tools/git.cpp
+++ b/src/tools/git.cpp
@@ -26,6 +26,16 @@ void for_each_ts(const fs::path& root, F&& f)
 	}
 }
 
+std::string make_url(
+	const std::string& org, const std::string& git_file,
+	const std::string& url_pattern)
+{
+	const std::string pattern = url_pattern.empty() ?
+		details::default_github_url_pattern : url_pattern;
+
+	return fmt::format(pattern, org, git_file);
+}
+
 
 [[nodiscard]] process make_process()
 {
@@ -342,7 +352,7 @@ void git_wrap::set_credentials(const std::string& username, const std::string& e
 		set_config("user.email", email);
 }
 
-void git_wrap::set_remote(
+void git_wrap::set_origin_and_upstream_remotes(
 	std::string org, std::string key,
 	bool no_push_upstream, bool push_default_origin)
 {
@@ -440,7 +450,7 @@ void git_wrap::add_remote(
 	if (!has_remote(remote_name))
 	{
 		run(details::add_remote(
-			root_, remote_name, make_url(username, gf, url_pattern)));
+			root_, remote_name, details::make_url(username, gf, url_pattern)));
 
 		if (push_default)
 			set_config("remote.pushdefault", remote_name);
@@ -558,19 +568,10 @@ bool git_wrap::has_stashed_changes()
 	return (run(p) == 0);
 }
 
-std::string git_wrap::make_url(
-	const std::string& org, const std::string& git_file,
-	const std::string& url_pattern)
-{
-	const std::string pattern = url_pattern.empty() ?
-		details::default_github_url_pattern : url_pattern;
 
-	return fmt::format(pattern, org, git_file);
-}
-
-
-git::git(ops o)
-	: basic_process_runner("git"), op_(o)
+git::git(ops o) :
+	basic_process_runner("git"), op_(o), ignore_ts_(false), revert_ts_(false),
+	shallow_(false), no_push_upstream_(false), push_default_origin_(false)
 {
 }
 
@@ -651,7 +652,9 @@ void git::do_run()
 
 		case ops::clone_or_pull:
 		{
-			do_clone_or_pull();
+			if (!do_clone())
+				do_pull();
+
 			break;
 		}
 
@@ -662,12 +665,6 @@ void git::do_run()
 	}
 }
 
-void git::do_clone_or_pull()
-{
-	if (!do_clone())
-		do_pull();
-}
-
 bool git::do_clone()
 {
 	const fs::path dot_git = root_ / ".git";
@@ -685,7 +682,10 @@ bool git::do_clone()
 		g.set_credentials(creds_username_, creds_email_);
 
 	if (!remote_org_.empty())
-		g.set_remote(remote_org_, remote_key_, no_push_upstream_, push_default_origin_);
+	{
+		g.set_origin_and_upstream_remotes(
+			remote_org_, remote_key_, no_push_upstream_, push_default_origin_);
+	}
 
 	if (ignore_ts_)
 		g.ignore_ts(true);
diff --git a/src/tools/git.h b/src/tools/git.h
index c99f177..00ff6cd 100644
--- a/src/tools/git.h
+++ b/src/tools/git.h
@@ -3,102 +3,268 @@
 namespace mob
 {
 
+// wrapper around git commands used by the git tool below or various `mob git`
+// commands
+//
 class git_wrap
 {
 public:
+	// path to the git binary
+	//
 	static fs::path binary();
 
+	// runs git commands in the given root directory
+	//
+	// `runner` is used when the commands are issued by the git tool below,
+	// they ask the runner to run the various processes instead of running them
+	// directly; this makes logs use the name of the task running the tool, etc.
+	//
 	git_wrap(fs::path root, basic_process_runner* runner=nullptr);
 
+	// runs `git clone` with the url and branch, adds `--depth 1` when `shallow`
+	// is true
+	//
 	void clone(const mob::url& url, const std::string& branch, bool shallow);
+
+	// runs `git pull` with the given url and branch
+	//
 	void pull(const mob::url& url, const std::string& branch);
 
+	// runs `git config` to set "user.name" and "user.email"
+	//
 	void set_credentials(const std::string& username, const std::string& email);
 
-	void set_remote(
+	// 1) renames the "origin" remote to "upstream",
+	// 2) sets the "upstream" push url to "nopushurl" of `no_push_upstrea` is
+	//    true
+	// 3) adds a new "origin" remote from github with the given org and key
+	// 4) sets the "origin" remote as the default push remote if
+	//    `push_default_origin` is true
+	//
+	// this is used when cloning a repo: "origin" is usually from the
+	// ModOrganizer2 org, but most devs have their own fork in which they
+	// develop, so "origin" becomes "upstream" and a new "origin" remote is
+	// created for their own repo
+	//
+	// if there's already a remote named "upstream", this is a no-op
+	//
+	void set_origin_and_upstream_remotes(
 		std::string org, std::string key,
 		bool no_push_upstream, bool push_default_origin);
 
+	// finds all the .ts files in the root (recursive) and either sets or
+	// removes the --assume-unchanged flag on all of them
+	//
+	// .ts files are translation files that are automatically generated by Qt
+	// when building the various projects and they can change at any time;
+	// pushing them creates unnecessary merge conflicts for other devs, and it's
+	// a pita when it happens
+	//
+	// this basically ignores .ts completely when pushing: they won't be shown
+	// as modified and won't be pushed if they've changed
+	//
 	void ignore_ts(bool b);
+
+	// finds all the .ts files in the root (recursive) and reverts them (does
+	// a `git checkout` on all of them)
+	//
+	// this is used when pulling changes to revert all the .ts before pulling
+	// so there are no conflicts
+	//
 	void revert_ts();
+
+	// returns whether the given file is known to git
+	//
 	bool is_tracked(const fs::path& file);
 
+	// returns whether the given remote name exists
+	//
 	bool has_remote(const std::string& name);
 
+	// adds a remote from github
+	//
+	// remote_name:  name of the new remote
+	//
+	// org:          organization on github
+	//
+	// key:          path to a putty key, may be empty
+	//
+	// push_default: whether this remote should be the default for push, sets
+	//               the remote.pushdefault config
+	//
+	// url_pattern:  the url pattern for the remote, should be a format string
+	//               with two {} for org and git file respectively; if empty,
+	//               defaults to default_github_url_pattern in git.cpp
+	//
+	// git_file:     the name of the git file on github, such as
+	//               "modorganizer.git"; if empty, defaults to the git file used
+	//               by the "origin" remote
+	//
+	//               this is necessary in some operations like in
+	//               set_origin_and_upstream_remotes() because the "origin"
+	//               remote might not exist at that point
+	//
 	void add_remote(
 		const std::string& remote_name,
 		const std::string& org, const std::string& key, bool push_default,
 		const std::string& url_pattern={}, const std::string& git_file={});
 
+	// renames remote `from` to `to`
+	//
 	void rename_remote(const std::string& from, const std::string& to);
 
+	// sets the push url of the given remote
+	//
 	void set_remote_push(const std::string& remote, const std::string& url);
 
+	// runs `git config key value`
+	//
 	void set_config(const std::string& key, const std::string& value);
 
+	// sets --assume-unchanged or --no-assume-unchanged for the given file
+	//
 	void set_assume_unchanged(const fs::path& relative_file, bool on);
 
+	// returns the .git file used by the origin remote, such as modorganizer.git
+	//
 	std::string git_file();
 
+
+	// runs `git init`
+	//
 	void init_repo();
 
+	// runs `git apply` and feeds the given string as stdin; used to apply a
+	// PR diff downloaded for github, for example
+	//
 	void apply(const std::string& diff);
 
+	// runs `git fetch remote branch`
+	//
 	void fetch(const std::string& remote, const std::string& branch);
 
+	// runs `git checkout what`
+	//
 	void checkout(const std::string& what);
 
+	// runs `git submodule add` for the given branch submodule and url
+	//
 	void add_submodule(
 		const std::string& branch, const std::string& submodule,
 		const mob::url& url);
 
+	// returns the output of `git branch --show-current`, which is the name of
+	// the active branch
+	//
 	std::string current_branch();
 
+	// whether the root directory given in the constructor is a valid git repo
+	//
 	bool is_git_repo();
 
+	// whether the repo has uncommitted changes (basically checks `git status`);
+	// see delete_directory() below
+	//
 	bool has_uncommitted_changes();
+
+	// whether the repo has stashed changes (checks `git stash show`); see
+	// delete_directory() below
+	//
 	bool has_stashed_changes();
 
+
+	// used by various tasks to delete a directory that was created by pulling
+	// from git
+	//
+	// if the directory has uncommitted or stashed changes, it will output an
+	// error and bail out; if not, the directory is deleted normally with
+	// op::delete_directory()
+	//
 	static void delete_directory(const context& cx, const fs::path& dir);
+
+	// runs `git ls-remote` to check if the repo at the url has the given branch
+	// name
+	//
+	// used mostly by `mob release official` when given a branch name to make
+	// sure the branch exists in all repos before starting the build so it
+	// doesn't fail in the middle
+	//
 	static bool remote_branch_exists(const mob::url& u, const std::string& name);
 
 private:
+	// git root directory, from constructor
 	fs::path root_;
+
+	// optional tool that's running these git commands
 	basic_process_runner* runner_;
 
+	// either runs the given process directly or asks runner_ to run it if it's
+	// not null
+	//
+	// both versions are needed because run() can be called with a temporary
+	// process object sometimes
+	//
 	int run(process&& p);
 	int run(process& p);
 
+	// log context, either gcx() or the one from runner_ if it's not null
+	//
 	const context& cx();
-
-	static std::string make_url(
-		const std::string& org, const std::string& git_file,
-		const std::string& url_pattern={});
 };
 
 
-
+// tool to handle git operations, used by tasks
+//
 class git : public basic_process_runner
 {
 public:
+	// what run() should do
+	//
 	enum ops
 	{
+		// clones the repo
 		clone = 1,
+
+		// pulls the repo
 		pull,
+
+		// pulls if the repo exists, clones otherwise
 		clone_or_pull
 	};
 
 	git(ops o);
 
+	// url to clone or pull from
+	//
 	git& url(const mob::url& u);
+
+	// root directory of the git repo
+	//
 	git& root(const fs::path& dir);
+
+	// branch to clone or pull
+	//
 	git& branch(const std::string& name);
 
+
+	// whether all .ts files should be marked as --assume-unchanged when cloning
+	//
 	git& ignore_ts_on_clone(bool b);
+
+	// whether all .ts files should be reverted when pulling
+	//
 	git& revert_ts_on_pull(bool b);
+
+	// if this is called, sets "user.name" and "user.email" when cloning
+	//
 	git& credentials(const std::string& username, const std::string& email);
+
+	// if true, clones with `--depth 1`
+	//
 	git& shallow(bool b);
 
+	// if set, calls git_wrap::set_origin_and_upstream_remotes()
+	//
 	git& remote(
 		std::string org, std::string key,
 		bool no_push_upstream, bool push_default_origin);
@@ -107,37 +273,54 @@ protected:
 	void do_run() override;
 
 private:
+	// operation
 	ops op_;
+
+	// set by the various functions above
 	mob::url url_;
 	fs::path root_;
 	std::string branch_;
-
-	bool ignore_ts_ = false;
-	bool revert_ts_ = false;
+	bool ignore_ts_;
+	bool revert_ts_;
 	std::string creds_username_;
 	std::string creds_email_;
-	bool shallow_ = false;
+	bool shallow_;
 	std::string remote_org_;
 	std::string remote_key_;
-	bool no_push_upstream_ = false;
-	bool push_default_origin_ = false;
+	bool no_push_upstream_;
+	bool push_default_origin_;
 
-	void do_clone_or_pull();
 	bool do_clone();
 	void do_pull();
 };
 
 
+// tool to handle git submodule operations, used by the modorganizer task to
+// set up the submodules
+//
+// this tool is not normally run directly, instances of git_submodule are given
+// to the git_submodule_adder, which runs all of them in a thread
+//
 class git_submodule : public basic_process_runner
 {
 public:
 	git_submodule();
 
+	// remote url
+	//
 	git_submodule& url(const mob::url& u);
-	git_submodule& root(const fs::path& dir);
-	git_submodule& branch(const std::string& name);
-	git_submodule& submodule(const std::string& name);
 
+	// root directory of the repo
+	//
+	git_submodule& root(const fs::path& dir);
+
+	// branch name
+	//
+	git_submodule& branch(const std::string& name);
+
+	// submodule name
+	//
+	git_submodule& submodule(const std::string& name);
 	const std::string& submodule() const;
 
 protected:
@@ -151,23 +334,40 @@ private:
 };
 
 
+// queues submodule operations with queue(), runs them in a thread because they
+// take a long time but can happen while stuff is building
+//
 class git_submodule_adder : public instrumentable<2>
 {
 public:
+	// instrumentable categories
+	//
 	enum class times
 	{
 		add_submodule_wait,
 		add_submodule
 	};
 
+	// calls stop() and joins
+	//
 	~git_submodule_adder();
 
+	// only one instance, runs the thread and waits for submodules to be added
+	// by queue()
+	//
 	static git_submodule_adder& instance();
 
+	// adds a submodule to the queue
+	//
 	void queue(git_submodule g);
+
+	// stops the thread
+	//
 	void stop();
 
 private:
+	// used to sleep until queue() is called
+	//
 	struct sleeper
 	{
 		std::mutex m;
@@ -175,18 +375,39 @@ private:
 		bool ready = false;
 	};
 
+	// log context
 	context cx_;
+
+	// thread
 	std::thread thread_;
+
+	// queue
 	std::vector<git_submodule> queue_;
 	mutable std::mutex queue_mutex_;
+
+	// true in stop(), stops the thread
 	std::atomic<bool> quit_;
+
+	// used to sleep until queue() is called
 	sleeper sleeper_;
 
+
 	git_submodule_adder();
+
+	// starts the thread
+	//
 	void run();
 
+	// thread function, sleeps until queue() is called
+	//
 	void thread_fun();
+
+	// forces the thread function to wake up
+	//
 	void wakeup();
+
+	// processes the queue
+	//
 	void process();
 };
 
diff --git a/src/tools/tools.h b/src/tools/tools.h
index d59ba95..5ef76e0 100644
--- a/src/tools/tools.h
+++ b/src/tools/tools.h
@@ -231,9 +231,14 @@ private:
 	//
 	fs::path path_for_url(const mob::url& u) const;
 
-	// checks if the file exists
+	// checks if one of the output files already exists, sets file_ to it if
+	// necessary and returns true
 	//
-	bool try_picking(const fs::path& file);
+	bool use_existing();
+
+	// tries to download the given url, returns whether it succeeded
+	//
+	bool try_download(const mob::url& u);
 };
 
 
@@ -355,7 +360,7 @@ private:
 	// some archives have a top level directory, this moves all the files up one
 	// directory and deletes the now empty top level directory
 	//
-	void check_duplicate_directory(const fs::path& ifile);
+	void check_for_top_level_directory(const fs::path& ifile);
 };
 
 
@@ -572,6 +577,8 @@ public:
 	static std::string sdk();
 
 
+	// what run() should do
+	//
 	enum ops
 	{
 		upgrade = 1