patch, 2 partsimprove relocate_git_dir to fail into a sane state.

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

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

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

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

>> On 12/19, Stefan Beller wrote:
> This code is heavily inspired by refs/files-backend.c which upon
> closer inspection only retries directory things within the git directory
> (which is assumed to be accessed in parallel by different invocations
> of Git)

>
> Thanks,
> Stefan
>
> Stefan Beller (2):
>   dir.c: split up connect_work_tree_and_git_dir
>   dir.c: add retry logic to relocate_gitdir
>
>  dir.c       | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  dir.h       |   6 ++--
>  submodule.c |   3 +-
>  3 files changed, 110 insertions(+), 17 deletions(-)

> Stefan Beller <sbeller@google.com> writes:
>
>> This goes on top of sb/submodule-embed-gitdir.
>> When the absorbing of a git dir fails, try to recover into a sane state,
>> i.e. try to undo the move of the git dir.
>
> Are these unconditionally good improvements?  I ask because the
> series is still not in 'next' and it is the last chance to fold
> these into existing patches if we wanted to.
>

> On Mon, Dec 19, 2016 at 2:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> This goes on top of sb/submodule-embed-gitdir.
>>> When the absorbing of a git dir fails, try to recover into a sane state,
>>> i.e. try to undo the move of the git dir.
>>
>> Are these unconditionally good improvements?  I ask because the
>> series is still not in 'next' and it is the last chance to fold
>> these into existing patches if we wanted to.
>
> Actually I forgot to mark these as RFC-ish as it is a response to
> https://public-inbox.org/git/20161219053507.GA2335@duynguyen.vn.dektech.internal/
> (which was the only review comment this round)
>
> So I'd say these patches are rather experimental in my mind
> unlike the absorb series, which I assume is ok as is already.