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

From: Stefan Beller <sbeller@google.com>
Date: Jul 20, 2017, 19:57 UTC
Message-ID: <CAGZ79kaQA+_qEy-wQOoW+XCHSsDx_bt2tKZi1=+CFS6BF1A8Og@mail.gmail.com>
In-Reply-To: <CAME+mvV9-spX0DrToZdtydhZ5cXA5A7R7JCca4f87Rn3tdLUpw@mail.gmail.com>

On Thu, Jul 20, 2017 at 12:36 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:

Show 20 quoted lines

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

Well that was more thinking out out, in the sense of explaining why it is the correct thing to do.

As the recursion is handled via spawning processes, each process has the_repository pointing at a different repository (and the correct repository for each process), at least to my understanding.

Show 15 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?

Well, then we may run into segfaults due to dereferencing a NULL pointer. So we have to figure out, what the code actually does when there is no URL set. According to my understanding this would

    url=$(git config -f .gitmodules --get submodule."$name".url)
    # second case, but empty vars:
    sub_origin_url="$url"
    super_config_url="$url"

    ....

The issue with this shell script is that there is no difference between "" and NULL, so the place where we do

    sub_origin_url="$url"
    super_config_url="$url"

we would need to translate NULL -> empty string

Show 34 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 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".

Ah yes, that is correct.

>
> Thanks,
> Prathamesh Chavan