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

From: Stefan Beller <sbeller@google.com>
Date: Dec 8, 2016, 18:55 UTC
Message-ID: <CAGZ79kYtEUvuTX09sJm3C0rG0-BrBz4bN0FCs6E5d2jHhtKN6w@mail.gmail.com>
In-Reply-To: <CACsJy8Bs78ywGq5p6yAFGi1KACAXFEeroyQSJye5-RL5gqOS+Q@mail.gmail.com>

On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 10 quoted lines

> 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)" ?

Show 7 quoted lines

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

Show 30 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.

ok I'll add a TODO/emptyfunction for that.

Thanks for the review! Stefan