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

From: Duy Nguyen <pclouds@gmail.com>
Date: Nov 30, 2016, 11:14 UTC
Message-ID: <CACsJy8Ce3Oa-xJ4BwgRRy6neM=Jxkfqq7yboHZDXLDG2tu9GzQ@mail.gmail.com>
In-Reply-To: <20161122192235.6055-5-sbeller@google.com>

On Wed, Nov 23, 2016 at 2:22 AM, Stefan Beller <sbeller@google.com> wrote:

Show 7 quoted lines

> +/*
> + * 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. The first one is this. "git worktree move" on a worktree that contains submodules .git also benefits from something like this [1]. I suggest you move this function to some neutral place and maybe rename it to relocate_gitdir() or something.

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.

[1] https://public-inbox.org/git/20161128094319.16176-11-pclouds@gmail.com/T/#u

Show 16 quoted lines

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

Show 14 quoted lines

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

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