{"thread":{"id":"14124","subject":"[PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","startedAt":"2008-06-24T15:54:58Z","lastAt":"2008-06-29T03:40:32Z","messageCount":7,"participants":["Avery Pennarun","Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"80917","messageId":"1214322898-9272-1-git-send-email-apenwarr@gmail.com","threadId":"14124","inReplyTo":null,"subject":"[PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-06-24T15:54:58Z","receivedAt":"2008-06-24T15:54:58Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"Without this patch, git-svn failed with the error:\n config --get svn-remote.D2007.Win32.url: command returned error: 1\n\n...upon trying to automatically follow a link from a child branch back to\nits parent branch D2007_Win32 (note the underscore, not dot, separating the\ntwo words).\n\nNote that I have each of my branches defined (by hand) as separate\nsvn-remote entries in .git/config since my svn repository layout is\nnonstandard.\n\nSigned-off-by: Avery Pennarun \n\n---\nI'm not sure why sanitize_remote_name is so picky about allowed characters,\nbut underscore should certainly be allowed. I'm worried that this has\nrevealed a more serious problem, since presumably sanitizing the name\nshouldn't break anything in any case.\n\n---\n git-svn.perl | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 4c9c59b..263d66c 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1465,7 +1465,7 @@ sub verify_remotes_sanity {\n # we allow more chars than remotes2config.sh...\n sub sanitize_remote_name {\n \tmy ($name) = @_;\n-\t$name =~ tr{A-Za-z0-9:,/+-}{.}c;\n+\t$name =~ tr{A-Za-z0-9:,_/+-}{.}c;\n \t$name;\n }\n \n-- \n1.5.6.56.g29b0d\n"},{"id":"81082","messageId":"20080625064435.GL21299@hand.yhbt.net","threadId":"14124","inReplyTo":"1214322898-9272-1-git-send-email-apenwarr@gmail.com","subject":"Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-06-25T06:44:35Z","receivedAt":"2008-06-25T06:44:35Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Avery Pennarun wrote:\n> Without this patch, git-svn failed with the error:\n> config --get svn-remote.D2007.Win32.url: command returned error: 1\n> \n> ...upon trying to automatically follow a link from a child branch back to\n> its parent branch D2007_Win32 (note the underscore, not dot, separating the\n> two words).\n> \n> Note that I have each of my branches defined (by hand) as separate\n> svn-remote entries in .git/config since my svn repository layout is\n> nonstandard.\n> \n> Signed-off-by: Avery Pennarun \n\nThanks,\n\nAcked-by: Eric Wong \n\n> ---\n> I'm not sure why sanitize_remote_name is so picky about allowed characters,\n> but underscore should certainly be allowed. I'm worried that this has\n> revealed a more serious problem, since presumably sanitizing the name\n> shouldn't break anything in any case.\n\nWeird. It looks like a stupid bug on my part. I'm surprised\nit took this long to find, since underscore is pretty common...\n\n> ---\n> git-svn.perl | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-svn.perl b/git-svn.perl\n> index 4c9c59b..263d66c 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -1465,7 +1465,7 @@ sub verify_remotes_sanity {\n> # we allow more chars than remotes2config.sh...\n> sub sanitize_remote_name {\n> \tmy ($name) = @_;\n> -\t$name =~ tr{A-Za-z0-9:,/+-}{.}c;\n> +\t$name =~ tr{A-Za-z0-9:,_/+-}{.}c;\n> \t$name;\n> }\n> \n> -- \n> 1.5.6.56.g29b0d\n"},{"id":"81084","messageId":"20080625065556.GM21299@hand.yhbt.net","threadId":"14124","inReplyTo":"20080625064435.GL21299@hand.yhbt.net","subject":"Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-06-25T06:55:56Z","receivedAt":"2008-06-25T06:55:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Eric Wong wrote:\n> Avery Pennarun wrote:\n> > Without this patch, git-svn failed with the error:\n> > config --get svn-remote.D2007.Win32.url: command returned error: 1\n> > \n> > ...upon trying to automatically follow a link from a child branch back to\n> > its parent branch D2007_Win32 (note the underscore, not dot, separating the\n> > two words).\n> > \n> > Note that I have each of my branches defined (by hand) as separate\n> > svn-remote entries in .git/config since my svn repository layout is\n> > nonstandard.\n> > \n> > Signed-off-by: Avery Pennarun \n> \n> Thanks,\n> \n> Acked-by: Eric Wong \n> \n> > ---\n> > I'm not sure why sanitize_remote_name is so picky about allowed characters,\n> > but underscore should certainly be allowed. I'm worried that this has\n> > revealed a more serious problem, since presumably sanitizing the name\n> > shouldn't break anything in any case.\n> \n> Weird. It looks like a stupid bug on my part. I'm surprised\n> it took this long to find, since underscore is pretty common...\n\nWait, nevermind, this is for remotes, not remote *branches*.\n\nUmm... are underscores now allowed in git config files?\n\n-- \nEric Wong\n"},{"id":"81086","messageId":"7vfxr23s6m.fsf@gitster.siamese.dyndns.org","threadId":"14124","inReplyTo":"20080625065556.GM21299@hand.yhbt.net","subject":"Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-25T07:11:13Z","receivedAt":"2008-06-25T07:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Wait, nevermind, this is for remotes, not remote *branches*.\n>\n> Umm... are underscores now allowed in git config files?\n\nIn\n\n\t[foo \"bar\"] baz = value\n\nfoo and baz must be config.c::iskeychar() (and baz must be isalpha()), but\n\"bar\" can be almost anything.\n\nIsn't \"not underscore\" coming from DNS hostname part restriction?\n"},{"id":"81090","messageId":"20080625074548.GA8984@hand.yhbt.net","threadId":"14124","inReplyTo":"7vfxr23s6m.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-06-25T07:45:48Z","receivedAt":"2008-06-25T07:45:48Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> \n> > Wait, nevermind, this is for remotes, not remote *branches*.\n> >\n> > Umm... are underscores now allowed in git config files?\n> \n> In\n> \n> \t[foo \"bar\"] baz = value\n> \n> foo and baz must be config.c::iskeychar() (and baz must be isalpha()), but\n> \"bar\" can be almost anything.\n> \n> Isn't \"not underscore\" coming from DNS hostname part restriction?\n\nNo, nothing to do with DNS hostnames in the remote names. I think I\njust looked at remotes2config.sh one day and used it as a reference :x\n\nIt's late and I've had a rough few days, but shouldn't\nsanitize_remote_name() just escape . and \"? Right now it's converting\nstuff to . which has me very confused...\n\n-- \nEric Wong (in need of sleep and sanity atm...)\n"},{"id":"81124","messageId":"32541b130806250801p1508d15axc610f335b8d235ef@mail.gmail.com","threadId":"14124","inReplyTo":"20080625074548.GA8984@hand.yhbt.net","subject":"Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-06-25T15:01:11Z","receivedAt":"2008-06-25T15:01:11Z","isPatch":true,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On 6/25/08, Eric Wong wrote:\n> No, nothing to do with DNS hostnames in the remote names. I think I\n> just looked at remotes2config.sh one day and used it as a reference :x\n>\n> It's late and I've had a rough few days, but shouldn't\n> sanitize_remote_name() just escape . and \"? Right now it's converting\n> stuff to . which has me very confused...\n\nI think there might be higher-level problems here: what is it\nsanitizing anyway, and why? If it found my D2007_Win32 svn-remote\nentry in the config (as it seems to have done when trying to locate\nits parent branch during fetch), and *then* it sanitized it to\nD2007.Win32, that doesn't even make any sense. Clearly something\nstraight from the config file doesn't need to be sanitized.\n\nHowever, I don't understand the code well enough to be able to say a)\nwhether that's exactly what happened, or b) other places where\nsanitize_remote_name() *is* important, or c) whether\nsanitize_remote_name() is even correct.\n\nHave fun,\n\nAvery\n"},{"id":"81596","messageId":"20080629034032.GA23492@untitled","threadId":"14124","inReplyTo":"32541b130806250801p1508d15axc610f335b8d235ef@mail.gmail.com","subject":"[PATCH] git-svn: don't sanitize remote names in config","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2008-06-29T03:40:32Z","receivedAt":"2008-06-29T03:40:32Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"The original sanitization code was just taken from the\nremotes2config.sh shell script in contrib.\n\nCredit to Avery Pennarun for noticing this mistake, and Junio\nfor clarifying the rules for config section names:\n\nJunio C Hamano wrote <7vfxr23s6m.fsf@gitster.siamese.dyndns.org>:\n> In\n>\n> \t[foo \"bar\"] baz = value\n>\n> foo and baz must be config.c::iskeychar() (and baz must be isalpha()), but\n> \"bar\" can be almost anything.\n\nSigned-off-by: Eric Wong \n---\n\n Avery Pennarun wrote:\n > On 6/25/08, Eric Wong wrote:\n > > No, nothing to do with DNS hostnames in the remote names. I think I\n > > just looked at remotes2config.sh one day and used it as a reference :x\n > >\n > > It's late and I've had a rough few days, but shouldn't\n > > sanitize_remote_name() just escape . and \"? Right now it's converting\n > > stuff to . which has me very confused...\n > \n > I think there might be higher-level problems here: what is it\n > sanitizing anyway, and why? If it found my D2007_Win32 svn-remote\n > entry in the config (as it seems to have done when trying to locate\n > its parent branch during fetch), and *then* it sanitized it to\n > D2007.Win32, that doesn't even make any sense. Clearly something\n > straight from the config file doesn't need to be sanitized.\n > \n > However, I don't understand the code well enough to be able to say a)\n > whether that's exactly what happened, or b) other places where\n > sanitize_remote_name() *is* important, or c) whether\n > sanitize_remote_name() is even correct.\n\n Nope. It's not important anywhere from what I can tell.\n\n-- \n\n git-svn.perl | 15 +++------------\n 1 files changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 50ace22..f789a6e 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -1462,13 +1462,6 @@ sub verify_remotes_sanity {\n \t}\n }\n \n-# we allow more chars than remotes2config.sh...\n-sub sanitize_remote_name {\n-\tmy ($name) = @_;\n-\t$name =~ tr{A-Za-z0-9:,/+-}{.}c;\n-\t$name;\n-}\n-\n sub find_existing_remote {\n \tmy ($url, $remotes) = @_;\n \treturn undef if $no_reuse_existing;\n@@ -2853,7 +2846,7 @@ sub _new {\n \tunless (defined $ref_id && length $ref_id) {\n \t\t$_[2] = $ref_id = $Git::SVN::default_ref_id;\n \t}\n-\t$_[1] = $repo_id = sanitize_remote_name($repo_id);\n+\t$_[1] = $repo_id;\n \tmy $dir = \"$ENV{GIT_DIR}/svn/$ref_id\";\n \t$_[3] = $path = '' unless (defined $path);\n \tmkpath([\"$ENV{GIT_DIR}/svn\"]);\n@@ -4707,8 +4700,7 @@ sub minimize_connections {\n \n \t\t# skip existing cases where we already connect to the root\n \t\tif (($ra->{url} eq $ra->{repos_root}) ||\n-\t\t (Git::SVN::sanitize_remote_name($ra->{repos_root}) eq\n-\t\t $repo_id)) {\n+\t\t ($ra->{repos_root} eq $repo_id)) {\n \t\t\t$root_repos->{$ra->{url}} = $repo_id;\n \t\t\tnext;\n \t\t}\n@@ -4747,8 +4739,7 @@ sub minimize_connections {\n \tforeach my $url (keys %$new_urls) {\n \t\t# see if we can re-use an existing [svn-remote \"repo_id\"]\n \t\t# instead of creating a(n ugly) new section:\n-\t\tmy $repo_id = $root_repos->{$url} ||\n-\t\t Git::SVN::sanitize_remote_name($url);\n+\t\tmy $repo_id = $root_repos->{$url} || $url;\n \n \t\tmy $fetch = $new_urls->{$url};\n \t\tforeach my $path (keys %$fetch) {\n-- \nEric Wong\n"}]}