clone: refactor to pass clone options around

Instead of dealing with the clone options sub-options (fetch, checkout,
etc) individually, treat them as a cohesive whole when passing them
throughout the system.

Additionally, move some functions around within the file to avoid
unnecessary decls at the top of the file.  And change a function
signature to avoid conflating truth with error.

3 files changed, 224 insertions, 165 deletions

diff --git a/src/libgit2/clone.c b/src/libgit2/clone.c
index 93ff7d802..bec8dc459 100644
--- a/src/libgit2/clone.c
+++ b/src/libgit2/clone.c
@@ -24,8 +24,6 @@
 #include "repository.h"
 #include "odb.h"
 
-static int clone_local_into(git_repository *repo, git_remote *remote, const git_fetch_options *fetch_opts, const git_checkout_options *co_opts, const char *branch, int link);
-
 static int create_branch(
 	git_reference **branch,
 	git_repository *repo,
@@ -360,42 +358,45 @@ on_error:
 static bool should_checkout(
 	git_repository *repo,
 	bool is_bare,
-	const git_checkout_options *opts)
+	const git_clone_options *opts)
 {
 	if (is_bare)
 		return false;
 
-	if (!opts)
-		return false;
-
-	if (opts->checkout_strategy == GIT_CHECKOUT_NONE)
+	if (opts->checkout_opts.checkout_strategy == GIT_CHECKOUT_NONE)
 		return false;
 
 	return !git_repository_head_unborn(repo);
 }
 
-static int checkout_branch(git_repository *repo, git_remote *remote, const git_checkout_options *co_opts, const char *branch, const char *reflog_message)
+static int checkout_branch(
+	git_repository *repo,
+	git_remote *remote,
+	const git_clone_options *opts,
+	const char *reflog_message)
 {
 	int error;
 
-	if (branch)
-		error = update_head_to_branch(repo, remote, branch, reflog_message);
+	if (opts->checkout_branch)
+		error = update_head_to_branch(repo, remote, opts->checkout_branch, reflog_message);
 	/* Point HEAD to the same ref as the remote's head */
 	else
 		error = update_head_to_remote(repo, remote, reflog_message);
 
-	if (!error && should_checkout(repo, git_repository_is_bare(repo), co_opts))
-		error = git_checkout_head(repo, co_opts);
+	if (!error && should_checkout(repo, git_repository_is_bare(repo), opts))
+		error = git_checkout_head(repo, &opts->checkout_opts);
 
 	return error;
 }
 
-static int clone_into(git_repository *repo, git_remote *_remote, const git_fetch_options *opts, const git_checkout_options *co_opts, const char *branch)
+static int clone_into(
+	git_repository *repo,
+	git_remote *_remote,
+	const git_clone_options *opts)
 {
-	int error;
 	git_str reflog_message = GIT_STR_INIT;
-	git_fetch_options fetch_opts;
 	git_remote *remote;
+	int error;
 
 	GIT_ASSERT_ARG(repo);
 	GIT_ASSERT_ARG(_remote);
@@ -408,15 +409,12 @@ static int clone_into(git_repository *repo, git_remote *_remote, const git_fetch
 	if ((error = git_remote_dup(&remote, _remote)) < 0)
 		return error;
 
-	memcpy(&fetch_opts, opts, sizeof(git_fetch_options));
-	fetch_opts.update_fetchhead = 0;
-	fetch_opts.download_tags = GIT_REMOTE_DOWNLOAD_TAGS_ALL;
 	git_str_printf(&reflog_message, "clone: from %s", git_remote_url(remote));
 
-	if ((error = git_remote_fetch(remote, NULL, &fetch_opts, git_str_cstr(&reflog_message))) != 0)
+	if ((error = git_remote_fetch(remote, NULL, &opts->fetch_opts, git_str_cstr(&reflog_message))) != 0)
 		goto cleanup;
 
-	error = checkout_branch(repo, remote, co_opts, branch, git_str_cstr(&reflog_message));
+	error = checkout_branch(repo, remote, opts, git_str_cstr(&reflog_message));
 
 cleanup:
 	git_remote_free(remote);
@@ -425,37 +423,143 @@ cleanup:
 	return error;
 }
 
-int git_clone__should_clone_local(const char *url_or_path, git_clone_local_t local)
+static bool can_link(const char *src, const char *dst, int link)
+{
+#ifdef GIT_WIN32
+	GIT_UNUSED(src);
+	GIT_UNUSED(dst);
+	GIT_UNUSED(link);
+	return false;
+#else
+
+	struct stat st_src, st_dst;
+
+	if (!link)
+		return false;
+
+	if (p_stat(src, &st_src) < 0)
+		return false;
+
+	if (p_stat(dst, &st_dst) < 0)
+		return false;
+
+	return st_src.st_dev == st_dst.st_dev;
+#endif
+}
+
+static int clone_local_into(
+	git_repository *repo,
+	git_remote *remote,
+	const git_clone_options *opts)
+{
+	int error, flags;
+	git_repository *src;
+	git_str src_odb = GIT_STR_INIT, dst_odb = GIT_STR_INIT, src_path = GIT_STR_INIT;
+	git_str reflog_message = GIT_STR_INIT;
+	bool link = (opts && opts->local != GIT_CLONE_LOCAL_NO_LINKS);
+
+	GIT_ASSERT_ARG(repo);
+	GIT_ASSERT_ARG(remote);
+
+	if (!git_repository_is_empty(repo)) {
+		git_error_set(GIT_ERROR_INVALID, "the repository is not empty");
+		return -1;
+	}
+
+	/*
+	 * Let's figure out what path we should use for the source
+	 * repo, if it's not rooted, the path should be relative to
+	 * the repository's worktree/gitdir.
+	 */
+	if ((error = git_fs_path_from_url_or_path(&src_path, git_remote_url(remote))) < 0)
+		return error;
+
+	/* Copy .git/objects/ from the source to the target */
+	if ((error = git_repository_open(&src, git_str_cstr(&src_path))) < 0) {
+		git_str_dispose(&src_path);
+		return error;
+	}
+
+	if (git_repository__item_path(&src_odb, src, GIT_REPOSITORY_ITEM_OBJECTS) < 0 ||
+	    git_repository__item_path(&dst_odb, repo, GIT_REPOSITORY_ITEM_OBJECTS) < 0) {
+		error = -1;
+		goto cleanup;
+	}
+
+	flags = 0;
+	if (can_link(git_repository_path(src), git_repository_path(repo), link))
+		flags |= GIT_CPDIR_LINK_FILES;
+
+	error = git_futils_cp_r(git_str_cstr(&src_odb), git_str_cstr(&dst_odb),
+				flags, GIT_OBJECT_DIR_MODE);
+
+	/*
+	 * can_link() doesn't catch all variations, so if we hit an
+	 * error and did want to link, let's try again without trying
+	 * to link.
+	 */
+	if (error < 0 && link) {
+		flags &= ~GIT_CPDIR_LINK_FILES;
+		error = git_futils_cp_r(git_str_cstr(&src_odb), git_str_cstr(&dst_odb),
+					flags, GIT_OBJECT_DIR_MODE);
+	}
+
+	if (error < 0)
+		goto cleanup;
+
+	git_str_printf(&reflog_message, "clone: from %s", git_remote_url(remote));
+
+	if ((error = git_remote_fetch(remote, NULL, &opts->fetch_opts, git_str_cstr(&reflog_message))) != 0)
+		goto cleanup;
+
+	error = checkout_branch(repo, remote, opts, git_str_cstr(&reflog_message));
+
+cleanup:
+	git_str_dispose(&reflog_message);
+	git_str_dispose(&src_path);
+	git_str_dispose(&src_odb);
+	git_str_dispose(&dst_odb);
+	git_repository_free(src);
+	return error;
+}
+
+int git_clone__should_clone_local(
+	bool *out,
+	const char *url_or_path,
+	git_clone_local_t local)
 {
 	git_str fromurl = GIT_STR_INIT;
 	const char *path = url_or_path;
-	bool is_url, is_local;
+	bool is_url;
+	int error = 0;
 
-	if (local == GIT_CLONE_NO_LOCAL)
-		return 0;
+	if (local == GIT_CLONE_NO_LOCAL) {
+		*out = false;
+		goto done;
+	}
 
 	if ((is_url = git_fs_path_is_local_file_url(url_or_path)) != 0) {
 		if (git_fs_path_fromurl(&fromurl, url_or_path) < 0) {
-			is_local = -1;
+			error = -1;
 			goto done;
 		}
 
 		path = fromurl.ptr;
 	}
 
-	is_local = (!is_url || local != GIT_CLONE_LOCAL_AUTO) &&
+	*out = (!is_url || local != GIT_CLONE_LOCAL_AUTO) &&
 		git_fs_path_isdir(path);
 
 done:
 	git_str_dispose(&fromurl);
-	return is_local;
+	return error;
 }
 
-static int git__clone(
+static int clone_repo(
 	git_repository **out,
 	const char *url,
 	const char *local_path,
-	const git_clone_options *_options,
+	const git_clone_options *given_opts,
 	int use_existing)
 {
 	int error = 0;
@@ -469,11 +573,15 @@ static int git__clone(
 	GIT_ASSERT_ARG(url);
 	GIT_ASSERT_ARG(local_path);
 
-	if (_options)
-		memcpy(&options, _options, sizeof(git_clone_options));
+	if (given_opts)
+		memcpy(&options, given_opts, sizeof(git_clone_options));
 
 	GIT_ERROR_CHECK_VERSION(&options, GIT_CLONE_OPTIONS_VERSION, "git_clone_options");
 
+	/* enforce some behavior on fetch */
+	options.fetch_opts.update_fetchhead = 0;
+	options.fetch_opts.download_tags = GIT_REMOTE_DOWNLOAD_TAGS_ALL;
+
 	/* Only clone to a new directory or an empty directory */
 	if (git_fs_path_exists(local_path) && !use_existing && !git_fs_path_is_empty_dir(local_path)) {
 		git_error_set(GIT_ERROR_INVALID,
@@ -494,19 +602,17 @@ static int git__clone(
 		return error;
 
 	if (!(error = create_and_configure_origin(&origin, repo, url, &options))) {
-		int clone_local = git_clone__should_clone_local(url, options.local);
-		int link = options.local != GIT_CLONE_LOCAL_NO_LINKS;
-
-		if (clone_local == 1)
-			error = clone_local_into(
-				repo, origin, &options.fetch_opts, &options.checkout_opts,
-				options.checkout_branch, link);
-		else if (clone_local == 0)
-			error = clone_into(
-				repo, origin, &options.fetch_opts, &options.checkout_opts,
-				options.checkout_branch);
+		bool clone_local;
+
+		if ((error = git_clone__should_clone_local(&clone_local, url, options.local)) < 0) {
+			git_remote_free(origin);
+			return error;
+		}
+
+		if (clone_local)
+			error = clone_local_into(repo, origin, &options);
 		else
-			error = -1;
+			error = clone_into(repo, origin, &options);
 
 		git_remote_free(origin);
 	}
@@ -531,18 +637,18 @@ int git_clone(
 	git_repository **out,
 	const char *url,
 	const char *local_path,
-	const git_clone_options *_options)
+	const git_clone_options *options)
 {
-	return git__clone(out, url, local_path, _options, 0);
+	return clone_repo(out, url, local_path, options, 0);
 }
 
 int git_clone__submodule(
 	git_repository **out,
 	const char *url,
 	const char *local_path,
-	const git_clone_options *_options)
+	const git_clone_options *options)
 {
-	return git__clone(out, url, local_path, _options, 1);
+	return clone_repo(out, url, local_path, options, 1);
 }
 
 int git_clone_options_init(git_clone_options *opts, unsigned int version)
@@ -558,99 +664,3 @@ int git_clone_init_options(git_clone_options *opts, unsigned int version)
 	return git_clone_options_init(opts, version);
 }
 #endif
-
-static bool can_link(const char *src, const char *dst, int link)
-{
-#ifdef GIT_WIN32
-	GIT_UNUSED(src);
-	GIT_UNUSED(dst);
-	GIT_UNUSED(link);
-	return false;
-#else
-
-	struct stat st_src, st_dst;
-
-	if (!link)
-		return false;
-
-	if (p_stat(src, &st_src) < 0)
-		return false;
-
-	if (p_stat(dst, &st_dst) < 0)
-		return false;
-
-	return st_src.st_dev == st_dst.st_dev;
-#endif
-}
-
-static int clone_local_into(git_repository *repo, git_remote *remote, const git_fetch_options *fetch_opts, const git_checkout_options *co_opts, const char *branch, int link)
-{
-	int error, flags;
-	git_repository *src;
-	git_str src_odb = GIT_STR_INIT, dst_odb = GIT_STR_INIT, src_path = GIT_STR_INIT;
-	git_str reflog_message = GIT_STR_INIT;
-
-	GIT_ASSERT_ARG(repo);
-	GIT_ASSERT_ARG(remote);
-
-	if (!git_repository_is_empty(repo)) {
-		git_error_set(GIT_ERROR_INVALID, "the repository is not empty");
-		return -1;
-	}
-
-	/*
-	 * Let's figure out what path we should use for the source
-	 * repo, if it's not rooted, the path should be relative to
-	 * the repository's worktree/gitdir.
-	 */
-	if ((error = git_fs_path_from_url_or_path(&src_path, git_remote_url(remote))) < 0)
-		return error;
-
-	/* Copy .git/objects/ from the source to the target */
-	if ((error = git_repository_open(&src, git_str_cstr(&src_path))) < 0) {
-		git_str_dispose(&src_path);
-		return error;
-	}
-
-	if (git_repository__item_path(&src_odb, src, GIT_REPOSITORY_ITEM_OBJECTS) < 0 ||
-	    git_repository__item_path(&dst_odb, repo, GIT_REPOSITORY_ITEM_OBJECTS) < 0) {
-		error = -1;
-		goto cleanup;
-	}
-
-	flags = 0;
-	if (can_link(git_repository_path(src), git_repository_path(repo), link))
-		flags |= GIT_CPDIR_LINK_FILES;
-
-	error = git_futils_cp_r(git_str_cstr(&src_odb), git_str_cstr(&dst_odb),
-				flags, GIT_OBJECT_DIR_MODE);
-
-	/*
-	 * can_link() doesn't catch all variations, so if we hit an
-	 * error and did want to link, let's try again without trying
-	 * to link.
-	 */
-	if (error < 0 && link) {
-		flags &= ~GIT_CPDIR_LINK_FILES;
-		error = git_futils_cp_r(git_str_cstr(&src_odb), git_str_cstr(&dst_odb),
-					flags, GIT_OBJECT_DIR_MODE);
-	}
-
-	if (error < 0)
-		goto cleanup;
-
-	git_str_printf(&reflog_message, "clone: from %s", git_remote_url(remote));
-
-	if ((error = git_remote_fetch(remote, NULL, fetch_opts, git_str_cstr(&reflog_message))) != 0)
-		goto cleanup;
-
-	error = checkout_branch(repo, remote, co_opts, branch, git_str_cstr(&reflog_message));
-
-cleanup:
-	git_str_dispose(&reflog_message);
-	git_str_dispose(&src_path);
-	git_str_dispose(&src_odb);
-	git_str_dispose(&dst_odb);
-	git_repository_free(src);
-	return error;
-}
diff --git a/src/libgit2/clone.h b/src/libgit2/clone.h
index 7d73cabd5..fde796f91 100644
--- a/src/libgit2/clone.h
+++ b/src/libgit2/clone.h
@@ -15,6 +15,9 @@ extern int git_clone__submodule(git_repository **out,
 	const char *url, const char *local_path,
 	const git_clone_options *_options);
 
-extern int git_clone__should_clone_local(const char *url, git_clone_local_t local);
+extern int git_clone__should_clone_local(
+	bool *out,
+	const char *url,
+	git_clone_local_t local);
 
 #endif
diff --git a/tests/libgit2/clone/local.c b/tests/libgit2/clone/local.c
index e0bd74df7..70d823774 100644
--- a/tests/libgit2/clone/local.c
+++ b/tests/libgit2/clone/local.c
@@ -54,41 +54,87 @@ static int unc_path(git_str *buf, const char *host, const char *path)
 void test_clone_local__should_clone_local(void)
 {
 	git_str buf = GIT_STR_INIT;
+	bool local;
 
 	/* we use a fixture path because it needs to exist for us to want to clone */
 	const char *path = cl_fixture("testrepo.git");
 
+	/* empty string */
 	cl_git_pass(file_url(&buf, "", path));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_AUTO));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(false, local);
 
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_assert_equal_i(false, local);
+
+	/* localhost is special */
 	cl_git_pass(file_url(&buf, "localhost", path));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_AUTO));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL));
+	cl_assert_equal_i(true, local);
 
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_assert_equal_i(false, local);
+
+	/* a remote host */
 	cl_git_pass(file_url(&buf, "other-host.mycompany.com", path));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_AUTO));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_NO_LOCAL));
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_assert_equal_i(false, local);
 
 	/* Ensure that file:/// urls are percent decoded: .git == %2e%67%69%74 */
 	cl_git_pass(file_url(&buf, "", path));
 	git_str_shorten(&buf, 4);
 	cl_git_pass(git_str_puts(&buf, "%2e%67%69%74"));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_AUTO));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
-	cl_assert_equal_i(0, git_clone__should_clone_local(buf.ptr, GIT_CLONE_NO_LOCAL));
-
-	cl_assert_equal_i(1,  git_clone__should_clone_local(path, GIT_CLONE_LOCAL_AUTO));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(path, GIT_CLONE_LOCAL));
-	cl_assert_equal_i(1,  git_clone__should_clone_local(path, GIT_CLONE_LOCAL_NO_LINKS));
-	cl_assert_equal_i(0, git_clone__should_clone_local(path, GIT_CLONE_NO_LOCAL));
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(false, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_LOCAL_NO_LINKS));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, buf.ptr, GIT_CLONE_NO_LOCAL));
+	cl_assert_equal_i(false, local);
+
+	/* a local path on disk */
+	cl_git_pass(git_clone__should_clone_local(&local, path, GIT_CLONE_LOCAL_AUTO));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, path, GIT_CLONE_LOCAL));
+
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, path, GIT_CLONE_LOCAL_NO_LINKS));
+	cl_assert_equal_i(true, local);
+
+	cl_git_pass(git_clone__should_clone_local(&local, path, GIT_CLONE_NO_LOCAL));
+	cl_assert_equal_i(false, local);
 
 	git_str_dispose(&buf);
 }