threads / rfc / 14124

RFC patchgit-svn: sanitize_remote_name should accept underscores.

Subject: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

## tl;dr

7 messages between Jun 24, 2008 and Jun 29, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Avery Pennarun· Jun 24, 2008, 15:54 UTC · lore
Without this patch, git-svn failed with the error:
 config --get svn-remote.D2007.Win32.url: command returned error: 1

...upon trying to automatically follow a link from a child branch back to its parent branch D2007_Win32 (note the underscore, not dot, separating the two words).

Note that I have each of my branches defined (by hand) as separate svn-remote entries in .git/config since my svn repository layout is nonstandard.

Signed-off-by: Avery Pennarun <apenwarr@gmail.com>

--- I'm not sure why sanitize_remote_name is so picky about allowed characters, but underscore should certainly be allowed. I'm worried that this has revealed a more serious problem, since presumably sanitizing the name shouldn't break anything in any case.

---
 git-svn.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-svn.perl +1 −1
diff --git a/git-svn.perl b/git-svn.perl
index 4c9c59b..263d66c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1465,7 +1465,7 @@ sub verify_remotes_sanity {
 # we allow more chars than remotes2config.sh...
 sub sanitize_remote_name {
 	my ($name) = @_;
-	$name =~ tr{A-Za-z0-9:,/+-}{.}c;
+	$name =~ tr{A-Za-z0-9:,_/+-}{.}c;
 	$name;
 }
 
-- 
1.5.6.56.g29b0d
Eric Wong· Jun 25, 2008, 06:44 UTC · re: Avery Pennarun · lore

Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

Avery Pennarun <apenwarr@gmail.com> wrote:
Show 12 quoted lines
> Without this patch, git-svn failed with the error:
>  config --get svn-remote.D2007.Win32.url: command returned error: 1
> 
> ...upon trying to automatically follow a link from a child branch back to
> its parent branch D2007_Win32 (note the underscore, not dot, separating the
> two words).
> 
> Note that I have each of my branches defined (by hand) as separate
> svn-remote entries in .git/config since my svn repository layout is
> nonstandard.
> 
> Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
Thanks,
Acked-by: Eric Wong <normalperson@yhbt.net>
Show 5 quoted lines
> ---
> I'm not sure why sanitize_remote_name is so picky about allowed characters,
> but underscore should certainly be allowed.  I'm worried that this has
> revealed a more serious problem, since presumably sanitizing the name
> shouldn't break anything in any case.

Weird. It looks like a stupid bug on my part. I'm surprised it took this long to find, since underscore is pretty common...

Show 19 quoted lines
> ---
>  git-svn.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 4c9c59b..263d66c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1465,7 +1465,7 @@ sub verify_remotes_sanity {
>  # we allow more chars than remotes2config.sh...
>  sub sanitize_remote_name {
>  	my ($name) = @_;
> -	$name =~ tr{A-Za-z0-9:,/+-}{.}c;
> +	$name =~ tr{A-Za-z0-9:,_/+-}{.}c;
>  	$name;
>  }
>  
> -- 
> 1.5.6.56.g29b0d
Eric Wong· Jun 25, 2008, 06:55 UTC · re: Eric Wong · lore

Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

Eric Wong <normalperson@yhbt.net> wrote:
Show 26 quoted lines
> Avery Pennarun <apenwarr@gmail.com> wrote:
> > Without this patch, git-svn failed with the error:
> >  config --get svn-remote.D2007.Win32.url: command returned error: 1
> > 
> > ...upon trying to automatically follow a link from a child branch back to
> > its parent branch D2007_Win32 (note the underscore, not dot, separating the
> > two words).
> > 
> > Note that I have each of my branches defined (by hand) as separate
> > svn-remote entries in .git/config since my svn repository layout is
> > nonstandard.
> > 
> > Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
> 
> Thanks,
> 
> Acked-by: Eric Wong <normalperson@yhbt.net>
> 
> > ---
> > I'm not sure why sanitize_remote_name is so picky about allowed characters,
> > but underscore should certainly be allowed.  I'm worried that this has
> > revealed a more serious problem, since presumably sanitizing the name
> > shouldn't break anything in any case.
> 
> Weird.  It looks like a stupid bug on my part.  I'm surprised
> it took this long to find, since underscore is pretty common...
Wait, nevermind, this is for remotes, not remote *branches*.
Umm... are underscores now allowed in git config files?
-- 
Eric Wong
Junio C Hamano· Jun 25, 2008, 07:11 UTC · re: Eric Wong · lore

Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

Eric Wong <normalperson@yhbt.net> writes:
> Wait, nevermind, this is for remotes, not remote *branches*.
>
> Umm... are underscores now allowed in git config files?
In
	[foo "bar"] baz = value

foo and baz must be config.c::iskeychar() (and baz must be isalpha()), but "bar" can be almost anything.

Isn't "not underscore" coming from DNS hostname part restriction?
Eric Wong· Jun 25, 2008, 07:45 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Eric Wong <normalperson@yhbt.net> writes:
> 
> > Wait, nevermind, this is for remotes, not remote *branches*.
> >
> > Umm... are underscores now allowed in git config files?
> 
> In
> 
> 	[foo "bar"] baz = value
> 
> foo and baz must be config.c::iskeychar() (and baz must be isalpha()), but
> "bar" can be almost anything.
> 
> Isn't "not underscore" coming from DNS hostname part restriction?

No, nothing to do with DNS hostnames in the remote names. I think I just looked at remotes2config.sh one day and used it as a reference :x

It's late and I've had a rough few days, but shouldn't sanitize_remote_name() just escape . and "? Right now it's converting stuff to . which has me very confused...

-- 
Eric Wong (in need of sleep and sanity atm...)
Avery Pennarun· Jun 25, 2008, 15:01 UTC · re: Eric Wong · lore

Re: [PATCH/RFC] git-svn: sanitize_remote_name should accept underscores.

On 6/25/08, Eric Wong <normalperson@yhbt.net> wrote:
Show 6 quoted lines
> No, nothing to do with DNS hostnames in the remote names.  I think I
>  just looked at remotes2config.sh one day and used it as a reference :x
>
>  It's late and I've had a rough few days, but shouldn't
>  sanitize_remote_name() just escape . and "?  Right now it's converting
>  stuff to . which has me very confused...

I think there might be higher-level problems here: what is it sanitizing anyway, and why? If it found my D2007_Win32 svn-remote entry in the config (as it seems to have done when trying to locate its parent branch during fetch), and *then* it sanitized it to D2007.Win32, that doesn't even make any sense. Clearly something straight from the config file doesn't need to be sanitized.

However, I don't understand the code well enough to be able to say a) whether that's exactly what happened, or b) other places where sanitize_remote_name() *is* important, or c) whether sanitize_remote_name() is even correct.

Have fun,
Avery
Eric Wong· Jun 29, 2008, 03:40 UTC · re: Avery Pennarun · lore

[PATCH] git-svn: don't sanitize remote names in config

The original sanitization code was just taken from the remotes2config.sh shell script in contrib.

Credit to Avery Pennarun for noticing this mistake, and Junio for clarifying the rules for config section names:

Junio C Hamano wrote <7vfxr23s6m.fsf@gitster.siamese.dyndns.org>:
Show 6 quoted lines
> In
>
> 	[foo "bar"] baz = value
>
> foo and baz must be config.c::iskeychar() (and baz must be isalpha()), but
> "bar" can be almost anything.
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
  Avery Pennarun <apenwarr@gmail.com> wrote:
  > On 6/25/08, Eric Wong <normalperson@yhbt.net> wrote:
  > > No, nothing to do with DNS hostnames in the remote names.  I think I
  > >  just looked at remotes2config.sh one day and used it as a reference :x
  > >
  > >  It's late and I've had a rough few days, but shouldn't
  > >  sanitize_remote_name() just escape . and "?  Right now it's converting
  > >  stuff to . which has me very confused...
  > 
  > I think there might be higher-level problems here: what is it
  > sanitizing anyway, and why?  If it found my D2007_Win32 svn-remote
  > entry in the config (as it seems to have done when trying to locate
  > its parent branch during fetch), and *then* it sanitized it to
  > D2007.Win32, that doesn't even make any sense.  Clearly something
  > straight from the config file doesn't need to be sanitized.
  > 
  > However, I don't understand the code well enough to be able to say a)
  > whether that's exactly what happened, or b) other places where
  > sanitize_remote_name() *is* important, or c) whether
  > sanitize_remote_name() is even correct.
  Nope.  It's not important anywhere from what I can tell.
-- 
 git-svn.perl |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 50ace22..f789a6e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1462,13 +1462,6 @@ sub verify_remotes_sanity {
 	}
 }
 
-# we allow more chars than remotes2config.sh...
-sub sanitize_remote_name {
-	my ($name) = @_;
-	$name =~ tr{A-Za-z0-9:,/+-}{.}c;
-	$name;
-}
-
 sub find_existing_remote {
 	my ($url, $remotes) = @_;
 	return undef if $no_reuse_existing;
@@ -2853,7 +2846,7 @@ sub _new {
 	unless (defined $ref_id && length $ref_id) {
 		$_[2] = $ref_id = $Git::SVN::default_ref_id;
 	}
-	$_[1] = $repo_id = sanitize_remote_name($repo_id);
+	$_[1] = $repo_id;
 	my $dir = "$ENV{GIT_DIR}/svn/$ref_id";
 	$_[3] = $path = '' unless (defined $path);
 	mkpath(["$ENV{GIT_DIR}/svn"]);
@@ -4707,8 +4700,7 @@ sub minimize_connections {
 
 		# skip existing cases where we already connect to the root
 		if (($ra->{url} eq $ra->{repos_root}) ||
-		    (Git::SVN::sanitize_remote_name($ra->{repos_root}) eq
-		     $repo_id)) {
+		    ($ra->{repos_root} eq $repo_id)) {
 			$root_repos->{$ra->{url}} = $repo_id;
 			next;
 		}
@@ -4747,8 +4739,7 @@ sub minimize_connections {
 	foreach my $url (keys %$new_urls) {
 		# see if we can re-use an existing [svn-remote "repo_id"]
 		# instead of creating a(n ugly) new section:
-		my $repo_id = $root_repos->{$url} ||
-		              Git::SVN::sanitize_remote_name($url);
+		my $repo_id = $root_repos->{$url} || $url;
 
 		my $fetch = $new_urls->{$url};
 		foreach my $path (keys %$fetch) {
-- 
Eric Wong

← back to recent threads