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

[Derrick Stolee <derrickstolee@github.com>]

On 8/2/2022 5:32 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +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))

Good ideas. Of course, we don't have a skip_iprefix(), but
I can use "istarts_with()" and then manually add the length.
If we see more need for that in the future, we can consider
adding it.

(It's interesting that these uses in bundle-uri.c are the
only uses of istarts_with() that I see in the codebase.)

>> +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://"?

I did a quick check of our HTTPS tests modifying the HTTPD_PROTO
variable in lib-httpd.sh to "HtTP" and we get this fun error:

+ git clone --filter=blob:limit=0 HtTP://127.0.0.1:5601/smart/server client
Cloning into 'client'...
git: 'remote-HtTP' is not a git command. See 'git --help'.

So I guess I can keep case-sensitive comparisons here.

Thanks,
-Stolee