On Mon, Dec 19, 2016 at 2:25 PM, Brandon Williams <bmw...@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 <sbel...@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.
