Re: [PATCHv8 6/6] submodule: add absorb-git-dir function

From: Stefan Beller <sbeller@google.com>
Date: Dec 19, 2016, 18:15 UTC
Message-ID: <CAGZ79kYPEiUGXR-qTbbHzaeOwbHH88mdx7GP8QX2Ff1bypcrwQ@mail.gmail.com>
In-Reply-To: <20161219053507.GA2335@duynguyen.vn.dektech.internal>

On Sun, Dec 18, 2016 at 9:35 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 23 quoted lines

> On Mon, Dec 12, 2016 at 11:04:35AM -0800, Stefan Beller wrote:
>> diff --git a/dir.c b/dir.c
>> index e0efd3c2c3..d872cc1570 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2773,3 +2773,15 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>>       free(work_tree);
>>       free(git_dir);
>>  }
>> +
>> +/*
>> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
>> + */
>> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
>> +{
>> +     if (rename(old_git_dir, new_git_dir) < 0)
>> +             die_errno(_("could not migrate git directory from '%s' to '%s'"),
>> +                     old_git_dir, new_git_dir);
>> +
>> +     connect_work_tree_and_git_dir(path, new_git_dir);
>
> Should we worry about recovering (e.g. maybe move new_git_dir back to
> old_git_dir) if this connect_work_tree_and_git_dir() fails?

What if the move back fails?

Show 9 quoted lines

>
> Both write_file() and git_config_set_.. in this function may die(). In
> such a case the repo is in broken state and the user needs pretty good
> submodule understanding to recover from it, I think.
>
> Recovering is not easy (nor entirely safe) either, though I suppose if
> we keep original copies for modified files, then we could restore them
> after moving the directory back and pray the UNIX gods that all
> operations succeed.

There are different levels of brokenness available. I just tried what happens if core.worktree is unset in a submodule and that seems to not impact git operations (I only tested git-status both in the superproject as well as in the submodule).

So I wonder why we set core.worktree at all here as it doesn't seem to be needed.

Which means that the move of the git directory as well as the .git file update to point at that moved directory are the important things to get right.

So maybe:

1) rename the git dir or die
2) write the new gitlink
    If that fails remove the .git file (if it exists partially or empty)
    and undo 1) by calling rename again with swapped arguments
    and then die
3) set core.worktree
    If that fails, just warn the user