On 12/08, Stefan Beller wrote:
> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@xxxxxxxxxx> 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.
So is using resolve_gitdir not ok when trying to see if a submodule has
a gitdir at a particular path?
--
Brandon Williams