On Fri, Dec 9, 2016 at 3:00 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> 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?
>
Well it depends on the question that you are trying to answer:
Assume we introduce a new repository format in Git version 3.0.
By some means the submodule repository is converted to the new format,
but the superproject is not. (e.g. it is auto migrating repositories that have
a lot of large blobs/files)
Now for some reason you use the older git 2.X in the superproject.
(e.g. the code is on a shared network mount, or git3 has a bug, or ...)
Then the code above may tell that the submodule doesn't use worktrees
(as we cannot make any assumptions on the new crazy repository format),
but in fact it is, so in this case we technically we would need to:
1) read the config
2) check the repository format version (if larger than we know of,
assume the worst or just die?)
As the function in this patch is used for safe guarding we may want to be
extra cautious, another function that is using resolve_gitdir may have other
assumptions on what is ok even for newer repository formats.