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

[Duy Nguyen <pclouds@gmail.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.

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

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

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