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

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