Re: [PATCH v3 04/10] submodule--helper clone: check for configured submodules using helper

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2017, 18:06 UTC
Message-ID: <xmqq4lyvka1i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170313214341.172676-5-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 20 quoted lines

> -	/*
> -	 * Looking up the url in .git/config.
> -	 * We must not fall back to .gitmodules as we only want
> -	 * to process configured submodules.
> -	 */
> -	strbuf_reset(&sb);
> -	strbuf_addf(&sb, "submodule.%s.url", sub->name);
> -	git_config_get_string(sb.buf, &url);
> -	if (!url) {
> +	/* Check if the submodule has been initialized. */
> +	if (!is_submodule_initialized(ce->name)) {
>  		next_submodule_warn_missing(suc, out, displaypath);
>  		goto cleanup;
>  	}
> @@ -835,7 +827,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  		argv_array_push(&child->args, "--depth=1");
>  	argv_array_pushl(&child->args, "--path", sub->path, NULL);
>  	argv_array_pushl(&child->args, "--name", sub->name, NULL);
> -	argv_array_pushl(&child->args, "--url", url, NULL);
> +	argv_array_pushl(&child->args, "--url", sub->url, NULL);

Even without this patch, we already had an instance of struct submodule available in this function, so the query to .git/config this patch removed was unnecessary?

I am wondering what was meant by the comment "We must not fall back to..." that is being removed---is that because sub->url can come from .gitmodules that is in-tree, not from .git/config? If that is the case, doesn't the change in this hunk change behaviour from using the URL the user prefers to using the URL the upstream suggests, overriding user's configuration?

Show 7 quoted lines

> @@ -845,7 +837,6 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
>  		argv_array_push(&child->args, suc->depth);
>  
>  cleanup:
> -	free(url);
>  	strbuf_reset(&displaypath_sb);
>  	strbuf_reset(&sb);