Re: [GSoC][PATCH 5/8] submodule: port submodule subcommand 'sync' from shell to C
- From
Prathamesh Chavan <pc44800@gmail.com>
- Date
- Jul 20, 2017, 19:36 UTC
- Message-ID
- <CAME+mvV9-spX0DrToZdtydhZ5cXA5A7R7JCca4f87Rn3tdLUpw@mail.gmail.com>
- In-Reply-To
- <CAGZ79kZrVQZH3ASoZavEeCSUBei8By1xs3fAQaEbVSCMOXoYxg@mail.gmail.com>
Firstly, thanks for reviewing my patches. I have even checked out the other reviews and improvised the other patches according to reviews as well. I had a few doubts about this one though.
Show 13 quoted lines
>> + const struct submodule *sub; >> + char *sub_key, *remote_key; >> + char *sub_origin_url, *super_config_url, *displaypath; >> + struct strbuf sb = STRBUF_INIT; >> + struct child_process cp = CHILD_PROCESS_INIT; >> + >> + if (!is_submodule_active(the_repository, list_item->name)) >> + return; > > We can use the_repository here, as we also use child processes to > recurse, such that we always operate on the_repository as the > superproject. >
Sorry, but can you explain this a bit more?
Show 11 quoted lines
>
>> +
>> + sub = submodule_from_path(null_sha1, list_item->name);
>> +
>> + if (!sub || !sub->url)
>> + die(_("no url found for submodule path '%s' in .gitmodules"),
>> + list_item->name);
>
> We do not die in the shell script when the url is missing in the
> .gitmodules file.
>Then to have a faithful conversion, IMO, deleting the above lines would be the correct way?
Show 29 quoted lines
>> +
>> + prepare_submodule_repo_env(&cp.env_array);
>> + cp.git_cmd = 1;
>> + cp.dir = list_item->name;
>> + argv_array_pushl(&cp.args, "submodule--helper",
>> + "print-default-remote", NULL);
>> + if (capture_command(&cp, &sb, 0))
>> + die(_("failed to get the default remote for submodule '%s'"),
>> + list_item->name);
>> +
>> + strbuf_strip_suffix(&sb, "\n");
>> + remote_key = xstrfmt("remote.%s.url", sb.buf);
>> + strbuf_release(&sb);
>> +
>> + child_process_init(&cp);
>> + prepare_submodule_repo_env(&cp.env_array);
>> + cp.git_cmd = 1;
>> + cp.dir = list_item->name;
>> + argv_array_pushl(&cp.args, "config", remote_key, sub_origin_url, NULL);
>> + if (run_command(&cp))
>> + die(_("failed to update remote for submodule '%s'"),
>> + list_item->name);
>
> While it is a strict conversion from the shell script, we could also
> try to do this in-process:
> 1) we'd find out the submodules git dir using submodule_to_gitdir
> 2) construct the path the the config file as "%s/.gitconfig"
> 3) using git_config_set_in_file (which presumably takes file name,
> key and value) the value can be setThanks for pointing that out. That surely reduced a child_process. Although the path of the config file for the case of submodules would be constructed by "%s/config".
Thanks, Prathamesh Chavan