Re: [PATCH v2 2/8] submodule--helper: get remote names from any repository

> Atharva Raykar <raykar.ath@gmail.com> writes:
>
>> `get_default_remote()` retrieves the name of a remote by resolving the
>> refs from of the current repository's ref store.
>>
>> Thus in order to use it for retrieving the remote name of a submodule,
>> we have to start a new subprocess which runs from the submodule
>> directory.
>>
>> Let's instead introduce a function called `repo_get_default_remote()`
>> which takes any repository object and retrieves the remote accordingly.
>>
>> `get_default_remote()` is then defined as a call to
>> `repo_get_default_remote()` with 'the_repository' passed to it.
>>
>> Now that we have `repo_get_default_remote()`, we no longer have to start
>> a subprocess that called `submodule--helper get-default-remote` from
>> within the submodule directory.
>>
>> So let's make a function called `get_default_remote_submodule()` which
>> takes a submodule path, and returns the default remote for that
>> submodule, all within the same process.
>>
>> We can now use this function to save an unnecessary subprocess spawn in
>> `sync_submodule()`, and also in the next patch, which will require this
>> functionality.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Shourya Shukla <periperidip@gmail.com>
>> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
>> ---
>>  builtin/submodule--helper.c | 34 +++++++++++++++++++++++-----------
>>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> The above covers a lot of stuff, because this change does a lot of
> things ;-)
>
> The commit could be split into 3 logically distinct phases (two
> independent, the third depends on the other two):
>
>  - extract the part that uses the run-command() API to run
>    "submodule--helper print-default-remote" from sync_submodule()
>    and create get_default_remote_submodule() function out of it.
>
>  - move bulk of get_default_remote() into repo_get_default_remote()
>    and reimplement the former as a thin wrapper of the latter.
>
>  - using the repo_get_default_remote() created in the second step,
>    update the get_default_remote_submodule() created in the first
>    step to work in-process without a subprocess.
>
> but a bit larger granularity used here is probably OK.
>

>>  	strbuf_reset(&sb);
>> -	if (capture_command(&cp, &sb, 0))
>> +	strbuf_addstr(&sb, get_default_remote_submodule(path));
>> +	if (!sb.buf)
>>  		die(_("failed to get the default remote for submodule '%s'"),
>>  		      path);
>
> There is this line after the post context presented here:
>
>         strbuf_strip_suffix(&sb, "\n");
>         remote_key = xstrfmt("remote.%s.url", sb.buf);
>
> The LF was expected to come from the capture_command(), but it no
> longer is needed after get_default_remote_submodule() switches to
> use the in-process method.  In fact, the use of sb for the purpose
> of forming remote_key is not needed.
>
>     tmp = get_default_remote_submodule(path);
>     if (!tmp)
> 	die(_("failed to get..."));
>     remote_key = xstrfmt("remote.%s.url", tmp);
>     free(tmp);
>
> Yes, I think the new code leaks, and makes it impossible not to
> leak, the returned value from get_default_remote_submodule() by
> passing the call as an argument to strbuf_addstr().