Re: [GSoC][PATCH 5/8] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan <pc44800@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.

>> +       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?

>
>> +
>> +       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?

>> +
>> +       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 set

Thanks 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