Re: [PATCHv2 4/4] submodule: add embed-git-dir function

[Stefan Beller <sbeller@google.com>]

On Wed, Nov 30, 2016 at 3:14 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Nov 23, 2016 at 2:22 AM, Stefan Beller <sbeller@google.com> wrote:
>> +/*
>> + * Migrate the given submodule (and all its submodules recursively) from
>> + * having its git directory within the working tree to the git dir nested
>> + * in its superprojects git dir under modules/.
>> + */
>> +void migrate_submodule_gitdir(const char *prefix, const char *path,
>> +                             int recursive)
>
> Submodules and worktrees seem to have many things in common.

Yes. :)

> The first
> one is this. "git worktree move" on a worktree that contains
> submodules .git also benefits from something like this [1].

That patch is a sensible approach. :)
(By checking all files to not be submodules a worktree would not run
into problems like
a127331cd81, mv: allow moving nested submodules)

> I suggest
> you move this function to some neutral place and maybe rename it to
> relocate_gitdir() or something.

ok tell me where this neutral place is found?
(I'd prefer to not clobber it into cache.h *the* most neutral place in git)
Maybe dir.{c,h} ?

>
> It probably should take a bit flag instead of "recursive" here. One
> thing I would need is the ability to tell this function "I have moved
> all these .git dirs already (because I move whole worktree in one
> operation), here are the old and new locations of them, fix them up!".
> In other words, no rename() could be optionally skipped.

In the non-main working trees you'd also have a .git file linking
to the actual git dir and you'd only have to fix them up instead of moving.


>
> [1] https://public-inbox.org/git/20161128094319.16176-11-pclouds@gmail.com/T/#u
>
>> +{
>> +       char *old_git_dir;
>> +       const char *new_git_dir;
>> +       const struct submodule *sub;
>> +
>> +       old_git_dir = xstrfmt("%s/.git", path);
>> +       if (read_gitfile(old_git_dir))
>> +               /* If it is an actual gitfile, it doesn't need migration. */
>> +               goto out;
>> +
>> +       sub = submodule_from_path(null_sha1, path);
>> +       if (!sub)
>> +               die(_("Could not lookup name for submodule '%s'"),
>> +                     path);
>> +
>> +       new_git_dir = git_common_path("modules/%s", sub->name);
>
> Why doesn't git_path() work here? This would make "modules" shared
> between worktrees, even though it's not normally. That inconsistency
> could cause trouble.

I thought that was a long term goal?
(I actually think about reviving the series you sent out a few weeks ago
to make worktree and submodules work well together)

So for that we'd want to have at least the object store shared across all
worktrees.

>
>> +       if (safe_create_leading_directories_const(new_git_dir) < 0)
>> +               die(_("could not create directory '%s'"), new_git_dir);
>> +
>> +       if (!prefix)
>> +               prefix = get_super_prefix();
>> +       printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
>> +               prefix ? prefix : "", path,
>> +               real_path(old_git_dir), 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);
>
> Another thing in common is, both submodules and worktrees use some
> form of textual symlinks. You need to fix up some here. But if this
> submodule has multiple worktreee, there may be some "symlinks" in
> .git/worktrees which would need fixing up as well.

We could signal that via one of the flag bits?
(e.g. FIXUP_WORKTREE_SYMLINKS )

>
> You don't have to do the fix up thing right away, but I think we
> should at least make sure we leave no dangling links behind (by
> die()ing early if we find a .git dir we can't handle yet)
> --
> Duy