Re: git submodules implementation question

[Uma Srinivasan <usrinivasan@twitter.com>]

Here's one more attempt where I changed prepare_submodule_repo_env()
to take the submodule git dir as an argument.
Sorry for including this long code diff inline. If there's a better
way to have this discussion with code please let me know.

Thanks,
Uma

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9d79f19..0741f6c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -465,7 +465,7 @@ static int clone_submodule(const char *path, const
char *gitdir, const char *url
        argv_array_push(&cp.args, path);

        cp.git_cmd = 1;
-       prepare_submodule_repo_env(&cp.env_array);
+       prepare_submodule_repo_env(&cp.env_array, gitdir);
        cp.no_stdin = 1;

        return run_command(&cp);
diff --git a/submodule.c b/submodule.c
index 5a62aa2..3d9174a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -522,11 +522,31 @@ static int has_remote(const char *refname, const
struct object_id *oid,
        return 1;
 }

+static const char *submodule_git_dir = NULL;
+const char *get_submodule_git_dir(const char *path, struct strbuf *bufptr)
+{
+       strbuf_addf(bufptr, "%s/.git", path);
+       submodule_git_dir = read_gitfile(bufptr->buf);
+       if (!submodule_git_dir)
+               submodule_git_dir = bufptr->buf;
+       if (!is_directory(submodule_git_dir)) {
+               return NULL;
+       }
+       return submodule_git_dir;
+}
+
 static int submodule_needs_pushing(const char *path, const unsigned
char sha1[20])
 {
        if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
                return 0;

+       struct strbuf gitdirbuf = STRBUF_INIT;
+        const char *sm_gitdir = get_submodule_git_dir(path, &gitdirbuf);
+       if (sm_gitdir == NULL) {
+               strbuf_release(&gitdirbuf);
+               return 0;
+       }
        if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
                struct child_process cp = CHILD_PROCESS_INIT;
                const char *argv[] = {"rev-list", NULL, "--not",
"--remotes", "-n", "1" , NULL};
@@ -535,7 +555,7 @@ static int submodule_needs_pushing(const char
*path, const unsigned char sha1[20

                argv[1] = sha1_to_hex(sha1);
                cp.argv = argv;
-               prepare_submodule_repo_env(&cp.env_array);
+               prepare_submodule_repo_env(&cp.env_array, sm_gitdir);
                cp.git_cmd = 1;
                cp.no_stdin = 1;
                cp.out = -1;
@@ -551,6 +571,7 @@ static int submodule_needs_pushing(const char
*path, const unsigned char sha1[20
                return needs_pushing;
        }

+       strbuf_release(&gitdirbuf);
        return 0;
 }

@@ -617,12 +638,18 @@ static int push_submodule(const char *path)
        if (add_submodule_odb(path))
                return 1;

+       struct strbuf gitdirbuf = STRBUF_INIT;
+        const char *sm_gitdir = get_submodule_git_dir(path, &gitdirbuf);
+       if (sm_gitdir == NULL) {
+               strbuf_release(&gitdirbuf);
+               return 0;
+       }
        if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
                struct child_process cp = CHILD_PROCESS_INIT;
                const char *argv[] = {"push", NULL};

                cp.argv = argv;
-               prepare_submodule_repo_env(&cp.env_array);
+               prepare_submodule_repo_env(&cp.env_array, sm_gitdir);
                cp.git_cmd = 1;
                cp.no_stdin = 1;
                cp.dir = path;
@@ -631,6 +658,7 @@ static int push_submodule(const char *path)
                close(cp.out);
        }

+       strbuf_release(&gitdirbuf);
        return 1;
 }

@@ -665,10 +693,16 @@ static int is_submodule_commit_present(const
char *path, unsigned char sha1[20])
                struct child_process cp = CHILD_PROCESS_INIT;
                const char *argv[] = {"rev-list", "-n", "1", NULL,
"--not", "--all", NULL};
                struct strbuf buf = STRBUF_INIT;
+               struct strbuf gitdirbuf = STRBUF_INIT;
+               const char *sm_gitdir = get_submodule_git_dir(path, &gitdirbuf);
+               if (sm_gitdir == NULL) {
+                       strbuf_release(&gitdirbuf);
+                       return 0;
+               }

                argv[3] = sha1_to_hex(sha1);
                cp.argv = argv;
-               prepare_submodule_repo_env(&cp.env_array);
+               prepare_submodule_repo_env(&cp.env_array, sm_gitdir);
                cp.git_cmd = 1;
                cp.no_stdin = 1;
                cp.dir = path;
@@ -676,6 +710,7 @@ static int is_submodule_commit_present(const char
*path, unsigned char sha1[20])
                        is_present = 1;

                strbuf_release(&buf);
+               strbuf_release(&gitdirbuf);
        }
        return is_present;
 }
@@ -851,7 +886,7 @@ static int get_next_submodule(struct child_process *cp,
                if (is_directory(git_dir)) {
                        child_process_init(cp);
                        cp->dir = strbuf_detach(&submodule_path, NULL);
-                       prepare_submodule_repo_env(&cp->env_array);
+                       prepare_submodule_repo_env(&cp->env_array, git_dir);
                        cp->git_cmd = 1;
                        if (!spf->quiet)
                                strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -958,15 +993,14 @@ unsigned is_submodule_modified(const char *path,
int ignore_untracked)
                strbuf_release(&buf);
                /* The submodule is not checked out, so it is not modified */
                return 0;
-
        }
-       strbuf_reset(&buf);

        if (ignore_untracked)
                argv[2] = "-uno";

        cp.argv = argv;
-       prepare_submodule_repo_env(&cp.env_array);
+       prepare_submodule_repo_env(&cp.env_array, git_dir);
+       strbuf_reset(&buf);
        cp.git_cmd = 1;
        cp.no_stdin = 1;
        cp.out = -1;
@@ -1023,11 +1057,11 @@ int submodule_uses_gitfile(const char *path)
                strbuf_release(&buf);
                return 0;
        }
-       strbuf_release(&buf);

        /* Now test that all nested submodules use a gitfile too */
        cp.argv = argv;
-       prepare_submodule_repo_env(&cp.env_array);
+       prepare_submodule_repo_env(&cp.env_array, git_dir);
+       strbuf_release(&buf);
        cp.git_cmd = 1;
        cp.no_stdin = 1;
        cp.no_stderr = 1;
@@ -1052,6 +1086,7 @@ int ok_to_remove_submodule(const char *path)
        };
        struct strbuf buf = STRBUF_INIT;
        int ok_to_remove = 1;
+       const char *git_dir;

        if (!file_exists(path) || is_empty_dir(path))
                return 1;
@@ -1060,7 +1095,10 @@ int ok_to_remove_submodule(const char *path)
                return 0;

        cp.argv = argv;
-       prepare_submodule_repo_env(&cp.env_array);
+       strbuf_addf(&buf, "%s/.git", path);
+       git_dir = read_gitfile(buf.buf);
+       prepare_submodule_repo_env(&cp.env_array, git_dir);
+       strbuf_release(&buf);
        cp.git_cmd = 1;
        cp.no_stdin = 1;
        cp.out = -1;
@@ -1272,12 +1310,16 @@ int parallel_submodules(void)
        return parallel_jobs;
 }

-void prepare_submodule_repo_env(struct argv_array *out)
+void prepare_submodule_repo_env(struct argv_array *out, const char* git_dir)
 {
        const char * const *var;

        for (var = local_repo_env; *var; var++) {
                if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
                        argv_array_push(out, *var);
+               if (strcmp(*var, GIT_DIR_ENVIRONMENT))
+                       argv_array_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+                                        real_path(git_dir));
        }
+
 }
diff --git a/submodule.h b/submodule.h
index d9e197a..4f8b0c7 100644
--- a/submodule.h
+++ b/submodule.h
@@ -73,6 +73,6 @@ int parallel_submodules(void);
  * a submodule by clearing any repo-specific envirionment variables, but
  * retaining any config in the environment.
  */
-void prepare_submodule_repo_env(struct argv_array *out);
+void prepare_submodule_repo_env(struct argv_array *out, const char *git_dir);

On Wed, Aug 31, 2016 at 11:58 AM, Uma Srinivasan
<usrinivasan@twitter.com> wrote:
>> We want to affect only the process we are going to spawn to work
>> inside the submodule, not ourselves, which is what this call does;
>> this does not sound like a good idea.
>
> Okay, in that case I would have to pass the "git_dir" as a new
> argument to prepare_submodule_repo_env(). I know what to pass from the
> is_submodule_modified() caller. I don't think it's all that obvious
> for the other callers.
>
> Thanks,
> Uma
>
> On Wed, Aug 31, 2016 at 11:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Uma Srinivasan <usrinivasan@twitter.com> writes:
>>
>>> diff --git a/submodule.c b/submodule.c
>>> index 5a62aa2..23443a7 100644
>>> --- a/submodule.c
>>> +++ b/submodule.c
>>> @@ -960,6 +960,9 @@ unsigned is_submodule_modified(const char *path,
>>> int ignore_untracked)
>>>                 return 0;
>>>
>>>         }
>>> +       /* stuff submodule git dir into env var */
>>> +       set_git_dir(git_dir);
>>
>> We want to affect only the process we are going to spawn to work
>> inside the submodule, not ourselves, which is what this call does;
>> this does not sound like a good idea.
>>