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

[Brandon Williams <bmwill@google.com>]

On 12/19, Stefan Beller wrote:
> 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.

I now see why an atomic filesystem swap operation would be useful :)

> 
> 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 <sbeller@google.com>
> ---
>  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;
> +}

Any reason why on attempt 2 or 3 this would succeed if it failed on try
1?

> +
> +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;

I guess in a later patch we could clean up these calls to real_path to
use real_pathdup from bw/realpath-wo-chdir.

> +
>  	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
> 

-- 
Brandon Williams