[PATCH 0/2] improve relocate_git_dir to fail into a sane state.

[PATCH 0/2] improve relocate_git_dir to fail into a sane state.

[Stefan Beller]

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.

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(-)

-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCH 1/2] dir.c: split up connect_work_tree_and_git_dir

[Stefan Beller]

In a later patch we want to treat the failures of each of the two steps
differently, so split them up first.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 dir.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/dir.c b/dir.c
index d872cc1570..b2cb23fe88 100644
--- a/dir.c
+++ b/dir.c
@@ -2749,27 +2749,41 @@ void untracked_cache_add_to_index(struct index_state *istate,
 	untracked_cache_invalidate_path(istate, path);
 }
 
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
+static void point_gitlink_file_to(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	char *git_dir = xstrdup(real_path(git_dir_));
-	char *work_tree = xstrdup(real_path(work_tree_));
 
-	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
 	write_file(file_name.buf, "gitdir: %s",
 		   relative_path(git_dir, work_tree, &rel_path));
 
-	/* Update core.worktree setting */
-	strbuf_reset(&file_name);
+	strbuf_release(&file_name);
+	strbuf_release(&rel_path);
+}
+
+static void 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;
+
 	strbuf_addf(&file_name, "%s/config", git_dir);
 	git_config_set_in_file(file_name.buf, "core.worktree",
 			       relative_path(work_tree, git_dir, &rel_path));
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
+}
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
+{
+	char *git_dir = xstrdup(real_path(git_dir_));
+	char *work_tree = xstrdup(real_path(work_tree_));
+
+	point_gitlink_file_to(work_tree, git_dir);
+	set_core_work_tree_to_connect(work_tree, git_dir);
+
 	free(work_tree);
 	free(git_dir);
 }
-- 
2.11.0.rc2.53.gb7b3fba.dirty

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

[Stefan Beller]

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

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

[Brandon Williams]

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

Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.

[Junio C Hamano]

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.

>
> 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(-)

Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.

[Stefan Beller]

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.

Thanks,
Stefan

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

[Stefan Beller]

On Mon, Dec 19, 2016 at 2:25 PM, Brandon Williams <bmwill@google.com> wrote:
> 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?
>

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)

So I think we could drop the retry logic.

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

good point.

Re: [PATCH 0/2] improve relocate_git_dir to fail into a sane state.

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

OK.  Thanks for clarification.

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

[Duy Nguyen]

On Tue, Dec 20, 2016 at 6:19 AM, Stefan Beller <sbeller@google.com> wrote:
>> 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)

I take inspiration from lock/temp files on the other hand. Could we
keep some sort of "undo journal" as we move along and clear it when
the "transaction" is completed? The good thing about lock files is,
even if you die() or SIGTERM'd, rolling back can still take place (but
it has to be something very simple, like removing or renaming because
you don't want to do big things in a signal handler).

If things turn out to be complicated and risky to be executed in an
unknown context, we could still print a helpful message like "yeah
you're in trouble, maybe try this and this, or consult git mailing
list. We apologise for the inconvenience,"
-- 
Duy