[PATCHv2 0/2] a couple of git-svn patches

[PATCHv2 0/2] a couple of git-svn patches

[Alejandro R. Sedeño]

(The patches themselves haven't changed since I sent them to the list
on Friday, but this does clean up attribution and sign-offs in the
commit messages.)

Here are two independent git-svn patches that my co-worker handed off
to me back in January.

The first changes the behavior of commit_url to be like that of url so
that you can commit to multiple branches.

The second uses Memoize to cache the results of shelling out to
git-config, which has good performance benefits for git-svn fetch.

[PATCH 1/2] git-svn: Fix the commit-url config to be the base url, just like the url config

[Alejandro R. Sedeño]

From: James Y Knight <jknight@itasoftware.com>

This is necessary if you want to be able to commit to multiple branches.

Signed-off-by: James Y Knight <jknight@itasoftware.com>
Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
---
 git-svn.perl |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index a5857c1..aa41896 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -523,12 +523,14 @@ sub cmd_dcommit {
 	}
 
 	if (defined $_commit_url) {
-		$url = $_commit_url;
+		$url = $_commit_url . (length $gs->{path} ? '/' . $gs->{path} : '');
 	} else {
 		$url = eval { command_oneline('config', '--get',
 			      "svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
 			$url = $gs->full_url
+		} else {
+			$url = $url . (length $gs->{path} ? '/' . $gs->{path} : '');
 		}
 	}
 
-- 
1.7.4.2.1.gd6f1f

[PATCH 2/2] git-svn: Cache results of running the executable "git config"

[Alejandro R. Sedeño]

From: James Y Knight <jknight@itasoftware.com>

Running programs is not cheap!

Signed-off-by: James Y Knight <jknight@itasoftware.com>
Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
---
 git-svn.perl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index aa41896..e47e04c 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -59,6 +59,7 @@ use File::Find;
 use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
 use IPC::Open3;
 use Git;
+use Memoize;  # core since 5.8.0, Jul 2002
 
 BEGIN {
 	# import functions from Git into our packages, en masse
@@ -72,6 +73,8 @@ BEGIN {
 			*{"${package}::$_"} = \&{"Git::$_"};
 		}
 	}
+	Memoize::memoize 'Git::config';
+	Memoize::memoize 'Git::config_bool';
 }
 
 my ($SVN);
@@ -3199,6 +3202,8 @@ sub has_no_changes {
 		Memoize::unmemoize 'check_cherry_pick';
 		Memoize::unmemoize 'has_no_changes';
 	}
+
+	Memoize::memoize 'Git::SVN::repos_root';
 }
 
 END {
-- 
1.7.4.2.1.gd6f1f

Re: [PATCH 1/2] git-svn: Fix the commit-url config to be the base url, just like the url config

[Eric Wong]

"Alejandro R. Sedeño" <asedeno@mit.edu> wrote:
> From: James Y Knight <jknight@itasoftware.com>
> 
> This is necessary if you want to be able to commit to multiple branches.

I'm not sure I understand the need for this patch, see below.

> @@ -523,12 +523,14 @@ sub cmd_dcommit {
>  	}
>  
>  	if (defined $_commit_url) {
> -		$url = $_commit_url;
> +		$url = $_commit_url . (length $gs->{path} ? '/' . $gs->{path} : '');

$_commit_url is a user-specified parameter from the --commit-url switch.
If they want to override it it's the user's perogative.  It's not the
default and not commonly used.

>  	} else {
>  		$url = eval { command_oneline('config', '--get',
>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
>  			$url = $gs->full_url

If unspecified, we always fall back to the gs->full_url case anyways
which just duplicates the logic you've added.  Again, commitUrl usually
does not need to be specified since it is autodetected.

> +		} else {
> +			$url = $url . (length $gs->{path} ? '/' . $gs->{path} : '');
>  		}
>  	}

-- 
Eric Wong

Re: [PATCH 2/2] git-svn: Cache results of running the executable "git config"

[Eric Wong]

"Alejandro R. Sedeño" <asedeno@mit.edu> wrote:
> From: James Y Knight <jknight@itasoftware.com>
> 
> Running programs is not cheap!
> 
> Signed-off-by: James Y Knight <jknight@itasoftware.com>
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>

Thanks, acked and pushed out to git://bogomips.org/git-svn.git

-- 
Eric Wong

Re: [PATCH 1/2] git-svn: Fix the commit-url config to be the base url, just like the url config

[James Y Knight]

On Apr 4, 2011, at 5:52 PM, Eric Wong wrote:
> $_commit_url is a user-specified parameter from the --commit-url switch.
> If they want to override it it's the user's perogative.  It's not the
> default and not commonly used.

In .git/config:

[svn-remote "svn"]
        url = http://hostname/svnrepo/
        commiturl = svn+ssh://hostname/svnrepo
        branches = branches/*:refs/remotes/origin/*
        fetch = trunk:refs/remotes/origin/master

The commiturl configuration is necessary so that the canonical URL is the http:// url (which can be used for read-only access), but if you want to commit, you have to use svn+ssh. This is a fairly common way of setting up access to an svn repository, so I'm surprised to be the first one to hit this issue.

Without the patch, you need:
        commiturl = svn+ssh://hostname/svnrepo/trunk
which of course then doesn't allow you to commit to other branches.

James

Re: [PATCH 1/2] git-svn: Fix the commit-url config to be the base url, just like the url config

[Eric Wong]

James Y Knight <jknight@itasoftware.com> wrote:
> On Apr 4, 2011, at 5:52 PM, Eric Wong wrote:
> > $_commit_url is a user-specified parameter from the --commit-url switch.
> > If they want to override it it's the user's perogative.  It's not the
> > default and not commonly used.
> 
> In .git/config:
> 
> [svn-remote "svn"]
>         url = http://hostname/svnrepo/
>         commiturl = svn+ssh://hostname/svnrepo
>         branches = branches/*:refs/remotes/origin/*
>         fetch = trunk:refs/remotes/origin/master
> 
> The commiturl configuration is necessary so that the canonical URL is the http:// url (which can be used for read-only access), but if you want to commit, you have to use svn+ssh. This is a fairly common way of setting up access to an svn repository, so I'm surprised to be the first one to hit this issue.
> 
> Without the patch, you need:
>         commiturl = svn+ssh://hostname/svnrepo/trunk
> which of course then doesn't allow you to commit to other branches.

Originally --commit-url was only intended to be a command-line option
and for overriding specific cases and also for dealing with permission
mismatches (limited commit access to a branch, unlimited read access
to the repo).

Your patch breaks existing use cases, I think.

I think Junio's suggestion for a pushurl config which does what you
think commitUrl does would be what you're after...

Thanks


-- 
Eric Wong

Re: [PATCH 2/2] git-svn: Cache results of running the executable "git config"

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> "Alejandro R. Sedeño" <asedeno@mit.edu> wrote:
>> From: James Y Knight <jknight@itasoftware.com>
>> 
>> Running programs is not cheap!
>> 
>> Signed-off-by: James Y Knight <jknight@itasoftware.com>
>> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
>
> Thanks, acked and pushed out to git://bogomips.org/git-svn.git

Hmph, any comment on 1/2 of the series?

Re: [PATCH 1/2] git-svn: Fix the commit-url config to be the base url, just like the url config

["Alejandro R. Sedeño"]

On 04/04/2011 06:54 PM, Eric Wong wrote:
> James Y Knight <jknight@itasoftware.com> wrote:
>> > On Apr 4, 2011, at 5:52 PM, Eric Wong wrote:
>>> > > $_commit_url is a user-specified parameter from the --commit-url switch.
>>> > > If they want to override it it's the user's perogative.  It's not the
>>> > > default and not commonly used.
>> > 
>> > In .git/config:
>> > 
>> > [svn-remote "svn"]
>> >         url = http://hostname/svnrepo/
>> >         commiturl = svn+ssh://hostname/svnrepo
>> >         branches = branches/*:refs/remotes/origin/*
>> >         fetch = trunk:refs/remotes/origin/master
>> > 
>> > The commiturl configuration is necessary so that the canonical URL is the http:// url (which can be used for read-only access), but if you want to commit, you have to use svn+ssh. This is a fairly common way of setting up access to an svn repository, so I'm surprised to be the first one to hit this issue.
>> > 
>> > Without the patch, you need:
>> >         commiturl = svn+ssh://hostname/svnrepo/trunk
>> > which of course then doesn't allow you to commit to other branches.
> Originally --commit-url was only intended to be a command-line option
> and for overriding specific cases and also for dealing with permission
> mismatches (limited commit access to a branch, unlimited read access
> to the repo).
> 
> Your patch breaks existing use cases, I think.
> 
> I think Junio's suggestion for a pushurl config which does what you
> think commitUrl does would be what you're after...

That sounds good to me. I'll submit an updated patch later today.

-Alejandro

[PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Alejandro R. Sedeño]

Similar to the 'remote.<name>.pushurl' config key for git remotes, the
'pushurl' key is treated the same as the 'url' key. This is distinct
from the 'commiturl' key, which is defined to be a full svn path.

This is necessary if you want to be able to commit to multiple branches.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
---
 Documentation/git-svn.txt |    6 ++++++
 git-svn.perl              |   14 ++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index ea8fafd..94f7497 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -648,6 +648,12 @@ svn-remote.<name>.rewriteUUID::
 	where the original UUID is not available via either useSvmProps
 	or useSvnsyncProps.
 
+svn-remote.<name>.pushurl::
+	Similar to git's remote.<name>.pushurl, this is useful in
+	cases where the SVN repository url is read-only. Unlike
+	'commiturl', 'pushurl' is a base path. This key is overridden
+	by the 'commiturl' config key.
+
 svn.brokenSymlinkWorkaround::
 	This disables potentially expensive checks to workaround
 	broken symlinks checked into SVN by broken clients.  Set this
diff --git a/git-svn.perl b/git-svn.perl
index fa8cd07..8372606 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -531,7 +531,13 @@ sub cmd_dcommit {
 		$url = eval { command_oneline('config', '--get',
 			      "svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $gs->full_url
+			$url = eval { command_oneline('config', '--get',
+				      "svn-remote.$gs->{repo_id}.pushurl") };
+			if ($url) {
+				$url .= (length $gs->{path} ? '/' . $gs->{path} : '');
+			} else {
+				$url = $gs->full_url
+			}
 		}
 	}
 
@@ -730,7 +736,11 @@ sub cmd_branch {
 		$url = eval { command_oneline('config', '--get',
 			"svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $remote->{url};
+			$url = eval { command_oneline('config', '--get',
+				      "svn-remote.$gs->{repo_id}.pushurl") };
+			if (!$url) {
+				$url = $remote->{url};
+			}
 		}
 	}
 	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
-- 
1.7.4.2.1.gd6f1f

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

On 04/05/2011 04:15 PM, Alejandro R. Sedeño wrote:
> @@ -730,7 +736,11 @@ sub cmd_branch {
>  		$url = eval { command_oneline('config', '--get',
>  			"svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $remote->{url};
> +			$url = eval { command_oneline('config', '--get',
> +				      "svn-remote.$gs->{repo_id}.pushurl") };
> +			if (!$url) {
> +				$url = $remote->{url};
> +			}
>  		}
>  	}
>  	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());

Actually, I was just finishing running this through its paces with
different values for url and pushurl, and branching has issues:

Trying to use an unsupported feature: Source and dest appear not to be in
the same repository (src: 'http://asedeno/svn/trunk'; dst:
'file:///tmp/svn/repo/branches/nb1') at /tmp/git/libexec/git-core/git-svn
line 770

So I'm continuing to look into that. Perhaps it makes sense to use pushurl
for both src and dst when branching?

-Alejandro

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Eric Wong]

"\"Alejandro R. Sedeño\"" <asedeno@mit.edu> wrote:
> Trying to use an unsupported feature: Source and dest appear not to be in
> the same repository (src: 'http://asedeno/svn/trunk'; dst:
> 'file:///tmp/svn/repo/branches/nb1') at /tmp/git/libexec/git-core/git-svn
> line 770

This may be a new warning/error in newer versions of Subversion itself.
Maybe it's better to always use the same URL for all read/writes

-- 
Eric Wong

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

Alejandro R. Sedeño venit, vidit, dixit 05.04.2011 22:15:
> Similar to the 'remote.<name>.pushurl' config key for git remotes, the
> 'pushurl' key is treated the same as the 'url' key. This is distinct
> from the 'commiturl' key, which is defined to be a full svn path.
> 
> This is necessary if you want to be able to commit to multiple branches.

Maybe one can understand this remark after reading the whole thread, but
reading it as a commit message I'm wondering: Huh? How can I have been
doing it then without pushurl?

Also, "treated the same as the url" makes a reader wonder why we have
two names for the same.

The point of pushurl is that you can use a passwordless transport for
fetches and another transport for pushes. The standing assumption is
that both urls point in fact at the same repo. Weird things can happen
if not. Is that assumption the same for your svn pushurl?

> 
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
> ---
>  Documentation/git-svn.txt |    6 ++++++
>  git-svn.perl              |   14 ++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
> index ea8fafd..94f7497 100644
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -648,6 +648,12 @@ svn-remote.<name>.rewriteUUID::
>  	where the original UUID is not available via either useSvmProps
>  	or useSvnsyncProps.
>  
> +svn-remote.<name>.pushurl::
> +	Similar to git's remote.<name>.pushurl, this is useful in
> +	cases where the SVN repository url is read-only. Unlike
> +	'commiturl', 'pushurl' is a base path. This key is overridden
> +	by the 'commiturl' config key.
> +

Exactly :)

>  svn.brokenSymlinkWorkaround::
>  	This disables potentially expensive checks to workaround
>  	broken symlinks checked into SVN by broken clients.  Set this
> diff --git a/git-svn.perl b/git-svn.perl
> index fa8cd07..8372606 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -531,7 +531,13 @@ sub cmd_dcommit {
>  		$url = eval { command_oneline('config', '--get',
>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $gs->full_url
> +			$url = eval { command_oneline('config', '--get',
> +				      "svn-remote.$gs->{repo_id}.pushurl") };
> +			if ($url) {
> +				$url .= (length $gs->{path} ? '/' . $gs->{path} : '');
> +			} else {
> +				$url = $gs->full_url
> +			}
>  		}
>  	}
>  
> @@ -730,7 +736,11 @@ sub cmd_branch {
>  		$url = eval { command_oneline('config', '--get',
>  			"svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $remote->{url};
> +			$url = eval { command_oneline('config', '--get',
> +				      "svn-remote.$gs->{repo_id}.pushurl") };
> +			if (!$url) {
> +				$url = $remote->{url};
> +			}
>  		}
>  	}
>  	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

"Alejandro R. Sedeño" venit, vidit, dixit 05.04.2011 22:25:
> On 04/05/2011 04:15 PM, Alejandro R. Sedeño wrote:
>> @@ -730,7 +736,11 @@ sub cmd_branch {
>>  		$url = eval { command_oneline('config', '--get',
>>  			"svn-remote.$gs->{repo_id}.commiturl") };
>>  		if (!$url) {
>> -			$url = $remote->{url};
>> +			$url = eval { command_oneline('config', '--get',
>> +				      "svn-remote.$gs->{repo_id}.pushurl") };
>> +			if (!$url) {
>> +				$url = $remote->{url};
>> +			}
>>  		}
>>  	}
>>  	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
> 
> Actually, I was just finishing running this through its paces with
> different values for url and pushurl, and branching has issues:
> 
> Trying to use an unsupported feature: Source and dest appear not to be in
> the same repository (src: 'http://asedeno/svn/trunk'; dst:
> 'file:///tmp/svn/repo/branches/nb1') at /tmp/git/libexec/git-core/git-svn
> line 770
> 
> So I'm continuing to look into that. Perhaps it makes sense to use pushurl
> for both src and dst when branching?

For branching, yes. This is just the following in disguise:

http://permalink.gmane.org/gmane.comp.version-control.git/135577

"svn cp" between 2 URLs simply requires they're within the same repo.

Michael

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

On 4/6/2011 8:44 AM, Michael J Gruber wrote:
> Alejandro R. Sedeño venit, vidit, dixit 05.04.2011 22:15:
>> > Similar to the 'remote.<name>.pushurl' config key for git remotes, the
>> > 'pushurl' key is treated the same as the 'url' key. This is distinct
>> > from the 'commiturl' key, which is defined to be a full svn path.
>> > 
>> > This is necessary if you want to be able to commit to multiple branches.
> Maybe one can understand this remark after reading the whole thread, but
> reading it as a commit message I'm wondering: Huh? How can I have been
> doing it then without pushurl?
> 
> Also, "treated the same as the url" makes a reader wonder why we have
> two names for the same.
> 
> The point of pushurl is that you can use a passwordless transport for
> fetches and another transport for pushes. The standing assumption is
> that both urls point in fact at the same repo. Weird things can happen
> if not. Is that assumption the same for your svn pushurl?

Yes, that is the idea. I will rephrase the commit message and
documentation to be clearer when I send the next version of this patch.

When I say, "treated the same as the url," what I mean to say is that
any manipulations that would be done to 'url' are also done to
'pushurl', such as appending '/trunk' or '/branches/<branch-name>/' if
necessary, which does not happen with commiturl since it is a full snv path.

-Alejandro

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

On 4/6/2011 8:53 AM, Michael J Gruber wrote:
>> > So I'm continuing to look into that. Perhaps it makes sense to use pushurl
>> > for both src and dst when branching?
> For branching, yes. This is just the following in disguise:
> 
> http://permalink.gmane.org/gmane.comp.version-control.git/135577
> 
> "svn cp" between 2 URLs simply requires they're within the same repo.

Yeah, I understand that. The purpose of that email was to make sure that
the patch was not committed as is, and it was hastily written because I
was practically out the door at the time.

Thanks for the pointer though.

I'll be sending an updated patch in a few hours.

-Alejandro

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

"Alejandro R. Sedeño" venit, vidit, dixit 06.04.2011 15:04:
> On 4/6/2011 8:53 AM, Michael J Gruber wrote:
>>>> So I'm continuing to look into that. Perhaps it makes sense to use pushurl
>>>> for both src and dst when branching?
>> For branching, yes. This is just the following in disguise:
>>
>> http://permalink.gmane.org/gmane.comp.version-control.git/135577
>>
>> "svn cp" between 2 URLs simply requires they're within the same repo.
> 
> Yeah, I understand that. The purpose of that email was to make sure that
> the patch was not committed as is, and it was hastily written because I
> was practically out the door at the time.

Long arms or small room? ;)

> 
> Thanks for the pointer though.
> 
> I'll be sending an updated patch in a few hours.

Thanks!

Michael

[PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Alejandro R. Sedeño]

Similar to the 'remote.<name>.pushurl' config key for git remotes,
'pushurl' is designed to be used in cases where 'url' points to an SVN
repository via a read-only transport, to provide an alternate
read/write transport. It is assumed that both keys point to the same
repository.

The 'pushurl' key is distinct from the 'commiturl' key in that
'commiturl' is a full svn path while 'pushurl' (like 'url') is a base
path. 'commiturl' takes precendece over 'pushurl' in cases where
either might be used.

The 'pushurl' is used by git-svn's dcommit and branch commands.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
---
 Documentation/git-svn.txt |   10 ++++++++++
 git-svn.perl              |   18 +++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index ea8fafd..4aa6404 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -648,6 +648,16 @@ svn-remote.<name>.rewriteUUID::
 	where the original UUID is not available via either useSvmProps
 	or useSvnsyncProps.
 
+svn-remote.<name>.pushurl::
+
+	Similar to git's 'remote.<name>.pushurl', this key is designed
+	to be used in cases where 'url' points to an SVN repository
+	via a read-only transport, to provide an alternate read/write
+	transport. It is assumed that both keys point to the same
+	repository. Unlike 'commiturl', 'pushurl' is a base path. If
+	either 'commiturl' or 'pushurl' could be used, 'commiturl'
+	takes precedence.
+
 svn.brokenSymlinkWorkaround::
 	This disables potentially expensive checks to workaround
 	broken symlinks checked into SVN by broken clients.  Set this
diff --git a/git-svn.perl b/git-svn.perl
index fa8cd07..184442a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -531,7 +531,7 @@ sub cmd_dcommit {
 		$url = eval { command_oneline('config', '--get',
 			      "svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $gs->full_url
+			$url = $gs->full_pushurl
 		}
 	}
 
@@ -679,7 +679,7 @@ sub cmd_branch {
 	$head ||= 'HEAD';
 
 	my (undef, $rev, undef, $gs) = working_head_info($head);
-	my $src = $gs->full_url;
+	my $src = $gs->full_pushurl;
 
 	my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
 	my $allglobs = $remote->{ $_tag ? 'tags' : 'branches' };
@@ -730,7 +730,7 @@ sub cmd_branch {
 		$url = eval { command_oneline('config', '--get',
 			"svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $remote->{url};
+			$url = $remote->{pushurl} || $remote->{url};
 		}
 	}
 	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
@@ -1834,6 +1834,8 @@ sub read_all_remotes {
 			$r->{$1}->{svm} = {};
 		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
 			$r->{$1}->{url} = $2;
+		} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
+			$r->{$1}->{pushurl} = $2;
 		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
 			my ($remote, $t, $local_ref, $remote_ref) =
 			                                     ($1, $2, $3, $4);
@@ -2071,6 +2073,8 @@ sub new {
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
+	$self->{pushurl} = eval { command_oneline('config', '--get',
+	                          "svn-remote.$repo_id.pushurl") };
 	$self->rebuild;
 	$self;
 }
@@ -2548,6 +2552,14 @@ sub full_url {
 	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
 }
 
+sub full_pushurl {
+	my ($self) = @_;
+	if ($self->{pushurl}) {
+		return $self->{pushurl} . (length $self->{path} ? '/' . $self->{path} : '');
+	} else {
+		return $self->full_url;
+	}
+}
 
 sub set_commit_header_env {
 	my ($log_entry) = @_;
-- 
1.7.4.2.1.gd6f1f

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

Alejandro R. Sedeño venit, vidit, dixit 06.04.2011 17:05:
> Similar to the 'remote.<name>.pushurl' config key for git remotes,
> 'pushurl' is designed to be used in cases where 'url' points to an SVN
> repository via a read-only transport, to provide an alternate
> read/write transport. It is assumed that both keys point to the same
> repository.
> 
> The 'pushurl' key is distinct from the 'commiturl' key in that
> 'commiturl' is a full svn path while 'pushurl' (like 'url') is a base
> path. 'commiturl' takes precendece over 'pushurl' in cases where
> either might be used.
> 
> The 'pushurl' is used by git-svn's dcommit and branch commands.
> 

Thanks, very clear now.

> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>

:) So, if that review is off, that means...

> ---
>  Documentation/git-svn.txt |   10 ++++++++++
>  git-svn.perl              |   18 +++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
> index ea8fafd..4aa6404 100644
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -648,6 +648,16 @@ svn-remote.<name>.rewriteUUID::
>  	where the original UUID is not available via either useSvmProps
>  	or useSvnsyncProps.
>  
> +svn-remote.<name>.pushurl::
> +
> +	Similar to git's 'remote.<name>.pushurl', this key is designed
> +	to be used in cases where 'url' points to an SVN repository
> +	via a read-only transport, to provide an alternate read/write
> +	transport. It is assumed that both keys point to the same
> +	repository. Unlike 'commiturl', 'pushurl' is a base path. If
> +	either 'commiturl' or 'pushurl' could be used, 'commiturl'
> +	takes precedence.
> +
>  svn.brokenSymlinkWorkaround::
>  	This disables potentially expensive checks to workaround
>  	broken symlinks checked into SVN by broken clients.  Set this
> diff --git a/git-svn.perl b/git-svn.perl
> index fa8cd07..184442a 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -531,7 +531,7 @@ sub cmd_dcommit {
>  		$url = eval { command_oneline('config', '--get',
>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $gs->full_url
> +			$url = $gs->full_pushurl

Wouldn't we want to do the same $gs->full_pushurl || $gs->full_url fall
back here as below, or is fullpush_url always set? OK, I see it always is.

>  		}
>  	}
>  
> @@ -679,7 +679,7 @@ sub cmd_branch {
>  	$head ||= 'HEAD';
>  
>  	my (undef, $rev, undef, $gs) = working_head_info($head);
> -	my $src = $gs->full_url;
> +	my $src = $gs->full_pushurl;

Same here.

>  
>  	my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
>  	my $allglobs = $remote->{ $_tag ? 'tags' : 'branches' };
> @@ -730,7 +730,7 @@ sub cmd_branch {
>  		$url = eval { command_oneline('config', '--get',
>  			"svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $remote->{url};
> +			$url = $remote->{pushurl} || $remote->{url};
>  		}
>  	}
>  	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
> @@ -1834,6 +1834,8 @@ sub read_all_remotes {
>  			$r->{$1}->{svm} = {};
>  		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
>  			$r->{$1}->{url} = $2;
> +		} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
> +			$r->{$1}->{pushurl} = $2;
>  		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
>  			my ($remote, $t, $local_ref, $remote_ref) =
>  			                                     ($1, $2, $3, $4);
> @@ -2071,6 +2073,8 @@ sub new {
>  	$self->{url} = command_oneline('config', '--get',
>  	                               "svn-remote.$repo_id.url") or
>                    die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
> +	$self->{pushurl} = eval { command_oneline('config', '--get',
> +	                          "svn-remote.$repo_id.pushurl") };

Why eval? We don't do it for url either.

>  	$self->rebuild;
>  	$self;
>  }
> @@ -2548,6 +2552,14 @@ sub full_url {
>  	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
>  }
>  
> +sub full_pushurl {
> +	my ($self) = @_;

Isn't that a noop?

> +	if ($self->{pushurl}) {
> +		return $self->{pushurl} . (length $self->{path} ? '/' . $self->{path} : '');
> +	} else {
> +		return $self->full_url;
> +	}
> +}
>  
>  sub set_commit_header_env {
>  	my ($log_entry) = @_;

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

On 04/06/2011 11:22 AM, Michael J Gruber wrote:
> Alejandro R. Sedeño venit, vidit, dixit 06.04.2011 17:05:
>> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
>> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
> 
> :) So, if that review is off, that means...

Um, s/-off//. Oops :)

I can send a follow-up, or let Eric deal with that change, however he prefers.

>> diff --git a/git-svn.perl b/git-svn.perl
>> index fa8cd07..184442a 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -531,7 +531,7 @@ sub cmd_dcommit {
>>  		$url = eval { command_oneline('config', '--get',
>>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>>  		if (!$url) {
>> -			$url = $gs->full_url
>> +			$url = $gs->full_pushurl
> 
> Wouldn't we want to do the same $gs->full_pushurl || $gs->full_url fall
> back here as below, or is fullpush_url always set? OK, I see it always is.

Yeah, I just went with full_pushurl returning full_url in cases where
pushurl is not set.

>> @@ -2071,6 +2073,8 @@ sub new {
>>  	$self->{url} = command_oneline('config', '--get',
>>  	                               "svn-remote.$repo_id.url") or
>>                    die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
>> +	$self->{pushurl} = eval { command_oneline('config', '--get',
>> +	                          "svn-remote.$repo_id.pushurl") };
> 
> Why eval? We don't do it for url either.

Because otherwise it would die with:

  $ git svn fetch
  config --get svn-remote.svn.pushurl: command returned error: 1

when pushurl wasn't defined. If that happens with 'url' too, well, that's
a mis-configured git-svn remote.

>> +sub full_pushurl {
>> +	my ($self) = @_;
> 
> Isn't that a noop?

I'm just copying the style of sub full_url here.

-Alejandro

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

"Alejandro R. Sedeño" venit, vidit, dixit 06.04.2011 17:34:
> On 04/06/2011 11:22 AM, Michael J Gruber wrote:
>> Alejandro R. Sedeño venit, vidit, dixit 06.04.2011 17:05:
>>> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
>>> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
>>
>> :) So, if that review is off, that means...
> 
> Um, s/-off//. Oops :)
> 
> I can send a follow-up, or let Eric deal with that change, however he prefers.
> 
>>> diff --git a/git-svn.perl b/git-svn.perl
>>> index fa8cd07..184442a 100755
>>> --- a/git-svn.perl
>>> +++ b/git-svn.perl
>>> @@ -531,7 +531,7 @@ sub cmd_dcommit {
>>>  		$url = eval { command_oneline('config', '--get',
>>>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>>>  		if (!$url) {
>>> -			$url = $gs->full_url
>>> +			$url = $gs->full_pushurl
>>
>> Wouldn't we want to do the same $gs->full_pushurl || $gs->full_url fall
>> back here as below, or is fullpush_url always set? OK, I see it always is.
> 
> Yeah, I just went with full_pushurl returning full_url in cases where
> pushurl is not set.
> 
>>> @@ -2071,6 +2073,8 @@ sub new {
>>>  	$self->{url} = command_oneline('config', '--get',
>>>  	                               "svn-remote.$repo_id.url") or
>>>                    die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
>>> +	$self->{pushurl} = eval { command_oneline('config', '--get',
>>> +	                          "svn-remote.$repo_id.pushurl") };
>>
>> Why eval? We don't do it for url either.
> 
> Because otherwise it would die with:
> 
>   $ git svn fetch
>   config --get svn-remote.svn.pushurl: command returned error: 1
> 
> when pushurl wasn't defined. If that happens with 'url' too, well, that's
> a mis-configured git-svn remote.
> 
>>> +sub full_pushurl {
>>> +	my ($self) = @_;
>>
>> Isn't that a noop?
> 
> I'm just copying the style of sub full_url here.
> 

OK, I finally understood the reason for "eval" and "my ($self) = @_;". I
simply shouldn't look at perl code ;)

Thanks!

Michael

[PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Alejandro R. Sedeño]

Similar to the 'remote.<name>.pushurl' config key for git remotes,
'pushurl' is designed to be used in cases where 'url' points to an SVN
repository via a read-only transport, to provide an alternate
read/write transport. It is assumed that both keys point to the same
repository.

The 'pushurl' key is distinct from the 'commiturl' key in that
'commiturl' is a full svn path while 'pushurl' (like 'url') is a base
path. 'commiturl' takes precendece over 'pushurl' in cases where
either might be used.

The 'pushurl' is used by git-svn's dcommit and branch commands.

Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
Reviewed-by: James Y Knight <jknight@itasoftware.com>
---
 Documentation/git-svn.txt |   10 ++++++++++
 git-svn.perl              |   18 +++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index ea8fafd..4aa6404 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -648,6 +648,16 @@ svn-remote.<name>.rewriteUUID::
 	where the original UUID is not available via either useSvmProps
 	or useSvnsyncProps.
 
+svn-remote.<name>.pushurl::
+
+	Similar to git's 'remote.<name>.pushurl', this key is designed
+	to be used in cases where 'url' points to an SVN repository
+	via a read-only transport, to provide an alternate read/write
+	transport. It is assumed that both keys point to the same
+	repository. Unlike 'commiturl', 'pushurl' is a base path. If
+	either 'commiturl' or 'pushurl' could be used, 'commiturl'
+	takes precedence.
+
 svn.brokenSymlinkWorkaround::
 	This disables potentially expensive checks to workaround
 	broken symlinks checked into SVN by broken clients.  Set this
diff --git a/git-svn.perl b/git-svn.perl
index fa8cd07..184442a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -531,7 +531,7 @@ sub cmd_dcommit {
 		$url = eval { command_oneline('config', '--get',
 			      "svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $gs->full_url
+			$url = $gs->full_pushurl
 		}
 	}
 
@@ -679,7 +679,7 @@ sub cmd_branch {
 	$head ||= 'HEAD';
 
 	my (undef, $rev, undef, $gs) = working_head_info($head);
-	my $src = $gs->full_url;
+	my $src = $gs->full_pushurl;
 
 	my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
 	my $allglobs = $remote->{ $_tag ? 'tags' : 'branches' };
@@ -730,7 +730,7 @@ sub cmd_branch {
 		$url = eval { command_oneline('config', '--get',
 			"svn-remote.$gs->{repo_id}.commiturl") };
 		if (!$url) {
-			$url = $remote->{url};
+			$url = $remote->{pushurl} || $remote->{url};
 		}
 	}
 	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
@@ -1834,6 +1834,8 @@ sub read_all_remotes {
 			$r->{$1}->{svm} = {};
 		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
 			$r->{$1}->{url} = $2;
+		} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
+			$r->{$1}->{pushurl} = $2;
 		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
 			my ($remote, $t, $local_ref, $remote_ref) =
 			                                     ($1, $2, $3, $4);
@@ -2071,6 +2073,8 @@ sub new {
 	$self->{url} = command_oneline('config', '--get',
 	                               "svn-remote.$repo_id.url") or
                   die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
+	$self->{pushurl} = eval { command_oneline('config', '--get',
+	                          "svn-remote.$repo_id.pushurl") };
 	$self->rebuild;
 	$self;
 }
@@ -2548,6 +2552,14 @@ sub full_url {
 	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
 }
 
+sub full_pushurl {
+	my ($self) = @_;
+	if ($self->{pushurl}) {
+		return $self->{pushurl} . (length $self->{path} ? '/' . $self->{path} : '');
+	} else {
+		return $self->full_url;
+	}
+}
 
 sub set_commit_header_env {
 	my ($log_entry) = @_;
-- 
1.7.4.2.1.gd6f1f

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Junio C Hamano]

A Tangent.

>> X-Mailer: git-send-email 1.7.4.2.1.gd6f1f
>> In-Reply-To: <1302102336-8800-1-git-send-email-asedeno@mit.edu>

This is not about this particular patch, but the From: address
git-send-email generates for you does not seem to quote the human readable
part, even though the name has a "." in it.

Your mails seem to reach the recipients fine, but I saw my reply to you
bounce, because "To:" or "Cc:" in my reply end up having the "R." part not
quoted, like this:

  (wrong)  To: Alejandro R. Sedeño <asedeno@mit.edu>
 (correct) To: "Alejandro R. Sedeño" <asedeno@mit.edu>

I wonder if we should do something about it in git-send-email.  Every time
I grab Shawn's address using my "git who" alias, I manually quote his name
to avoid my message thrown into dustbin by vger.

    [alias]
    who = "!sh -c 'git log -1 --format=\"%an <%ae>\" --author=\"$1\"' -"

I suspect Michael is not "Michael J. Gruber" for the same reason...

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 08.04.2011 22:13:
> A Tangent.
> 
>>> X-Mailer: git-send-email 1.7.4.2.1.gd6f1f
>>> In-Reply-To: <1302102336-8800-1-git-send-email-asedeno@mit.edu>
> 
> This is not about this particular patch, but the From: address
> git-send-email generates for you does not seem to quote the human readable
> part, even though the name has a "." in it.
> 
> Your mails seem to reach the recipients fine, but I saw my reply to you
> bounce, because "To:" or "Cc:" in my reply end up having the "R." part not
> quoted, like this:
> 
>   (wrong)  To: Alejandro R. Sedeño <asedeno@mit.edu>
>  (correct) To: "Alejandro R. Sedeño" <asedeno@mit.edu>
> 
> I wonder if we should do something about it in git-send-email.  Every time
> I grab Shawn's address using my "git who" alias, I manually quote his name
> to avoid my message thrown into dustbin by vger.
> 
>     [alias]
>     who = "!sh -c 'git log -1 --format=\"%an <%ae>\" --author=\"$1\"' -"
> 
> I suspect Michael is not "Michael J. Gruber" for the same reason...

Yep, saved a lot of trouble. OTOH, I didn't want to give up completely
on the chance to make my widespread name at least a little less
ambiguous by keeping the middle initial.

Michael

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Jeff King]

On Fri, Apr 08, 2011 at 01:13:47PM -0700, Junio C Hamano wrote:

> A Tangent.
> 
> >> X-Mailer: git-send-email 1.7.4.2.1.gd6f1f
> >> In-Reply-To: <1302102336-8800-1-git-send-email-asedeno@mit.edu>
> 
> This is not about this particular patch, but the From: address
> git-send-email generates for you does not seem to quote the human readable
> part, even though the name has a "." in it.
> 
> Your mails seem to reach the recipients fine, but I saw my reply to you
> bounce, because "To:" or "Cc:" in my reply end up having the "R." part not
> quoted, like this:
> 
>   (wrong)  To: Alejandro R. Sedeño <asedeno@mit.edu>
>  (correct) To: "Alejandro R. Sedeño" <asedeno@mit.edu>

Hmm. His case has an extra level of confusion, though, because the
non-ascii characters all need rfc2047 encoding. So two emails I've seen
from him have:

  From: =?UTF-8?B?IkFsZWphbmRybyBSLiBTZWRlw7FvIg==?= <asedeno@MIT.EDU>
  From: =?UTF-8?q?Alejandro=20R=2E=20Sede=C3=B1o?= <asedeno@mit.edu>

where the first is from Icedove (i.e., Thunderbird) and the second is
from git-send-email.

The first one contains double-quotes embedded in the encoded portion.
The second one (send-email) does not.

But I'm not clear on if that is necessary. I thought that rfc2047 could
only encode a "word" in the "phrase" portion in an address header,
meaning the parsing should be unambiguous.

That being said, I think we are not quoting in the non-rfc2047 case,
anyway, and that is a bug. rfc5322 says this (section 4.1, Miscellaneous
obsolete tokens):

  Note: The "period" (or "full stop") character (".") in obs-phrase is
  not a form that was allowed in earlier versions of this or any other
  specification.  Period (nor any other character from specials) was not
  allowed in phrase because it introduced a parsing difficulty
  distinguishing between phrases and portions of an addr-spec (see
  section 4.4).  It appears here because the period character is
  currently used in many messages in the display-name portion of
  addresses, especially for initials in names, and therefore must be
  interpreted properly.

which recognizes this situation. But being in the obsolete section, I
think it is saying "you still need to interpret these, but don't
generate them". IOW, we should still be generating quotes now.

I think format-patch is totally lacking in this type of quoting. If I
do:

  $ git init
  $ git config user.name '<bogus> with "quotes"'
  $ echo contents >foo && git add . && git commit -m foo
  $ git format-patch --stdout --root
  ...
  From: bogus with "quotes <peff@peff.net>

So some of my magic characters are just stripped, and some of them get
included, making the output bogus (the stripping of <> actually happens
within git, so the commit itself is missing them).

Not that I think a name like that is sane, but probably we should be
double-quoting properly anyway, and then the "." case would just fall
out.

-Peff

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Jeff King]

On Fri, Apr 08, 2011 at 02:22:11PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think format-patch is totally lacking in this type of quoting. If I
> > do:
> >
> >   $ git init
> >   $ git config user.name '<bogus> with "quotes"'
> >   $ echo contents >foo && git add . && git commit -m foo
> >   $ git format-patch --stdout --root
> >   ...
> >   From: bogus with "quotes <peff@peff.net>
> >
> > So some of my magic characters are just stripped, and some of them get
> > included, making the output bogus (the stripping of <> actually happens
> > within git, so the commit itself is missing them).
> 
> The output from format-patch is meant to be slurped into MUA, so I would
> say that they should show what a human user would type the name to
> Thunderbird or message.el or whatever, and it is MUA's respoinsibility to
> make it RFC comformant.  And that is why I mentioned that send-email
> may want to learn a new trick or two.

I disagree. Format-patch claims to make an mbox, so it should make one
that is valid (actually, the documentation is very wishy-washy about
whether it is an mbox; we say "formatted to resemble UNIX mailbox
format" at one point, and "print all commits...in mbox format" later).

But that is a philosophical distinction. What matters in practice is
what MUAs expect to get, and how they deal with it. I expect most MUAs
handle bare "." just fine, because it's fairly unambiguous. But try:

  git init repo && cd repo
  git config user.name 'Jeff "Peff" King' &&
  echo content >>file &&
  git add file &&
  git commit -m one

  git format-patch -1 --stdout >mbox

now open the result in your MUA. Mutt strips the quotes and I become
"Jeff Peff King".

-Peff

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I disagree. Format-patch claims to make an mbox, so it should make one
> that is valid.

OK.  That makes sense (even though I think it would make cutting and
pasting somewhat awkward).

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Jeff King]

On Fri, Apr 08, 2011 at 03:25:09PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I disagree. Format-patch claims to make an mbox, so it should make one
> > that is valid.
> 
> OK.  That makes sense (even though I think it would make cutting and
> pasting somewhat awkward).

Cutting and pasting is already potentially awkward, because we
rfc2047-quote in format-patch, too. But we can still get away without
quoting at all in regular "git log". The "Author:" lines, while
rfc822-looking do not have to be real header lines, and we know that
everything before the <> must be the name.

So I think this is the patch we want:

-- >8 --
Subject: [PATCH] pretty: quote rfc822 specials in email addresses

If somebody has a name that includes an rfc822 special, we
will output it literally in the "From:" header. This is
usually OK, but certain characters (like ".") are supposed
to be enclosed in double-quotes in a mail header.

In practice, whether this matters may depend on your MUA.
Some MUAs will happily take in:

   From: Foo B. Bar <author@example.com>

without quotes, and properly quote the "." when they send
the actual mail.  Others may not, or may screw up harder
things like:

  From: Foo "The Baz" Bar <author@example.com>

For example, mutt will strip the quotes, thinking they are
actual syntactic rfc822 quotes.

So let's quote properly, and then (if necessary) we still
apply rfc2047 encoding on top of that, which should make all
MUAs happy.

Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c                |   61 ++++++++++++++++++++++++++++++++++++++++++++++-
 t/t4014-format-patch.sh |   42 ++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 1 deletions(-)

diff --git a/pretty.c b/pretty.c
index e1d8a8f..8345485 100644
--- a/pretty.c
+++ b/pretty.c
@@ -208,6 +208,58 @@ int has_non_ascii(const char *s)
 	return 0;
 }
 
+static int is_rfc822_special(char ch)
+{
+	switch (ch) {
+	case '(':
+	case ')':
+	case '<':
+	case '>':
+	case '[':
+	case ']':
+	case ':':
+	case ';':
+	case '@':
+	case ',':
+	case '.':
+	case '"':
+	case '\\':
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static int has_rfc822_specials(const char *s, int len)
+{
+	int i;
+	for (i = 0; i < len; i++)
+		if (is_rfc822_special(s[i]))
+			return 1;
+	return 0;
+}
+
+static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
+{
+	int i;
+
+	/* just a guess, we may have to also backslash-quote */
+	strbuf_grow(out, len + 2);
+
+	strbuf_addch(out, '"');
+	for (i = 0; i < len; i++) {
+		switch (s[i]) {
+		case '"':
+		case '\\':
+			strbuf_addch(out, '\\');
+			/* fall through */
+		default:
+			strbuf_addch(out, s[i]);
+		}
+	}
+	strbuf_addch(out, '"');
+}
+
 static int is_rfc2047_special(char ch)
 {
 	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
@@ -293,7 +345,14 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
 			name_tail--;
 		display_name_length = name_tail - line;
 		strbuf_addstr(sb, "From: ");
-		add_rfc2047(sb, line, display_name_length, encoding);
+		if (has_rfc822_specials(line, display_name_length)) {
+			struct strbuf quoted = STRBUF_INIT;
+			add_rfc822_quoted(&quoted, line, display_name_length);
+			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
+			strbuf_release(&quoted);
+		}
+		else
+			add_rfc2047(sb, line, display_name_length, encoding);
 		strbuf_add(sb, name_tail, namelen - display_name_length);
 		strbuf_addch(sb, '\n');
 	} else {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c3cdb52..dd406c4 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -793,4 +793,46 @@ test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
 	test_cmp expect subject
 '
 
+check_author() {
+	echo content >>file &&
+	git add file &&
+	GIT_AUTHOR_NAME=$1 git commit -m author-check &&
+	git format-patch --stdout -1 >patch &&
+	grep ^From: patch >actual &&
+	test_cmp expect actual
+}
+
+cat >expect <<'EOF'
+From: "Foo B. Bar" <author@example.com>
+EOF
+test_expect_success 'format-patch quotes dot in headers' '
+	check_author "Foo B. Bar"
+'
+
+cat >expect <<'EOF'
+From: "Foo \"The Baz\" Bar" <author@example.com>
+EOF
+test_expect_success 'format-patch quotes double-quote in headers' '
+	check_author "Foo \"The Baz\" Bar"
+'
+
+cat >expect <<'EOF'
+From: =?UTF-8?q?"F=C3=B6o=20B.=20Bar"?= <author@example.com>
+EOF
+test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
+	check_author "Föo B. Bar"
+'
+
+cat >expect <<'EOF'
+Subject: header with . in it
+EOF
+test_expect_success 'subject lines do not have 822 atom-quoting' '
+	echo content >>file &&
+	git add file &&
+	git commit -m "header with . in it" &&
+	git format-patch -k -1 --stdout >patch &&
+	grep ^Subject: patch >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.5.rc1.24.gcff72

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Jeff King]

On Fri, Apr 08, 2011 at 06:40:36PM -0400, Jeff King wrote:

> +cat >expect <<'EOF'
> +From: =?UTF-8?q?"F=C3=B6o=20B.=20Bar"?= <author@example.com>
> +EOF
> +test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
> +	check_author "Föo B. Bar"

I'm slightly unsure of this one. Notice that we keep the literal
double-quotes inside the rfc2047 encoded-word. Whereas in Alejandro's
mail sent by Thunderbird, they ended up encoded.

My reading of rfc2047 is that it's OK either way, but I'm not 100% sure.

-Peff

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Eric Wong]

"Alejandro R. Sedeño" <asedeno@mit.edu> wrote:
> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Reviewed-by: James Y Knight <jknight@itasoftware.com>

Thanks all, I've acked and pushed out to git://bogomips.org/git-svn.git

If it's not too much trouble, I'd strongly prefer to have a test case
for this so future work doesn't break it.  Thanks.

-- 
Eric Wong

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

(Sorry I didn't chime in on this tangent. My life has been very busy since
Google finished acquiring ITA Software. At least it's a good kind of busy.
:) )

I'd be happy to see something like this get in. I've often noticed
git-send-email telling me that it couldn't parse an address out of my
signed-of-by lines, but I never stopped and took the time to look into it.

-Alejandro

On 04/08/2011 06:40 PM, Jeff King wrote:
> On Fri, Apr 08, 2011 at 03:25:09PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> I disagree. Format-patch claims to make an mbox, so it should make one
>>> that is valid.
>>
>> OK.  That makes sense (even though I think it would make cutting and
>> pasting somewhat awkward).
> 
> Cutting and pasting is already potentially awkward, because we
> rfc2047-quote in format-patch, too. But we can still get away without
> quoting at all in regular "git log". The "Author:" lines, while
> rfc822-looking do not have to be real header lines, and we know that
> everything before the <> must be the name.
> 
> So I think this is the patch we want:
> 
> -- >8 --
> Subject: [PATCH] pretty: quote rfc822 specials in email addresses
> 
> If somebody has a name that includes an rfc822 special, we
> will output it literally in the "From:" header. This is
> usually OK, but certain characters (like ".") are supposed
> to be enclosed in double-quotes in a mail header.
> 
> In practice, whether this matters may depend on your MUA.
> Some MUAs will happily take in:
> 
>    From: Foo B. Bar <author@example.com>
> 
> without quotes, and properly quote the "." when they send
> the actual mail.  Others may not, or may screw up harder
> things like:
> 
>   From: Foo "The Baz" Bar <author@example.com>
> 
> For example, mutt will strip the quotes, thinking they are
> actual syntactic rfc822 quotes.
> 
> So let's quote properly, and then (if necessary) we still
> apply rfc2047 encoding on top of that, which should make all
> MUAs happy.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  pretty.c                |   61 ++++++++++++++++++++++++++++++++++++++++++++++-
>  t/t4014-format-patch.sh |   42 ++++++++++++++++++++++++++++++++
>  2 files changed, 102 insertions(+), 1 deletions(-)
> 
> diff --git a/pretty.c b/pretty.c
> index e1d8a8f..8345485 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -208,6 +208,58 @@ int has_non_ascii(const char *s)
>  	return 0;
>  }
>  
> +static int is_rfc822_special(char ch)
> +{
> +	switch (ch) {
> +	case '(':
> +	case ')':
> +	case '<':
> +	case '>':
> +	case '[':
> +	case ']':
> +	case ':':
> +	case ';':
> +	case '@':
> +	case ',':
> +	case '.':
> +	case '"':
> +	case '\\':
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int has_rfc822_specials(const char *s, int len)
> +{
> +	int i;
> +	for (i = 0; i < len; i++)
> +		if (is_rfc822_special(s[i]))
> +			return 1;
> +	return 0;
> +}
> +
> +static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
> +{
> +	int i;
> +
> +	/* just a guess, we may have to also backslash-quote */
> +	strbuf_grow(out, len + 2);
> +
> +	strbuf_addch(out, '"');
> +	for (i = 0; i < len; i++) {
> +		switch (s[i]) {
> +		case '"':
> +		case '\\':
> +			strbuf_addch(out, '\\');
> +			/* fall through */
> +		default:
> +			strbuf_addch(out, s[i]);
> +		}
> +	}
> +	strbuf_addch(out, '"');
> +}
> +
>  static int is_rfc2047_special(char ch)
>  {
>  	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
> @@ -293,7 +345,14 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
>  			name_tail--;
>  		display_name_length = name_tail - line;
>  		strbuf_addstr(sb, "From: ");
> -		add_rfc2047(sb, line, display_name_length, encoding);
> +		if (has_rfc822_specials(line, display_name_length)) {
> +			struct strbuf quoted = STRBUF_INIT;
> +			add_rfc822_quoted(&quoted, line, display_name_length);
> +			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
> +			strbuf_release(&quoted);
> +		}
> +		else
> +			add_rfc2047(sb, line, display_name_length, encoding);
>  		strbuf_add(sb, name_tail, namelen - display_name_length);
>  		strbuf_addch(sb, '\n');
>  	} else {
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index c3cdb52..dd406c4 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -793,4 +793,46 @@ test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
>  	test_cmp expect subject
>  '
>  
> +check_author() {
> +	echo content >>file &&
> +	git add file &&
> +	GIT_AUTHOR_NAME=$1 git commit -m author-check &&
> +	git format-patch --stdout -1 >patch &&
> +	grep ^From: patch >actual &&
> +	test_cmp expect actual
> +}
> +
> +cat >expect <<'EOF'
> +From: "Foo B. Bar" <author@example.com>
> +EOF
> +test_expect_success 'format-patch quotes dot in headers' '
> +	check_author "Foo B. Bar"
> +'
> +
> +cat >expect <<'EOF'
> +From: "Foo \"The Baz\" Bar" <author@example.com>
> +EOF
> +test_expect_success 'format-patch quotes double-quote in headers' '
> +	check_author "Foo \"The Baz\" Bar"
> +'
> +
> +cat >expect <<'EOF'
> +From: =?UTF-8?q?"F=C3=B6o=20B.=20Bar"?= <author@example.com>
> +EOF
> +test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
> +	check_author "Föo B. Bar"
> +'
> +
> +cat >expect <<'EOF'
> +Subject: header with . in it
> +EOF
> +test_expect_success 'subject lines do not have 822 atom-quoting' '
> +	echo content >>file &&
> +	git add file &&
> +	git commit -m "header with . in it" &&
> +	git format-patch -k -1 --stdout >patch &&
> +	grep ^Subject: patch >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Jeff King]

On Fri, Apr 22, 2011 at 03:11:46PM -0400, "Alejandro R. Sedeño" wrote:

> I'd be happy to see something like this get in. I've often noticed
> git-send-email telling me that it couldn't parse an address out of my
> signed-of-by lines, but I never stopped and took the time to look into it.

That is a separate issue. The patch I posted will make format-patch
properly quote rfc822 specials in actual email header lines. But
Signed-off-by lines will remain as they are, being part of the message
body. And I think that's what we want. You certainly wouldn't want to
rfc2047-encode them (they are already covered by the body's content-type
encoding).

And while you could quote rfc822 specials, people don't tend to do that.
There's no formal syntax defined, but people tend to treat them as:

  Signed-off-by: (.*) <(.*)>

i.e., everything inside <> is an address, and everything before that is
the name.

If there is an issue with send-email parsing signed-off-by lines or
formatting the addresses it pulls from them, that's a separate problem
that will need to be dealt with in send-email.

-Peff

Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

["Alejandro R. Sedeño"]

On 04/22/2011 03:36 PM, Jeff King wrote:
> On Fri, Apr 22, 2011 at 03:11:46PM -0400, "Alejandro R. Sedeño" wrote:
>> I'd be happy to see something like this get in. I've often noticed
>> git-send-email telling me that it couldn't parse an address out of my
>> signed-of-by lines, but I never stopped and took the time to look into it.
> 
> That is a separate issue. The patch I posted will make format-patch
> properly quote rfc822 specials in actual email header lines. But
> Signed-off-by lines will remain as they are, being part of the message
> body. And I think that's what we want. You certainly wouldn't want to
> rfc2047-encode them (they are already covered by the body's content-type
> encoding).
> 
> And while you could quote rfc822 specials, people don't tend to do that.
> There's no formal syntax defined, but people tend to treat them as:
> 
>   Signed-off-by: (.*) <(.*)>
> 
> i.e., everything inside <> is an address, and everything before that is
> the name.
> 
> If there is an issue with send-email parsing signed-off-by lines or
> formatting the addresses it pulls from them, that's a separate problem
> that will need to be dealt with in send-email.

Okay, then I'll try look into that when things settle down here, if no one
else has by then.

-Alejandro