Re: [PATCH v2 4/5] bundle-uri: add support for http(s):// and file://
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 4, 2022, 18:19 UTC
- Message-ID
- <xmqqsfmb50s0.fsf@gitster.g>
- In-Reply-To
- <9afd5eb2-44a0-6342-6006-5dbdefba9947@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
Show 8 quoted lines
>>> + 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://"))
This part (i.e. check for "<scheme>://", not "<scheme>:") still stands, as the latter could be an scp style Git URL that goes to the host whose name is <scheme>. As mentioned later, s/istarts/starts/ is probably a good thing to do here.
Show 12 quoted lines
>> 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.
Guarding them to lowercase-only may sound like a cop-out to purists, but I think it is reasonable thing to do. The only folks that would be offended by are protocol lawyers, and as your check shows, we are treating <scheme> case sensitively already.
An obvious alternative is to downcase the "<scheme>://" part but I do not think it is worth it; we have to do that everywhere and we need to be confident that we covered all the code paths---the latter is expensive.
There do exist skip_iprefix() that are used in many places, like convert, http, mailinfo, etc. by the way. That is a moot point as we are not doing case insensitive comparison anymore.
Thanks.