Re: [PATCH v2 4/5] bundle-uri: add support for http(s):// and file://

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2022, 21:32 UTC
Message-ID: <xmqq8ro6fi1j.fsf@gitster.g>
In-Reply-To: <e4f2dcc7a45388663aeac786e5abdcf2164cfe62.1659443384.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> +static int copy_uri_to_file(const char *filename, const char *uri)
> +{
> +	const char *out;
> +
> +	if (istarts_with(uri, "https:") ||
> +	    istarts_with(uri, "http:"))

Let's be a bit more strict to avoid mistakes and make the code immediately obvious, e.g.

	if (istarts_with(uri, "https://") ||
	    istarts_with(uri, "http://"))

> +		return download_https_uri_to_file(filename, uri);
> +
> +	if (!skip_prefix(uri, "file://", &out))
> +		out = uri;

If we are using istarts_with because URI scheme name is case insensitive, shouldn't we do the same for "file://" URL, not just for "http(s)://" URL? IOW

	if (!skip_iprefix(uri, "file://", &out))

Show 9 quoted lines

> +static int download_https_uri_to_file(const char *file, const char *uri)
>  {
> +	int result = 0;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	FILE *child_in = NULL, *child_out = NULL;
> +	struct strbuf line = STRBUF_INIT;
> +	int found_get = 0;
> +
> +	strvec_pushl(&cp.args, "git-remote-https", "origin", uri, NULL);

Does "git-remote-https" talk to a "http://" URL just fine when uri parameter starts with "http://"? Would it be the same if the uri parameter begins with say "Http://"?

Other than that, looks sensible.