[PATCH] Allow '+', '-' and '.' in remote helper names

According to relevant RFCs, in addition to alphanumerics, the following
characters are valid in URL scheme parts: '+', '-' and '.', but
currently only alphanumerics are allowed in remote helper names.

Allow those three characters in remote helper names (both 'foo://' and
'foo::' syntax).

Signed-off-by: Ilari Liusvaara <redacted>
---
 transport.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/transport.c b/transport.c
index 08e4fa0..00acff1 100644
--- a/transport.c
+++ b/transport.c

@@ -872,6 +872,21 @@ static int is_file(const char *url)
 	return S_ISREG(buf.st_mode);
 }
 
+static int isurlschemechar(int first_flag, int ch)
+{
+	/*
+	 * The set of valid URL schemes, as per STD66 (RFC3986) is
+	 * '[A-Za-z][A-Za-z0-9+.-]*'. But use sightly looser check
+	 * of '[A-Za-z0-9][A-Za-z0-9+.-]*' because earlier version
+	 * of check used '[A-Za-z0-9]+' so not to break any remote
+	 * helpers.
+	 */
+	int alphanumeric, special;
+	alphanumeric = ch > 0 && isalnum(ch);
+	special = ch == '+' || ch == '-' || ch == '.';
+	return alphanumeric || (!first_flag && special);
+}
+
 static int is_url(const char *url)
 {
 	const char *url2, *first_slash;

@@ -896,7 +911,7 @@ static int is_url(const char *url)
 	 */
 	url2 = url;
 	while (url2 < first_slash - 1) {
-		if (!isalnum((unsigned char)*url2))
+		if (!isurlschemechar(url2 == url, (unsigned char)*url2))
 			return 0;
 		url2++;
 	}

@@ -930,7 +945,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	if (url) {
 		const char *p = url;
 
-		while (isalnum(*p))
+		while (isurlschemechar(p == url, *p))
 			p++;
 		if (!prefixcmp(p, "::"))
 			helper = xstrndup(url, p - url);

-- 
1.7.0.86.gb35cab

Hi,

On Tue, 23 Feb 2010, Ilari Liusvaara wrote:

According to relevant RFCs, in addition to alphanumerics, the following
characters are valid in URL scheme parts: '+', '-' and '.', but
currently only alphanumerics are allowed in remote helper names.

May I caution against allowing "+" as part of filenames? On Windows, 
thanks to the DOS garb^Wheritage, "+" is not really allowed...

Or maybe I misunderstood the intent of the patch?

Ciao,
Dscho

Heya,

On Tue, Feb 23, 2010 at 14:03, Johannes Schindelin
[off-list ref] wrote:

May I caution against allowing "+" as part of filenames? On Windows,
thanks to the DOS garb^Wheritage, "+" is not really allowed...

Would it be safe to say "raplce all occurences of '+' and '.' with
'-'? Is it feasible that we would want to support two protocols with a
different helper that map to the same 'name' using that scheme? So,
would there ever be a case where we want to handle "bzr+ssh"
and "bzr-ssh" with a different helper? I reckon not, and if it does
occur it's always possible to put a simple dispatcher in between?

-- 
Cheers,

Sverre Rabbelier

Johannes Schindelin schrieb:

May I caution against allowing "+" as part of filenames? On Windows, 
thanks to the DOS garb^Wheritage, "+" is not really allowed...

I don't think that's true (and I also think it never was).

You may be refering to the copy command, where you can say

   copy a.txt+b.txt+c.txt abc.txt

to concatenate the source files. But this does not restrict how you can
name your files. If you have a file a+b.txt and want to use it with copy,
then you must use double-quotes:

   copy "a+b.txt" elsewhere.txt
   copy "a+b.txt"+c.txt abc.txt

-- Hannes

On Tue, Feb 23, 2010 at 2:19 PM, Johannes Sixt [off-list ref] wrote:

Johannes Schindelin schrieb:

quoted

May I caution against allowing "+" as part of filenames? On Windows,
thanks to the DOS garb^Wheritage, "+" is not really allowed...

I don't think that's true (and I also think it never was).

Wikipedia doesn't seem to think so either:
http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words

-- 
Erik "kusma" Faye-Lund

Hi,

On Tue, 23 Feb 2010, Sverre Rabbelier wrote:

On Tue, Feb 23, 2010 at 14:03, Johannes Schindelin 
[off-list ref] wrote:

quoted

May I caution against allowing "+" as part of filenames? On Windows, 
thanks to the DOS garb^Wheritage, "+" is not really allowed...

Would it be safe to say "raplce all occurences of '+' and '.' with
'-'?

I'd rather only replace '+' with '-'. While "short" DOS filenames can only 
have one dot, and only up to three (upper-case) characters after that, 
"long" ones do not share that restriction.

Is it feasible that we would want to support two protocols with a 
different helper that map to the same 'name' using that scheme? So, 
would there ever be a case where we want to handle "bzr+ssh" and 
"bzr-ssh" with a different helper? I reckon not, and if it does occur 
it's always possible to put a simple dispatcher in between?

Possible.

Ciao,
Dscho

On 02/23/2010 01:33 PM, Ilari Liusvaara wrote:

According to relevant RFCs, in addition to alphanumerics, the following
characters are valid in URL scheme parts: '+', '-' and '.', but
currently only alphanumerics are allowed in remote helper names.

Allow those three characters in remote helper names (both 'foo://' and
'foo::' syntax).

I think '+' could be special-cased in that, for example, "svn+ssh://" 
should still invoke an hypothetic git-remote-svn helper.  There is no 
use yet for this feature, but I'm sure that foreign VCS helpers would 
use it.

Paolo

Hi,

On Tue, 23 Feb 2010, Erik Faye-Lund wrote:

On Tue, Feb 23, 2010 at 2:19 PM, Johannes Sixt [off-list ref] wrote:

quoted

Johannes Schindelin schrieb:

quoted

May I caution against allowing "+" as part of filenames? On Windows,
thanks to the DOS garb^Wheritage, "+" is not really allowed...

I don't think that's true (and I also think it never was).

Wikipedia doesn't seem to think so either:
http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words

Okay, I stand corrected, and therefore gladly retract my objections!

Ciao,
Dscho

Paolo Bonzini wrote:

On 02/23/2010 01:33 PM, Ilari Liusvaara wrote:

quoted

According to relevant RFCs, in addition to alphanumerics, the following
characters are valid in URL scheme parts: '+', '-' and '.', but
currently only alphanumerics are allowed in remote helper names.

Allow those three characters in remote helper names (both 'foo://' and
'foo::' syntax).

I think '+' could be special-cased in that, for example, "svn+ssh://"
should still invoke an hypothetic git-remote-svn helper.  There is no
use yet for this feature, but I'm sure that foreign VCS helpers would
use it.

Special-casing the + could be useful to simplify support for
"svn::ssh://"-style addresses as both could receive the same URL
("ssh://..").

It would also mean less clutter in the script directory. Instead of
having two scripts, one to catch "helper://.." remote URLs and another
to catch "helper+ssh://.." URLs, the same script would differentiate
between used protocols.

-- 
Gabriel Filion