Re: [GSoC] [PATCH v4 1/8] submodule--helper: add options for compute_submodule_clone_url()

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Aug 8, 2021, 17:41 UTC
Message-ID: <0752736f-11d5-103b-653f-a4bbe6436304@gmail.com>
In-Reply-To: <20210807071613.99610-2-raykar.ath@gmail.com>

On 07/08/21 12:46 pm, Atharva Raykar wrote:

Show 9 quoted lines

> Let's modify the interface to `compute_submodule_clone_url()` function
> by adding two more arguments, so that we can reuse this in various parts
> of `submodule--helper.c` that follow a common pattern, which is--read
> the remote url configuration of the superproject and then call
> `relative_url()`.
> 
> This function is nearly identical to `resolve_relative_url()`, the only
> difference being the extra warning message. We can add a quiet flag to
> it, ...

It took me a while to figure what "it" meant in the above sentence. Does it refer to `compute_submodule_clone_url` or `resolve_relative_url`. After one sees the patch and takes a look at `resolve_relative_url`, it's clear the "it" indeed does refer to `resolve_relative_url`. But it might worth clarifying this in the commit message itself.

Certainly not worth a re-roll on its own. May be Junio could amend this while queing ?

Show 10 quoted lines

> ...  to suppress that warning when not needed, and then refactor
> `resolve_relative_url()` by using this function, something we will do in
> the next patch.
> 
> Having this functionality factored out will be useful for converting the
> rest of `submodule add` in subsequent patches.
> 
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <periperidip@gmail.com>

Otherwise, this looks good.

-- 
Sivaraam