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

> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> > 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.
> 
> Unrelated side question: What would you think of "variable context line
> configuration" ? e.g. you could configure it to include anything from
> up that line
> that is currently shown after the @@ which is the function signature line.
> 
> As to the add_head_info/resolve_ref_unsafe what impact does that have?
> It produces a wrong head info but AFAICT it will never die(), such that for the
> purposes of this series (which only wants to know if a submodule uses the
> worktree feature) it should be fine.
> 
> It is highly misleading though for others to build upon this.
> So maybe I'll only add the functionality internally in worktree.c
> and document why the values are wrong, and only expose the
> "int submodule_uses_worktrees(const char *path)" ?
> 
> >> @@ -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.
> 
> It actually works correctly: "No submodule is the current worktree".
> 
> 
> >
> >>  }
> >>
> >> +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.
> 
> ok I'll add a TODO/emptyfunction for that.