Re: [PATCH v2 4/5] bundle-uri: add support for http(s):// and file://
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Aug 4, 2022, 15:34 UTC
- Message-ID
- <9afd5eb2-44a0-6342-6006-5dbdefba9947@github.com>
- In-Reply-To
- <xmqq8ro6fi1j.fsf@gitster.g>
On 8/2/2022 5:32 PM, Junio C Hamano wrote:
Show 25 quoted lines
> "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.)
Show 13 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://"?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