git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: git submodules implementation question

From
USUma Srinivasan <usrinivasan@twitter.com>
Date
Sep 1, 2016, 01:04 UTC
Message-ID
<CAN5XQfth-MLXOG0RjtJZ=4HZf2km3TgQ=4A88Wa7cOuRBgpi_g@mail.gmail.com>
In-Reply-To
<CAN5XQfujk2HpRGCeeGgDeeHJV3amEX=gSGva0Zot6LfEBv4CVg@mail.gmail.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.
>>
Previous: Uma SrinivasanNext: Junio C Hamano
Message 20 of 31 in “git submodules implementation question”
  1. Uma SrinivasanAug 28, 2016
  2. Junio C HamanoAug 29, 2016
  3. Uma SrinivasanAug 29, 2016
  4. Junio C HamanoAug 29, 2016
  5. Uma SrinivasanAug 29, 2016
  6. Uma SrinivasanAug 29, 2016
  7. Junio C HamanoAug 29, 2016
  8. Uma SrinivasanAug 29, 2016
  9. Jacob KellerAug 30, 2016
  10. Uma SrinivasanAug 30, 2016
  11. Jacob KellerAug 30, 2016
  12. Jacob KellerAug 30, 2016
  13. Uma SrinivasanAug 30, 2016
  14. Junio C HamanoAug 30, 2016
  15. Uma SrinivasanAug 31, 2016
  16. Junio C HamanoAug 31, 2016
  17. Uma SrinivasanAug 31, 2016
  18. Junio C HamanoAug 31, 2016
  19. Uma SrinivasanAug 31, 2016
  20. Uma SrinivasanSep 1, 2016
  21. Junio C HamanoSep 1, 2016
  22. Uma SrinivasanSep 1, 2016
  23. Junio C HamanoSep 1, 2016
  24. Stefan BellerSep 1, 2016
  25. Junio C HamanoSep 1, 2016
  26. Junio C HamanoSep 1, 2016
  27. Junio C HamanoSep 1, 2016
  28. Stefan BellerSep 1, 2016
  29. Junio C HamanoSep 1, 2016
  30. Uma SrinivasanSep 1, 2016
  31. Junio C HamanoSep 1, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.