Re: [RFC PATCHv2 09/17] update submodules: add scheduling to update submodules

[Stefan Beller <sbeller@google.com>]

On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@twosigma.com> wrote:
> This patch confuses me -- see below.
>
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Friday, December 02, 2016 7:30 PM
>> To: bmwill@google.com; David Turner
>> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
>> gitster@pobox.com; Stefan Beller
>> Subject: [RFC PATCHv2 09/17] update submodules: add scheduling to update
>> submodules
> [snip]
>> +static int update_submodule(const char *path, const struct object_id
>> *oid,
>> +                         int force, int is_new)
>> +{
>> +     const char *git_dir;
>> +     struct child_process cp = CHILD_PROCESS_INIT;
>> +     const struct submodule *sub = submodule_from_path(null_sha1, path);
>> +
>> +     if (!sub || !sub->name)
>> +             return -1;
>> +
>> +     git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
>> +
>> +     if (!git_dir)
>> +             return -1;
>> +
>> +     if (is_new)
>> +             connect_work_tree_and_git_dir(path, git_dir);
>> +
>> +     /* update index via `read-tree --reset sha1` */
>> +     argv_array_pushl(&cp.args, "read-tree",
>> +                                force ? "--reset" : "-m",
>> +                                "-u", sha1_to_hex(oid->hash), NULL);
>> +     prepare_submodule_repo_env(&cp.env_array);
>> +     cp.git_cmd = 1;
>> +     cp.no_stdin = 1;
>> +     cp.dir = path;
>> +     if (run_command(&cp)) {
>> +             warning(_("reading the index in submodule '%s' failed"),
>> path);
>
> The error is not (usually) in "reading the index" -- it's "updating the index" (or the working tree)
>
>> +             child_process_clear(&cp);
>> +             return -1;
>> +     }
>> +
>> +     /* write index to working dir */
>> +     child_process_clear(&cp);
>> +     child_process_init(&cp);
>> +     argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
>
> I'm confused -- doesn't read-tree -u already do this?  And if not, shouldn't we back out the result of the read-tree, to leave the submodule as it was?
>
>> +     cp.git_cmd = 1;
>> +     cp.no_stdin = 1;
>> +     cp.dir = path;
>> +     if (force)
>> +             argv_array_push(&cp.args, "-f");
>> +
>> +     if (run_command(&cp)) {
>> +             warning(_("populating the working directory in submodule '%s'
>> failed"), path);
>> +             child_process_clear(&cp);
>> +             return -1;
>> +     }
>> +
>> +     /* get the HEAD right */
>> +     child_process_clear(&cp);
>> +     child_process_init(&cp);
>> +     argv_array_pushl(&cp.args, "checkout", "--recurse-submodules",
>> NULL);
>
>
> Why are we running checkout on the submodule when we've already done most of the checkout? The only thing left is to set HEAD and recurse, right?  I must be missing something.
>

Yes this is only used to set the HEAD correctly and then recurse down.
I tried to remove the first 2 calls to ch8ild processes at one point in time,
which did not work out.  I should have written in the commit message why
that was a problem. So I'll redo that just to see the problem and improve
the commit message.