On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@xxxxxxxxxxxx> wrote: > This patch confuses me -- see below. > >> -----Original Message----- >> From: Stefan Beller [mailto:sbeller@xxxxxxxxxx] >> Sent: Friday, December 02, 2016 7:30 PM >> To: bmwill@xxxxxxxxxx; David Turner >> Cc: git@xxxxxxxxxxxxxxx; sandals@xxxxxxxxxxxxxxxxxxxx; hvoigt@xxxxxxxxxx; >> gitster@xxxxxxxxx; 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.