Re: [PATCHv6 4/7] worktree: get worktrees from submodules

From: Duy Nguyen <pclouds@gmail.com>
Date: Dec 8, 2016, 10:09 UTC
Message-ID: <CACsJy8Bs78ywGq5p6yAFGi1KACAXFEeroyQSJye5-RL5gqOS+Q@mail.gmail.com>
In-Reply-To: <20161208014623.7588-5-sbeller@google.com>

On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:

>
>         worktree = xcalloc(1, sizeof(*worktree));
>         worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)

All the good stuff is outside context lines again.. Somewhere between here we call add_head_info() which calls resolve_ref_unsafe(), which always uses data from current repo, not the submodule we want it to look at.

> @@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
>         return fspathcmp((*a)->path, (*b)->path);

fspathcmp() reads core.ignorecase from current repo. I guess it's insane to have this key different between repos on the same machine, so it should be ok. But I want to point this out just in case I'm missing something.

Show 11 quoted lines

> @@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
>                         if (is_dot_or_dotdot(d->d_name))
>                                 continue;
>
> -                       if ((linked = get_linked_worktree(d->d_name))) {
> +                       if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
>                                 ALLOC_GROW(list, counter + 1, alloc);
>                                 list[counter++] = linked;
>                         }
> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>         return list;

Right before this line is mark_current_worktree(), which in turn calls get_git_dir() and not suitable for peeking into another repository the way submodule code does. get_worktree_git_dir() called within that function shares the same problem.

Show 19 quoted lines

>  }
>
> +struct worktree **get_worktrees(unsigned flags)
> +{
> +       return get_worktrees_internal(get_git_common_dir(), flags);
> +}
> +
> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
> +{
> +       char *submodule_gitdir;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct worktree **ret;
> +
> +       submodule_gitdir = git_pathdup_submodule(path, "%s", "");
> +       if (!submodule_gitdir)
> +               return NULL;
> +
> +       /* the env would be set for the superproject */
> +       get_common_dir_noenv(&sb, submodule_gitdir);

Technically we need to read submodule_gitdir/.config and see if we can understand core.repositoryformatversion, or find any unrecognized extensions. But the problem is not specific to this code. And fixing it is no small task. But perhaps we could call a dummy validate_submodule_gitdir() here? Then when we implement that function for real, we don't have to search the entire code base to see where to put it.

Kinda off-topic though. Feel free to ignore the above comment.

Show 6 quoted lines

> +       ret = get_worktrees_internal(sb.buf, flags);
> +
> +       strbuf_release(&sb);
> +       free(submodule_gitdir);
> +       return ret;
> +}

-- 
Duy