Subject: [PATCH 2/2] dir.c: add retry logic to relocate_gitdir

From: Stefan Beller <[email-masked]>
Date: Mon, 19 Dec 2016 13:57:09 -0800

Relocating a git directory consists of 3 steps:
1) Move the directory.
2) Update the gitlink file.
3) Set core.worktree correctly.

In an ideal world all three steps would be part of one transaction, such
that either all of them happen correctly or none of them.
However currently we just execute these three steps sequentially and die
in case of an error in any of these 3 steps.

Dying is ok in 1) as the transaction hasn't started and the state is
recoverable.

When dying in 2), this is a problem as the repo state is left in an
inconsistent state, e.g. the git link file of a submodule could be
empty and hence even the superproject appears to be broken as basic
commands such as git-status would die as there is it cannot tell the
state of the submodule.
So in that case try to undo 1) to be in a less sufferable state.

3) is less of an issue as experiments with submodules show. When
core.worktree is unset or set incorrectly, git-status still works
both in the superproject as well as the working tree of the submodule.

Signed-off-by: Stefan Beller <[email-masked]>
---
 dir.c       | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 dir.h       |  6 ++--
 submodule.c |  3 +-
 3 files changed, 91 insertions(+), 12 deletions(-)

diff --git a/dir.c b/dir.c
index b2cb23fe88..e4e3f69869 100644
--- a/dir.c
+++ b/dir.c
@@ -2749,30 +2749,66 @@ void untracked_cache_add_to_index(struct index_state *istate,
 	untracked_cache_invalidate_path(istate, path);
 }
 
-static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
+/*
+ * Just like write_file, we try hard to write the full content to the file.
+ * If there is suspicion the write did not work correctly, make sure the file
+ * is removed again.
+ * Return 0 if the write succeeded, -1 if the file was removed,
+ * -2 if the (partial) file is still there.
+ */
+static int write_file_or_remove(const char *path, const char *buf, size_t len)
+{
+	int retries = 3;
+	int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+	if (write_in_full(fd, buf, len) != len) {
+		warning_errno(_("could not write '%s'"), path);
+		goto err;
+	}
+	if (close(fd)) {
+		warning_errno(_("could not close '%s'"), path);
+		goto err;
+	}
+	return 0;
+err:
+	while (retries-- > 0) {
+		if (file_exists(path))
+			unlink_or_warn(path);
+		else
+			return -1;
+	}
+	return -2;
+}
+
+static int point_gitlink_file_to(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
+	struct strbuf content = STRBUF_INIT;
+	int ret;
 
 	strbuf_addf(&file_name, "%s/.git", work_tree);
-	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, work_tree, &rel_path));
+	strbuf_addf(&content, "gitdir: %s",
+		    relative_path(git_dir, work_tree, &rel_path));
+	ret = write_file_or_remove(file_name.buf, content.buf, content.len);
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
+	return ret;
 }
 
-static void set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
+static int set_core_work_tree_to_connect(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
+	int ret;
 
 	strbuf_addf(&file_name, "%s/config", git_dir);
-	git_config_set_in_file(file_name.buf, "core.worktree",
+	ret = git_config_set_in_file_gently(file_name.buf, "core.worktree",
 			       relative_path(work_tree, git_dir, &rel_path));
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
+	return ret;
 }
 
 /* Update gitfile and core.worktree setting to connect work tree and git dir */
@@ -2790,12 +2826,54 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
 
 /*
  * Migrate the git directory of the given path from old_git_dir to new_git_dir.
+ * Return 0 on success and -1 on a minor issue. Die in case the repository is
+ * fatally messed up.
  */
-void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
+int relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
 {
+	char *git_dir = xstrdup(real_path(new_git_dir));
+	char *work_tree = xstrdup(real_path(path));
+	int c;
+
 	if (rename(old_git_dir, new_git_dir) < 0)
-		die_errno(_("could not migrate git directory from '%s' to '%s'"),
+		die_errno(_("could not rename git directory from '%s' to '%s'"),
 			old_git_dir, new_git_dir);
 
-	connect_work_tree_and_git_dir(path, new_git_dir);
+	c = point_gitlink_file_to(work_tree, git_dir);
+	if (c < 0) {
+		warning(_("tried to move the git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+		warning(_("problems with creating a .git file in '%s' to point to '%s'"),
+			work_tree, new_git_dir);
+		if (c == -1) {
+			warning(_("try to undo the move"));
+			if (rename(new_git_dir, old_git_dir) < 0)
+				die_errno(_("could not rename git directory from '%s' to '%s'"),
+					new_git_dir, old_git_dir);
+			return -1;
+		} else if (c == -2) {
+			warning(_("The .git file is still there, "
+				"where the undo operation would move the git "
+				"directory"));
+			die(_("failed to undo the git directory relocation"));
+		}
+	};
+
+	if (set_core_work_tree_to_connect(work_tree, git_dir) < 0) {
+		/*
+		 * At this point the git dir was moved and the gitlink file
+		 * was updated correctly, this leaves the repository in a
+		 * state that is not totally broken.  Setting the core.worktree
+		 * correctly would have been the last step to perform a
+		 * complete git directory relocation, but this is good enough
+		 * to not die.
+		 */
+		warning(_("could not set core.worktree in '%s' to point at '%s'"),
+			git_dir, work_tree);
+		return -1;
+	}
+
+	free(work_tree);
+	free(git_dir);
+	return 0;
 }
diff --git a/dir.h b/dir.h
index bf23a470af..86f1cf790f 100644
--- a/dir.h
+++ b/dir.h
@@ -336,7 +336,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
-extern void relocate_gitdir(const char *path,
-			    const char *old_git_dir,
-			    const char *new_git_dir);
+extern int relocate_gitdir(const char *path,
+			   const char *old_git_dir,
+			   const char *new_git_dir);
 #endif
diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..fa1f44bb5a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1277,7 +1277,8 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
 		prefix ? prefix : "", path,
 		real_old_git_dir, real_new_git_dir);
 
-	relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
+	if (relocate_gitdir(path, real_old_git_dir, real_new_git_dir))
+		die(_("could not relocate git directory of '%s'"), path);
 
 	free(old_git_dir);
 	free(real_old_git_dir);
-- 
2.11.0.rc2.53.gb7b3fba.dirty