Junio C Hamano <gitster@xxxxxxxxx> writes:
> Atharva Raykar <raykar.ath@xxxxxxxxx> 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@xxxxxxxxx>
>> Mentored-by: Shourya Shukla <periperidip@xxxxxxxxx>
>> Signed-off-by: Atharva Raykar <raykar.ath@xxxxxxxxx>
>> ---
>> 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().
Thanks, this was a good catch.
> Both of the two bugs pointed out here would have been easier to spot
> if the commits were smaller, I would think, but as I said, a bit
> larger granularity used here was still manageable.
My initial series had this patch split up similar to what you suggested,
but I ultimately chose to favour the larger granularity to make the
whole series a bit shorter. This patch definitely towed the line a bit.
> Thanks.
