submodule embedgitdirs

The current caller of connect_work_tree_and_git_dir passes
an absolute path for the git_dir parameter. In the future patch we will also pass in relative path for git_dir. Extend the functionality of connect_work_tree_and_git_dir to take relative paths for parameters.

We could work around this in the future patch by computing the absolute path for the git_dir in the calling site, however accepting relative paths for either parameter makes the API for this function much harder to misuse.

Signed-off-by Stefan Beller <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 submodule.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

submodule.c +0 -0

index 6f7d883de9..66c5ce5a24 100644

submodule.c +6 -5

+       char *real_work_tree = xstrdup(real_path(work_tree));
 
        /* Update gitfile */
        strbuf_addf(&file_name, "%s/.git", work_tree);
        write_file(file_name.buf, "gitdir: %s",
-                  relative_path(git_dir, real_work_tree, &rel_path));
+                  relative_path(real_git_dir, real_work_tree, &rel_path));
 
        /* Update core.worktree setting */
        strbuf_reset(&file_name);
-       strbuf_addf(&file_name, "%s/config", git_dir);
+       strbuf_addf(&file_name, "%s/config", real_git_dir);
        git_config_set_in_file(file_name.buf, "core.worktree",
-                              relative_path(real_work_tree, git_dir,
+                              relative_path(real_work_tree, real_git_dir,
                                             &rel_path));
 
        strbuf_release(&file_name);
        strbuf_release(&rel_path);
-       free((void *)real_work_tree);
+       free(real_work_tree);
+       free(real_git_dir);
 }
 
 int parallel_submodules(void)
--

2.11.0.rc2.30.gc512cbd.dirty

Just like main commands in Git, the submodule helper needs access to the superproject prefix. Enable this in the git.c but have its own fuse in the helper code by having a flag to turn on the super prefix.

Signed-off-by Stefan Beller <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
 git.c                       |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

builtin/submodule--helper.c +0 -0

index 4beeda5f9f..33676a57cf 100644

builtin/submodule--helper.c +2 -0

                        return commands[i].fn(argc - 1, argv + 1, prefix);
+               }
+       }
 
        die(_("'%s' is not a valid submodule--helper "
              "subcommand"), argv[1]);

git.c +0 -0

index efa1059fe0..98dcf6c518 100644

unsigned int is probably safer for variables that are used as bit-flags.

If it's meant for users to see, please _() the string.

Quoted 1 line

+ { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},

The same macro defined twice in two separate .c files? Hmm.. it confused me a bit because i thought there was a connection.. I guess it's ok.

worktree.c +7 -0

 }
 
+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)
+{

worktree.c +6 -0

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

Review static struct worktree *get_main_worktree(void)

Quoted 4 lines · Stefan Beller

        worktree = xcalloc(1, sizeof(*worktree));
        worktree->path = strbuf_detach(&worktree_path, NULL);

(quoted diff) +0 -0

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

worktree.c struct worktree **get_worktrees(unsigned flags) · line 211

Quoted 2 lines

worktree.c +0 -0

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

Quoted 10 lines

worktree.c +7 -0

 }
 
+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)
+{

worktree.c struct worktree **get_worktrees(unsigned flags) · line 230

Quoted 10 lines

worktree.c +10 -0

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

Quoted 10 lines · Duy Nguyen

Review static struct worktree *get_main_worktree(void)

Quoted 4 lines · Stefan Beller

        worktree = xcalloc(1, sizeof(*worktree));
        worktree->path = strbuf_detach(&worktree_path, NULL);

(quoted diff) +0 -0

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

Quoted 10 lines · Duy Nguyen

Review static struct worktree *get_main_worktree(void)

Quoted 4 lines · Stefan Beller

        worktree = xcalloc(1, sizeof(*worktree));
        worktree->path = strbuf_detach(&worktree_path, NULL);

(quoted diff) +0 -0

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

Quoted 7 lines

worktree.c struct worktree **get_worktrees(unsigned flags) · line 211

Quoted 2 lines

worktree.c +0 -0

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

Quoted 29 lines

Quoted 10 lines

worktree.c +7 -0

 }
 
+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)
+{

worktree.c struct worktree **get_worktrees(unsigned flags) · line 230

Quoted 10 lines

worktree.c +10 -0

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

Quoted 1 line

Kinda off-topic though. Feel free to ignore the above comment.

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

Quoted 67 lines · Stefan Beller

Quoted 10 lines · Duy Nguyen

Review static struct worktree *get_main_worktree(void)

Quoted 4 lines · Stefan Beller

        worktree = xcalloc(1, sizeof(*worktree));
        worktree->path = strbuf_detach(&worktree_path, NULL);

(quoted diff) +0 -0

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

Quoted 7 lines

worktree.c struct worktree **get_worktrees(unsigned flags) · line 211

Quoted 2 lines

worktree.c +0 -0

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

Quoted 29 lines

Quoted 10 lines

worktree.c +7 -0

 }
 
+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)
+{

worktree.c struct worktree **get_worktrees(unsigned flags) · line 230

Quoted 10 lines

worktree.c +10 -0

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

Quoted 3 lines

Quoted 1 line

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?

Signed-off-by Stefan Beller <[email protected]>

---
 worktree.c | 24 ++++++++++++++++++++++++
 worktree.h |  7 +++++++
 2 files changed, 31 insertions(+)

worktree.c +0 -0

index 75db689672..2559f33846 100644
@@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,
 

worktree.c +12 -0

+{
+       return uses_worktree_internal(get_worktrees(0));
+}
+
+int submodule_uses_worktrees(const char *path)
+{
+       struct worktree **worktrees = get_submodule_worktrees(path, 0);
+       if (!worktrees)
+               return 0;
+
+       return uses_worktree_internal(worktrees);
+}

worktree.h +7 -1

index 157fbc4a66..76027b1fd2 100644
@@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
 extern struct worktree **get_submodule_worktrees(const char *path,
                                                 unsigned flags);
 
+/*
+ * Returns 1 if more than one worktree exists.
+ * Returns 0 if only the main worktree exists.
+ */
+extern int uses_worktrees(void);
+extern int submodule_uses_worktrees(const char *path);
+
 /*
  * Return git dir of the worktree. Note that the path may be relative.
  * If wt is NULL, git dir of current worktree is returned.
--

2.11.0.rc2.30.gc512cbd.dirty

Alternatively, we could add a new flag to get_worktrees() to tell it to return all worktrees if there is a least one linked worktree, or return NULL if there's only main worktree. I'm not sure if this is clever or very stupid.

Quoted 4 lines · Duy Nguyen

Alternatively, we could add a new flag to get_worktrees() to tell it to return all worktrees if there is a least one linked worktree, or return NULL if there's only main worktree. I'm not sure if this is clever or very stupid.

No, this may be better. Add a flag to say "returns linked worktrees _only_". Which means when you're in a "normal" repo, get_worktrees() with this flag returns NULL. When you're in a multiple-worktree repo, it returns all linked worktrees (no main worktree). I think I might have a use for this flag in addition to this uses_worktrees() here. uses_worktrees() look quite simple with that flag

int uses_worktrees(void)

{
    struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);
    int retval = worktrees != NULL;

dir.c +2 -0

index 8b74997c66..cc5729f733 100644
@@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
        free(real_work_tree);
        free(real_git_dir);
 }
+
+/*