Bug #29730 for SVN-Notify: (feature request) --svnlook-diff option

Subject:	(feature request) --svnlook-diff option
Date:	Wed, 03 Oct 2007 16:13:52 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	kin <kaneyuki [...] eva.hi-ho.ne.jp>

Hello, Our development team are big fans of SVN::Notify. It is much helpful if we can specify "svnlook diff" filter program by command line option, like this: --svnlook-diff /usr/local/bin/svnlook_diff_sjis In above example, we use following filter script as svnlook_diff_sjis: #!/bin/sh PATH=/bin:/usr/bin env LANG=C svnlook "$@" | iconv -f SHIFT-JIS -t UTF-8 Can you take this feature in? Here is the patch against v2.66, w/o documentation part: --- Notify.pm.org 2007-09-05 09:35:16.000000000 +0900 +++ Notify.pm 2007-09-05 10:29:50.000000000 +0900 @@ -647,16 +647,17 @@ $params{svnlook} ||= $ENV{SVNLOOK} || $class->find_exe('svnlook'); $params{with_diff} ||= $params{attach_diff}; $params{verbose} ||= 0; $params{charset} ||= 'UTF-8'; $params{io_layer} ||= "encoding($params{charset})"; $params{smtp_authtype} ||= 'PLAIN'; $params{sendmail} ||= $ENV{SENDMAIL} || $class->find_exe('sendmail') unless $params{smtp}; + $params{svnlook_diff} ||= $params{svnlook}; die qq{Cannot find sendmail and no "smtp" parameter specified} unless $params{sendmail} || $params{smtp}; # Set up the revision URL. $params{revision_url} ||= delete $params{svnweb_url} || delete $params{viewcvs_url}; if ($params{revision_url} && $params{revision_url} !~ /%s/) { @@ -767,16 +768,17 @@ 'repos-path|p=s' => \$opts->{repos_path}, 'revision|r=s' => \$opts->{revision}, 'to|t=s@' => \$opts->{to}, 'to-regex-map|x=s%' => \$opts->{to_regex_map}, 'to-email-map=s%' => \$opts->{to_email_map}, 'from|f=s' => \$opts->{from}, 'user-domain|D=s' => \$opts->{user_domain}, 'svnlook|l=s' => \$opts->{svnlook}, + 'svnlook-diff=s' => \$opts->{svnlook_diff}, 'sendmail|s=s' => \$opts->{sendmail}, 'set-sender|E' => \$opts->{set_sender}, 'smtp=s' => \$opts->{smtp}, 'charset|c=s' => \$opts->{charset}, 'io-layer|o=s' => \$opts->{io_layer}, 'language|g=s' => \$opts->{language}, 'with-diff|d' => \$opts->{with_diff}, 'attach-diff|a' => \$opts->{attach_diff}, @@ -1549,17 +1551,17 @@ will usually be passed as the second argument to C<output_diff()> or C<output_attached_diff()>. =cut sub diff_handle { my $self = shift; return $self->_pipe( - '-|' => $self->{svnlook}, + '-|' => $self->{svnlook_diff}, 'diff' => $self->{repos_path}, '-r' => $self->{revision}, ( $self->{diff_switches} ? grep { defined && $_ ne '' } # Allow quoting of arguments, but strip out the quotes. split /(?:'([^']+)'|"([^"]+)")?\s+(?:'([^']+)'|"([^"]+)")?/, $self->{diff_switches} : () @@ -1607,16 +1609,17 @@ __PACKAGE__->_accessors(qw( repos_path revision to_regex_map to_email_map from user_domain svnlook + svnlook_diff sendmail set_sender add_headers smtp charset io_layer language with_diff Thanks in advance. ================ Toshikazu Kinkoh kaneyuki@eva.hi-ho.ne.jp

Fri Oct 05 00:49:46 2007 david [...] kineticode.com - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Thu, 4 Oct 2007 21:48:38 -0700
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <david [...] kineticode.com>

On Oct 3, 2007, at 00:19, kin via RT wrote: Show quoted text

> Our development team are big fans of SVN::Notify.

Thanks. :-) Show quoted text

> It is much helpful if we can specify "svnlook diff" filter program by > command line option, like this: > > --svnlook-diff /usr/local/bin/svnlook_diff_sjis > > In above example, we use following filter script as svnlook_diff_sjis: > #!/bin/sh > PATH=/bin:/usr/bin > env LANG=C svnlook "$@" | iconv -f SHIFT-JIS -t UTF-8 > > Can you take this feature in?

Hrm. Could you not just use the --charset and --io-layer options? svnnotify --charset SHIFT-JIS --io-layer raw Does that not properly collect the log message and diff output in SHIFT-JIS for you? Best, David

Fri Oct 05 00:50:05 2007 The RT System itself - Status changed from 'new' to 'open'

Mon Oct 08 00:06:09 2007 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Mon, 08 Oct 2007 12:59:15 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	kin <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, Thank you for the reply. Show quoted text

> Hrm. Could you not just use the --charset and --io-layer options? > > svnnotify --charset SHIFT-JIS --io-layer raw > > Does that not properly collect the log message and diff output in > SHIFT-JIS for you?

No, it slays log message. Our repository runs UTF-8 base, so the log messages must be stored in UTF-8. And most files stored in the repository are encoded in SHIFT-JIS, so raw diff output is SHIFT-JIS. My understanding for getting uncorrupted strings both for log message and diff output (in one e-mail) is to convert both outputs in different way. And it is the need for --snvlook-diff option. FYI, we are using following options with hacked --svnlook-diff option: --with-diff \ --language ja \ --charset UTF-8 \ --io-layer raw \ --svnlook /usr/local/bin/svnlook2utf8 \ Please point out if we are doing something wrong. Best regards, Toshikazu

Mon Oct 08 14:17:46 2007 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Mon, 8 Oct 2007 11:17:11 -0700
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Oct 7, 2007, at 21:06, kin via RT wrote: Show quoted text

> No, it slays log message. > > Our repository runs UTF-8 base, so the log messages must be stored in > UTF-8. And most files stored in the repository are encoded in > SHIFT-JIS, > so raw diff output is SHIFT-JIS. > > My understanding for getting uncorrupted strings both for log > message and > diff output (in one e-mail) is to convert both outputs in different > way. > And it is the need for --snvlook-diff option.

Ah, I understand. Well, what if we added a --diff-charset option? -- charset would apply to both the log message and the diff, unless -- diff-charset was specified, in which case it would use its own character set. That way you can specify two different character sets, one for the log message and one for the diff. Does that make sense? Best, David

Wed Oct 10 02:34:23 2007 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 10 Oct 2007 15:28:43 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, Show quoted text

> Ah, I understand. Well, what if we added a --diff-charset option? -- > charset would apply to both the log message and the diff, unless -- > diff-charset was specified, in which case it would use its own > character set. That way you can specify two different character sets, > one for the log message and one for the diff. > > Does that make sense?

Oh, it sounds better solution. I think --diff-charset option would work diff outputs to be converted from specified charset to the charset specified by --charset option, because --charset determines the final charset for email. Is that right ? Best regards, Toshikazu

Wed Oct 10 13:49:53 2007 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 10 Oct 2007 10:48:27 -0700
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Oct 9, 2007, at 23:34, kin via RT wrote: Show quoted text

> Oh, it sounds better solution. > > I think --diff-charset option would work diff outputs to be > converted from > specified charset to the charset specified by --charset option, > because > --charset determines the final charset for email. Is that right ?

Yes. Care to submit a patch? It'll probably be a couple of months before I can work on this (and a million other planned improvements for SVN::Notify)… Thanks, David

Thu Oct 11 08:25:10 2007 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Thu, 11 Oct 2007 21:19:17 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, Show quoted text

> Yes. Care to submit a patch? It'll probably be a couple of months > before I can work on this (and a million other planned improvements > for SVN::Notify)…

Well, ok. I'll try it, but I'm not very well aware of perl. So please wait patiently. And where should I send a patch to ? This email address ? Best regards, Toshikazu

Thu Oct 11 14:57:01 2007 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Thu, 11 Oct 2007 11:54:50 -0700
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Oct 11, 2007, at 05:25, kin via RT wrote: Show quoted text

>> Yes. Care to submit a patch? It'll probably be a couple of months >> before I can work on this (and a million other planned improvements >> for SVN::Notify)…

> > Well, ok. I'll try it, but I'm not very well aware of perl. > So please wait patiently.

No problem. It should be quite similar to how --charset is handled now. Show quoted text

> And where should I send a patch to ? This email address ?

Yep, that would be fine. Thanks! David

Tue Nov 20 09:59:58 2007 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Tue, 20 Nov 2007 23:53:27 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Message body is not shown because it is too large.

Wed Feb 06 14:16:33 2008 dwheeler [...] cpan.org - Severity Wishlist added

Wed Feb 06 15:13:38 2008 dwheeler [...] cpan.org - Correspondence added

On Tue Nov 20 09:59:58 2007, kaneyuki@eva.hi-ho.ne.jp wrote: Show quoted text

> Hi David,

Hi Toshikazu. Apologies for the delayed reply. Apparently RT thought that your message was "too big", and so didn't forward it to me. I'm working on SVN::Notify now, and so finally saw your patch as I was going through the RT queue. Show quoted text

> I have finished implementation for --diff-charset option.

Many thanks for your hard work on this. It looks great! Tell me, are you using this patch in production? And if so, are the encodings now all as they should be? Show quoted text

> Basic ideas for the change are: > 1. To encode input pipe for svnlook properly. > 2. To keep svnlook outputs as PerlIO internal expression until just > before printing them to mail pipe. > 3. To decode them to output encoding (specified by --charset option) > by ourselves just before printing them to mail pipe. > 4. Mail pipe simply receives octet stream. > > For this change, I have to add two more options. > Here is the brief descriptions for added options, and the changes of > meaning for > some existing options. > > --env-lang [New] > Specify LANG environmental variable, on which svnlook runs. > Defaults to undef, usually only people dealing multi byte character > should specify.

Very good idea. I've tried to set this to 'C' in the past to solve encoding problems, but making it an option is a much better idea. I think it should default to the value of --charset though, no? Show quoted text

> --server-charset [New] > Character set to be set to svnlook input pipe. This should be > consistent with the setting of --env-lang option. For example: > --env-lang "ja_JP.eucJP" --server-charset "euc-jp" > Defaults to 'ascii'.

Hrm. Should it not then default to the proper equivalent of --env-lang? I'm trying to limit how many options the user actually has to specify. And since more and more folks are using UTF-8 for nearly everything (outside of a few well-known geolocations, yours included), if --charset was set to utf-8, then --env-lang should default to, perhaps, 'en_US.UTF8', and then --server-charset would default to 'utf-8'. Or does it make sense to leave some of these blank? At any rate, I'd prefer a default of utf-8 to ascii. Show quoted text

> --diff-charset [New] > Character set used by svnlook diff. This is the character set > mainly used as actual file contents in the repository. For example: > --diff-charset "shiftjis" > Defaults to the same value as --server-charsest.

Great, perfect. Show quoted text

> --charset [meaning change] > Basically this specifies simply the output character set, resulting > in mail and HTML character set. This is independent how svnlook runs > on server. > For example: > --charset "UTF-8" (default) > --charset "iso-8859-1" > --charset "iso-2022-jp"

Well, maybe then we need --output-charset, but if a user specifies --charset and nothing else, we try to set all of the other encoding-related options to their appropriate values. Thoughts? Show quoted text

> --io-layer [meaning and default value change] > Modified defaults to undef. And this option is basically obsolete.

So this is deprecated, then. Show quoted text

> The people using single byte character set need not change current > options of svnnotify.

That's fewer and fewer folks every day. :-) Show quoted text

> Below details the modification (and you can skip reading it ...): > ----------------------------------------------------------------- > First of all, hook scripts of svn are executed under empty > environmental variables.

Right. Or, well, maybe. It depends on how the system is set up, IIRC. Show quoted text

> This is perhaps equivalent to LANG=C, so when svnlook outputs > multibyte characters under that situation, the output is broken > for the first moment.

Good point. I wonder why it has been working so well up till now (impefectly, but not badly). Show quoted text

> For this reason, --env-lang option was added. This option defaults > undef and if it is not specified, svnnotify should behave > identically as current version.

Right, good. Show quoted text

> When the --env-lang option is specified appropriately and the encoding > for input pipe of svnlook is consistently specified, then commit > messages will not be broken. This encoding is specified by > --server-charset option.

Like I said, I'd like to set this to a reasonable default relative to the value of --env-lang. Something like this (pseudo-code): if ( $opts->{env_lang} && !$opts->{server_charset} ) { $opts->{server_charset} = $opts->{env_lang} =~ /[.]UTF-?8/ ? 'UTF-8' : split( '.', $opts->{env_lang})[-1] =~ s/_/-/g; } Show quoted text

> svnlook diff outputs diff as binary data, and they are received with > the encoding specified by --diff-charset.

Good. Show quoted text

> Item #3 in above listing stands on that specifying encoding to tied_fh > of SMTP by binmode has no effect. This is the case of "When Unicode > Does Not Happen" in perlunicode doc.

Ah, I didn't know that. No wonder it never worked! Show quoted text

> For this reason, --io-layer has been modified to default to undef, and > normally it is recommended not to be specified.

Yes, we should deprecate it. Show quoted text

> Also for the change of #3, HTML.pm and ColorDiff.pm are modified.

Right, makes sense. Show quoted text

> ----------------------------------------------------------------- > > > And finally the patch against 2.66:

Thanks, this looks good. You obviously worked on it a lot! I'm going to spend a bit of time distilling it a little, and then I'll send a patch back to you via RT to test. Would you mind doing that? Thanks! David

Wed Feb 06 20:13:28 2008 dwheeler [...] cpan.org - Correspondence added

On Wed Feb 06 15:13:38 2008, DWHEELER wrote: Show quoted text

> Thanks, this looks good. You obviously worked on it a lot! I'm going to > spend a bit of time distilling it a little, and then I'll send a patch > back to you via RT to test. Would you mind doing that?

Hi Toshikazu, I've now integrated your patch. I changed quite a bit from how you had it, but I *think* it's correct: * I used the `language` attribute to set `env_lang` rather than add another option. It was already being used for this purpose, as it happens. Let me know if it works properly, or if I need to strip out dashes from character sets to make it work properly. * I named the option --svn-charset instead of --server-charset. It defaults to the same value as --charset. * -diff-charset deffaults to the same value as --svn-charset. * I eliminated --io-layer altogether. Did I understand you right that it is essentially a no-op? * Rather than have separate _pipe and _read_pipe methods, I just integrated the encodings into the existing methods. This eliminates a lot of duplication. * I changed a couple of other method names: _encode instead of _octets and print_lines instead of print_octets. * I added documentation and got all tests passing. :-) Would you mind giving this patch a try and letting me know how it works for you? If it's easier (it should apply cleanly to 2.67), you can pull it directly from Subversion: https://svn.kineticode.com/SVN-Notify/trunk/ Please let me know if this works for you. I'd like to release it soon, but I'd really like to hear from you that it works in the wild before I do. Thanks! David

Message body is not shown because it is too large.

Sun Feb 10 03:03:59 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Sun, 10 Feb 2008 16:56:18 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I'm happy to hear you basically accept my patch! Show quoted text

> Hi Toshikazu. Apologies for the delayed reply. Apparently RT thought > that your message was "too big", and so didn't forward it to me. I'm > working on SVN::Notify now, and so finally saw your patch as I was going > through the RT queue.

Aaahhh, I'm sorry. Show quoted text

> Many thanks for your hard work on this. It looks great! Tell me, are you > using this patch in production? And if so, are the encodings now all as > they should be?

I'm using the patched version in production for about 3 months, and working fine. Show quoted text

> Hrm. Should it not then default to the proper equivalent of --env-lang? > I'm trying to limit how many options the user actually has to specify. > And since more and more folks are using UTF-8 for nearly everything > (outside of a few well-known geolocations, yours included), if --charset > was set to utf-8, then --env-lang should default to, perhaps, > 'en_US.UTF8', and then --server-charset would default to 'utf-8'. Or > does it make sense to leave some of these blank? At any rate, I'd prefer > a default of utf-8 to ascii.

I'd forgotten that language and charset form $LANG at that time, and I agree with you. On Wed, 06 Feb 2008 20:13:40 -0500 David Wheeler via RT <bug-SVN-Notify@rt.cpan.org>-san wrote: Show quoted text

> I've now integrated your patch. I changed quite a bit from how you had > it, but I *think* it's correct:

Ok. Show quoted text

> * I used the `language` attribute to set `env_lang` rather than add > another option. It was already being used for this purpose, as it > happens. Let me know if it works properly, or if I need to strip out > dashes from character sets to make it work properly.

I will answer after investigation and test run. Show quoted text

> * I eliminated --io-layer altogether. Did I understand you right that > it is essentially a no-op?

Yes. Show quoted text

> * I added documentation and got all tests passing. :-)

Good! Thank you. Show quoted text

> Would you mind giving this patch a try and letting me know how it works > for you? If it's easier (it should apply cleanly to 2.67), you can pull > it directly from Subversion: > > https://svn.kineticode.com/SVN-Notify/trunk/ > > Please let me know if this works for you. I'd like to release it soon, > but I'd really like to hear from you that it works in the wild before I do.

Ok, I will test this week and let you know the result. Best regards. Toshikazu

Sun Feb 10 13:10:35 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Sun, 10 Feb 2008 10:10:05 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 10, 2008, at 00:04, kin via RT wrote: Show quoted text

>> Hi Toshikazu. Apologies for the delayed reply. Apparently RT thought >> that your message was "too big", and so didn't forward it to me. I'm >> working on SVN::Notify now, and so finally saw your patch as I was >> going >> through the RT queue.

> > Aaahhh, I'm sorry.

No worries. I would not have had time to work on it before now, anyway. Show quoted text

> I'd forgotten that language and charset form $LANG at that time, and > I agree > with you.

Great. Show quoted text

>> Please let me know if this works for you. I'd like to release it >> soon, >> but I'd really like to hear from you that it works in the wild >> before I do.

> > Ok, I will test this week and let you know the result.

Brilliant, many thanks. David

Tue Feb 19 09:25:46 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Tue, 19 Feb 2008 23:15:56 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I've been testing the patched version that you sent and it works fine with small modifications. I attach the patch. It will be applied on patched v2.67 that your patch has been applied. Here is brief explanations about my modification: *HTML.pm Some print calls were replaced by print_lines. *Notify.pm I think the last half of $LANG should be created from svn_charset (1st hunk). Show quoted text

> > * I used the `language` attribute to set `env_lang` rather than add > > another option. It was already being used for this purpose, as it > > happens. Let me know if it works properly, or if I need to strip out > > dashes from character sets to make it work properly.

> > I will answer after investigation and test run.

I think '-' in language-and-country of $LANG should be replaced by '_', and '-' in encoding part should be stripped out unless it is 'UTF-8' (1st hunk). Some print calls were replaced by print_lines (2nd hunk). It was recognized that they should be so just after I sent the first patch, and has been tested in a couple of months. Print call in _dbpnt was replaced print_lines (3rd hunk). This is very helpful when we debug with various charsets. Otherwise we will get many complains saying "Wide character in print at ....", so please integrate this modification if you not mind. Many thanks! Toshikazu

Message body is not shown because sender requested not to inline it.

Tue Feb 19 14:25:18 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Tue, 19 Feb 2008 11:24:21 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 19, 2008, at 06:25, kin via RT wrote: Show quoted text

> I've been testing the patched version that you sent and it works fine > with small modifications. > I attach the patch. It will be applied on patched v2.67 that your > patch > has been applied

Many thanks, Toshikazu. I've applied it in r3439, along with some fixes for the subject line encoding. Can you check it out and give it a try? svn co https://svn.kineticode.com/SVN-Notify/trunk/ The only change I made was to use the `charset` parameter to create `env_lang`, rather than `svn_charset`. I think that this is more likely to be the expected value, and if it's not, users can always explicitly set the `$LANG` environment variable before executing `svnnotify`, in which case SVN::Notify will not set it. I'm looking to do a release in the next day or two with all of your fixes, plus a new feature: output filtering. Thanks for all your help! David

Wed Feb 20 04:22:42 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 20 Feb 2008 18:14:37 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, Thank you for integration of the patch. On Tue, 19 Feb 2008 14:25:21 -0500 David Wheeler via RT <bug-SVN-Notify@rt.cpan.org>-san wrote: Show quoted text

> The only change I made was to use the `charset` parameter to create > `env_lang`, rather than `svn_charset`. I think that this is more > likely to be the expected value, and if it's not, users can always > explicitly set the `$LANG` environment variable before executing > `svnnotify`, in which case SVN::Notify will not set it.

Well, then svnnotify results in error in a certain test case... (I have confirmed on trunk source, r3442) My test case is as follows: --language ja-JP \ --charset iso-2022-jp \ --svn-charset eucJP \ --diff-charset shiftjis \ The charset iso-2022-jp is one of standard encoding for Japanese email and widely used, so this is expected as one of normal usage. Suppose user wants to receive email notification which is encoded in iso-2022-jp, so we set charset parameter to be so. When svnnotify runs with above parameters, it makes $LANG to be ja_JP.iso2022jp then resulting runtime error because no such name supported. Here is the error outputs: svnlook: error: cannot set LC_ALL locale svnlook: error: environment variable LANG is ja-JP.iso2022jp svnlook: error: please check that your locale name is correct I think svn_charset specifies the output from svnlook on server side so indeed specified in _pipe() parameter, but $ENV{LANG} that is set just before it specifies another charset. They should be consistent so I said $LANG should be made from svn_charset not from charset. Please point out if I have something misunderstanding. Best, Toshikazu

Wed Feb 20 16:51:46 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 20 Feb 2008 12:49:33 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 20, 2008, at 01:22, kin via RT wrote: Show quoted text

> Thank you for integration of the patch.

My pleasure. I've been doing a lot more work on it, though, and now I don't think I'm going to need Encode anymore. I have `binmode` doing the right thing and am properly converting everything to Perl's utf8 format when bringing it in. This will make things a lot cleaner. Please do give r3450 a try and let me know what you think. More on that revision below: Show quoted text

> Well, then svnnotify results in error in a certain test case... (I > have > confirmed on trunk source, r3442) > > My test case is as follows: > --language ja-JP \ > --charset iso-2022-jp \ > --svn-charset eucJP \ > --diff-charset shiftjis \ > > The charset iso-2022-jp is one of standard encoding for Japanese > email and widely used, so this is expected as one of normal usage.

Of course. It looks like I had incorrectly set up the language determination to do `s/_/-/` instead of `s/-/_/`. On my system at least, it should be "ja_JP", not "ja-JP", when setting the $LANG environment variable: % locale -a | grep ja ja_JP ja_JP.eucJP ja_JP.SJIS ja_JP.UTF-8 So I fixed that. Show quoted text

> Suppose user wants to receive email notification which is encoded > in iso-2022-jp, so we set charset parameter to be so.

Right. And it looks like it's supported, according to Encode::Supported. Show quoted text

> When svnnotify runs with above parameters, it makes $LANG to be > ja_JP.iso2022jp then resulting runtime error because no such name > supported.

Hrm. Show quoted text

> Here is the error outputs: > svnlook: error: cannot set LC_ALL locale > svnlook: error: environment variable LANG is ja-JP.iso2022jp > svnlook: error: please check that your locale name is correct

This might be the problem that I fixed above. Does your OS have a locale named "ja_JP.iso2022jp"? Mine doesn't (Mac OS X 10.5.2), but if yours does, it should work much better as of r3450. Show quoted text

> I think svn_charset specifies the output from svnlook on server > side so indeed specified in _pipe() parameter, but $ENV{LANG} > that is set just before it specifies another charset. > They should be consistent so I said $LANG should be made from > svn_charset not from charset.

Yes, also in r3450, I've changed it so that there are now three env_lang attributes: env_lang: Used for the call to sendmail. svn_env_lang: Used for calls to svnlook diff_env_lang: Used for calls to svnlook diff So that should make things better, too, since it now sets $LANG as appropriate for each setting of charset, svn-charset, and diff-charset. Show quoted text

> Please point out if I have something misunderstanding.

No, I think it was my bug. Please do give r3450 or later a shot and let me know what you think. Thanks, David

Fri Feb 22 22:02:49 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Sat, 23 Feb 2008 10:43:09 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, On Wed, 20 Feb 2008 16:51:52 -0500 David Wheeler via RT <bug-SVN-Notify@rt.cpan.org>-san wrote: Show quoted text

> Of course. It looks like I had incorrectly set up the language > determination to do `s/_/-/` instead of `s/-/_/`. On my system at > least, it should be "ja_JP", not "ja-JP", when setting the $LANG > environment variable: > > % locale -a | grep ja > ja_JP > ja_JP.eucJP > ja_JP.SJIS > ja_JP.UTF-8 > > So I fixed that.

Ok, thank you. Show quoted text

> > When svnnotify runs with above parameters, it makes $LANG to be > > ja_JP.iso2022jp then resulting runtime error because no such name > > supported.

> > Hrm. >

> > Here is the error outputs: > > svnlook: error: cannot set LC_ALL locale > > svnlook: error: environment variable LANG is ja-JP.iso2022jp > > svnlook: error: please check that your locale name is correct

> > This might be the problem that I fixed above. Does your OS have a > locale named "ja_JP.iso2022jp"? Mine doesn't (Mac OS X 10.5.2), but if > yours does, it should work much better as of r3450.

No my environment also does not support "ja_JP.iso2022jp". It is common in email encoding but normally unsupported as LANG environment variable. So the hyphen-underbar problem is not responsible for it. I will try to summarize the situation and my thinking: 1. iso-2022-jp is not valid encoding as $LANG so we will get the same result even if we fix hyphen-underbar problem. 2. In this case, we only want the mail encoded with iso-2022-jp, but it is not necessary that svnlook runs under that encoding on server at all. 3. Here is my understanding about encoding transition on above test case: svnlook --> [eucJP -> internal exp.] --> [internal exp. -> iso-2022-jp ] --> sendmail --svn-charset (UTF-8) --charset svnlook diff --> [shift-jis -> internal exp.] --> [internal exp. -> iso-2022-jp] --> sendmail --diff-charset (UTF-8) --charset As shown above, encoding specified by --charset is only effective when the outputs will be sent to receivers via sendmail. 4. I'm not familiar with sendmail, but I suspect it doesn't require $LANG setting when it runs. We give sendmail just octet stream that was appropriately encoded with what we want. If so, we only should set $LANG to "<--language>.<--svn-charset>" at beginning and only should reset before svnlook-diff to "C". Other complex control is not required. I think this is simple and clear. Show quoted text

> > I think svn_charset specifies the output from svnlook on server > > side so indeed specified in _pipe() parameter, but $ENV{LANG} > > that is set just before it specifies another charset. > > They should be consistent so I said $LANG should be made from > > svn_charset not from charset.

> > Yes, also in r3450, I've changed it so that there are now three > env_lang attributes: > > env_lang: Used for the call to sendmail. > svn_env_lang: Used for calls to svnlook > diff_env_lang: Used for calls to svnlook diff > > So that should make things better, too, since it now sets $LANG as > appropriate for each setting of charset, svn-charset, and diff-charset. >

> > Please point out if I have something misunderstanding.

> > No, I think it was my bug. Please do give r3450 or later a shot and > let me know what you think.

I don't know these changes are really necessary, but I will test the properties. Can you please tell me how to specify the properties? Thanks, Toshikazu

Sat Feb 23 00:25:34 2008 david [...] justatheory.com - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Fri, 22 Feb 2008 21:23:41 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <david [...] justatheory.com>

On Feb 22, 2008, at 19:02, kin via RT wrote: Show quoted text

> No my environment also does not support "ja_JP.iso2022jp". It is > common > in email encoding but normally unsupported as LANG environment > variable. > So the hyphen-underbar problem is not responsible for it.

I see. Show quoted text

> I will try to summarize the situation and my thinking: > > 1. iso-2022-jp is not valid encoding as $LANG so we will get the same > result even if we fix hyphen-underbar problem.

Right. Show quoted text

> 2. In this case, we only want the mail encoded with iso-2022-jp, but > it is > not necessary that svnlook runs under that encoding on server at all.

Okay. Show quoted text

> 3. Here is my understanding about encoding transition on above test > case: > > svnlook --> [eucJP -> internal exp.] --> [internal exp. -> iso-2022- > jp ] --> sendmail > --svn-charset > (UTF-8) --charset > > svnlook diff --> [shift-jis -> internal exp.] --> [internal exp. -> > iso-2022-jp] --> sendmail > --diff-charset > (UTF-8) --charset

I don't really follow this, but see below. Show quoted text

> As shown above, encoding specified by --charset is only effective when > the outputs will be sent to receivers via sendmail.

Yes. If you specify --svn-charset, then --charset applies only to what gets generated in the email. If you also specfy --diff-charset, then -- svn-charset is only applied to `svn info` and `svn dirs-changed`. Show quoted text

> 4. I'm not familiar with sendmail, but I suspect it doesn't require > $LANG > setting when it runs. We give sendmail just octet stream that was > appropriately encoded with what we want.

Yes, I think that's right. I wish the same was true of svnlook. :-( Show quoted text

> If so, we only should set $LANG to "<--language>.<--svn-charset>" at > beginning and only should reset before svnlook-diff to "C".

I've modified the code so that it sets $LANG to "<--language>.<--svn- Show quoted text

charset>" when it calls `svnlook info` and `svnlook dirs-changed`. It

sets $LANG to "<--language>.<--diff-charset>" for the call to `svnlook diff`. And it sets it to "<--language>.<--charset>" for sendmail just to be safe. I think taht covers all that you need. SVN::Notify no longer sets $LANG to "C", though, as `svnlook diff` does not seem to pay attention to $LANG (gotta love the consistency, eh? NOT). Show quoted text

> Other complex control is not required. I think this is simple and > clear.

Yes, I think we're on the same page here. Please let me know if I'm wrong. Show quoted text

> I don't know these changes are really necessary, but I will test the > properties. Can you please tell me how to specify the properties?

I think you want svnnotify -p "$1" -r "$2" --charset iso-2022-jp \ --svn-charset eucJP --diff-charset shift-jis I'm assuming that: 1. You want mail sent in iso-2022-jp 2. Your commit log messages and file names are in eucJP 3. Your source code is in shift-jis If not, modify accordingly. Many thanks, David

Sat Feb 23 00:33:57 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Fri, 22 Feb 2008 21:33:00 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

Show quoted text

> > I think you want > > svnnotify -p "$1" -r "$2" --charset iso-2022-jp \ > --svn-charset eucJP --diff-charset shift-jis

Um, make that: svnnotify -p "$1" -r "$2" --charset iso-2022-jp \ --svn-charset eucJP --diff-charset shift-jis --language ja_JP \ --with-diff Sorry to have omitted those last two bits. David

Sat Feb 23 12:19:47 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Sun, 24 Feb 2008 02:11:00 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I'm really glad to see we're on the same wavelength on this issue. Show quoted text

>> As shown above, encoding specified by --charset is only effective when >> the outputs will be sent to receivers via sendmail.

> >Yes. If you specify --svn-charset, then --charset applies only to what >gets generated in the email. If you also specfy --diff-charset, then -- >svn-charset is only applied to `svn info` and `svn dirs-changed`.

Okay, my understanding agrees exactly what you says. Show quoted text

>I've modified the code so that it sets $LANG to "<--language>.<--svn-

>charset>" when it calls `svnlook info` and `svnlook dirs-changed`. It

>sets $LANG to "<--language>.<--diff-charset>" for the call to `svnlook >diff`. And it sets it to "<--language>.<--charset>" for sendmail just >to be safe. I think taht covers all that you need. SVN::Notify no >longer sets $LANG to "C", though, as `svnlook diff` does not seem to >pay attention to $LANG (gotta love the consistency, eh? NOT).

Thanks, this seems to be almost good. But as for `svnlook diff`, $LANG needs still set to "C". Why it should be is commented shortly in source (see below in detail). Show quoted text

>`svnlook diff` does not seem to pay attention to $LANG

Urm, that is true for diff contents themselves, but other output is effected. For example, "Modified:" keyword shown in `svnlook diff` output will be localized according to $LANG. In Japanese it is shown as "変更:". This meddlesomeness of svnlook is harmful in two ways: 1. Filters such as Alternative::HTML expect the keywords in English so if it was output in other language, the result is broken. 2. When $LANG and diff-charset are differ, at least that keyword will be broken, and often break whole output of `svnlook diff`. I think this will only be resolved to set it to "C". Show quoted text

>> Other complex control is not required. I think this is simple and >> clear.

> >Yes, I think we're on the same page here. Please let me know if I'm >wrong.

Yes we're nearly the same, except only for `svnlook diff` part. Show quoted text

>> I don't know these changes are really necessary, but I will test the >> properties. Can you please tell me how to specify the properties?

> >I think you want >svnnotify -p "$1" -r "$2" --charset iso-2022-jp \ >--svn-charset eucJP --diff-charset shift-jis --language ja_JP \ >--with-diff

Ah, I see. I had been misunderstood that I should specify something another options, thank you. I will do testing next week (sorry not to have the environment to test at home), and let you know the result ASAP. Thanks, and good night! Toshikazu

Sat Feb 23 19:05:00 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Sat, 23 Feb 2008 16:04:45 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 23, 2008, at 09:19, kin via RT wrote: Show quoted text

> I'm really glad to see we're on the same wavelength on this issue.

I am, too. Show quoted text

> Okay, my understanding agrees exactly what you says.

Excellent. Show quoted text

>> Thanks, this seems to be almost good. But as for `svnlook diff`, >> $LANG

> needs still set to "C". Why it should be is commented shortly in > source > (see below in detail).

D'oh! That's what I get for not reading comments in the code. Show quoted text

>> `svnlook diff` does not seem to pay attention to $LANG

> > Urm, that is true for diff contents themselves, but other output is > effected. For example, "Modified:" keyword shown in `svnlook diff` > output will be localized according to $LANG. In Japanese it is shown > as "変更:". > > This meddlesomeness of svnlook is harmful in two ways: > 1. Filters such as Alternative::HTML expect the keywords in English so > if it was output in other language, the result is broken. > 2. When $LANG and diff-charset are differ, at least that keyword will > be broken, and often break whole output of `svnlook diff`. > > I think this will only be resolved to set it to "C".

Yes, you are quite right. There was even a comment in the code to this effect. I've now reverted to setting $LANG to "C" in r3460. And with that, I think we've got everything covered. Now if only I could get my ancient FreeBSD box to support all my own UTF-8 multibyte characters, I'd be in business! Show quoted text

>> I think you want >> svnnotify -p "$1" -r "$2" --charset iso-2022-jp \ >> --svn-charset eucJP --diff-charset shift-jis --language ja_JP \ >> --with-diff

> > Ah, I see. I had been misunderstood that I should specify > something another options, thank you. > I will do testing next week (sorry not to have the environment > to test at home), and let you know the result ASAP.

That would be greatly appreciated. I've been trying to get this release out for a week, but I really need confirmation from you that it all works as it should, as my BSD box, as I said, isn't helping me out any. :-( Many thanks for your help. Best, David

Mon Feb 25 05:50:18 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Mon, 25 Feb 2008 19:38:09 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I have tested r3460. post-commit stops in $notifier->output_metadata($out) with following error: | Date: 2008-02-14 11:20:45 +0900 (?━ 14 2? 2008) | Wide character in syswrite at /usr/lib/perl5/5.8.5/Net/Cmd.pm line 436. The commit Date output is localized (so broken), but this is not the reason for the error. As I mentioned in the past mail, we can't pass Perl's internal expression directly to sendmail even if the encoding is specified by binmode, but current code does that again. What I sent: --------------------------------------------------------- 3. To decode them to output encoding (specified by --charset option) by ourselves just before printing them to mail pipe. 4. Mail pipe simply receives octet stream. Item #3 in above listing stands on that specifying encoding to tied_fh of SMTP by binmode has no effect. This is the case of "When Unicode Does Not Happen" in perlunicode doc. --------------------------------------------------------- As a result, I think it is imperative that we have to encode internal expression (UTF-8) to output encoding at least for mail contents. Frankly speaking, I feel r3442 is very close to goal as for encoding. How about re-start at that point? If your decision is no, I will still give my cooperation for testing and debugging of course. But maybe my response will get slow because the code understanding seems to take a time little. Show quoted text

> That would be greatly appreciated. I've been trying to get this > release out for a week, but I really need confirmation from you that > it all works as it should, as my BSD box, as I said, isn't helping me > out any. :-(

I understand it is really difficult that this kind of matter to be confirmed because you don't have environment. Best, Toshikazu

Mon Feb 25 13:10:27 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Mon, 25 Feb 2008 10:10:05 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 25, 2008, at 02:50, kin via RT wrote: Show quoted text

> I have tested r3460.

Great! Show quoted text

> post-commit stops in $notifier->output_metadata($out) with > following error: > > | Date: 2008-02-14 11:20:45 +0900 (?━ 14 2? 2008) > | Wide character in syswrite at /usr/lib/perl5/5.8.5/Net/Cmd.pm line > 436. > > The commit Date output is localized (so broken), but this is > not the reason for the error.

Grrrr. Show quoted text

> As I mentioned in the past mail, we can't pass Perl's internal > expression directly to sendmail even if the encoding is specified > by binmode, but current code does that again. > > What I sent: > --------------------------------------------------------- > 3. To decode them to output encoding (specified by --charset option) > by ourselves > just before printing them to mail pipe. > 4. Mail pipe simply receives octet stream.

That's exactly what binmode is supposed to automate, and does in the tests I've written for it. Show quoted text

> Item #3 in above listing stands on that specifying encoding to > tied_fh of SMTP by > binmode has no effect. This is the case of "When Unicode Does Not > Happen" in > perlunicode doc. > --------------------------------------------------------- > As a result, I think it is imperative that we have to encode internal > expression (UTF-8) to output encoding at least for mail contents.

Oh, okay, so you're using --smtp rather than --sendmail? That would explain it; I can work with that. And looking up to that error, I see that the warning comes from Net::Cmd, which would confirm the use of SMTP. That explains a lot! Show quoted text

> Frankly speaking, I feel r3442 is very close to goal as for encoding. > How about re-start at that point?

I have a better idea: I'll fix SVN::Notify::SMTP so that it properly converts Perl's internal representation to octets…Done! Would you please try r3464? Show quoted text

> If your decision is no, I will still give my cooperation for testing > and > debugging of course. But maybe my response will get slow because > the code understanding seems to take a time little.

I really appreciate the help you've given me with this. I want it to work as well as possible (not US English bias please!), and your assistance has been invaluable. Your patience with me is much appreciated. Show quoted text

> I understand it is really difficult that this kind of matter to be > confirmed > because you don't have environment.

My Mac OS X box can do all the major locales, but I don't have my SVN server on it. :-( That means I can't really test production commits, but I certainly can write lots of Perl tests until I get it right. So in that vein, would you mind executing some commands against some revision of your SVN repository that I could add to tests? Such tests would be publicly distributed with SVN::Notify, so they should not contain anything you don't want to be public. Any simple change will do with a few wide characters as you normally encode them in your commit message and source code. I'm assuming that you use eucJP for your commit messages and file names, and shift-jis for your source code. If not, please adjust accordingly and let me know what the proper encodings are. Substitute "/path/to/svnroot" for your svnroot and "-r1234" with an appropriate revision number. LANG=ja_JP.eucJP svnlook info /path/to/svnroot -r1234 > info.txt LANG=ja_JP.eucJP svnlook changed /path/to/svnroot -r1234 > changed.txt LANG=ja_JP.eucJP svnlook dirs-changed /path/to/svnroot -r1234 > dirs- changed.txt LANG=ja_JP.shift-jis svnlook diff /path/to/svnroot -r1234 > diff.txt If you could then send me those *.diff files, I'll create test files for them in t/data and write a test that compares all of the encodings as appropriate, using sendmail *and* SMTP. Then we can make sure that this issue is licked once and for all. Many thanks, David

Tue Feb 26 01:05:22 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Mon, 25 Feb 2008 22:04:30 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 25, 2008, at 10:10, David E. Wheeler wrote: Show quoted text

>> Frankly speaking, I feel r3442 is very close to goal as for encoding. >> How about re-start at that point?

> > I have a better idea: I'll fix SVN::Notify::SMTP so that it properly > converts Perl's internal representation to octets…Done! Would you > please try r3464?

FYI: As of r3476, I've renamed the options from *charset to *encoding. So you'll need to do something like this now: svnnotify -p "$1" -r "$2" --encoding iso-2022-jp \ --svn-encoding eucJP --diff-encoding shift-jis --language ja_JP \ --with-diff Thanks! David

Tue Feb 26 07:18:19 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Tue, 26 Feb 2008 21:09:38 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, Show quoted text

> Oh, okay, so you're using --smtp rather than --sendmail? That would > explain it; I can work with that. And looking up to that error, I see > that the warning comes from Net::Cmd, which would confirm the use of > SMTP. That explains a lot!

Yes, I'm using --smtp and sorry for that I should have tell you earlier. Show quoted text

> > Frankly speaking, I feel r3442 is very close to goal as for encoding. > > How about re-start at that point?

> > I have a better idea: I'll fix SVN::Notify::SMTP so that it properly > converts Perl's internal representation to octets…Done! Would you > please try r3464?

I have checked and got the same result. I'm afraid Perl's IO layer does nothing because it is the case "glob (aka the <*>)" in "When Unicode Does Not Happen", in perlunicode doc. # http://perldoc.perl.org/perlunicode.html#When-Unicode-Does-Not-Happen So we should encode it by ourselves with Encode::encode(), which has been eliminated in recent changes. Show quoted text

> I really appreciate the help you've given me with this. I want it to > work as well as possible (not US English bias please!), and your > assistance has been invaluable. Your patience with me is much > appreciated.

I also appreciate your patience with me ! :-) Show quoted text

> My Mac OS X box can do all the major locales, but I don't have my SVN > server on it. :-( That means I can't really test production commits, > but I certainly can write lots of Perl tests until I get it right.

I see. Show quoted text

> If you could then send me those *.diff files, I'll create test files > for them in t/data and write a test that compares all of the encodings > as appropriate, using sendmail *and* SMTP. Then we can make sure that > this issue is licked once and for all.

That's a pretty good idea. I will make some test commits and send the files. Please wait a while because I will try make effective data for test and make ... say, ready-to-expose data. BTW, current code contains a little bug on debug print: -sub _dbpnt { print __PACKAGE__, ": $_[0]\n" } +sub _dbpnt { print __PACKAGE__, ": $_[1]\n" } Best, Toshikazu

Tue Feb 26 12:56:58 2008 david [...] justatheory.com - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Tue, 26 Feb 2008 09:43:31 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <david [...] justatheory.com>

On Feb 26, 2008, at 04:18, kin via RT wrote: Show quoted text

> I have checked and got the same result.

Gaaahh! Show quoted text

> I'm afraid Perl's IO layer does nothing because it is the case > "glob (aka the <*>)" in "When Unicode Does Not Happen", in > perlunicode doc. > # http://perldoc.perl.org/perlunicode.html#When-Unicode-Does-Not- > Happen

I don't understand at all. Where is the code using a glob? SMTP is a tied file handle at line 2279, just like the PIPE tied file handles elsewhere. Adding the binmode() call to it at line 2282 should have fixed the issue. There is no globbing there that I can see: it's just a file handle. Show quoted text

> So we should encode it by ourselves with Encode::encode(), which has > been eliminated in recent changes.

I can added it to the PRINT and PRINTF methods. That's probably what's needed. In fact, looking at it, that's probably the problem: although SMTP is binmode'd, I guess it doesn't pass the encoded strings to PRINT. So it has to be done manually. So…it's fixed. For reals! In r3479. I know that it's really fixed because a test failed in t/ smtp.t, and all I had to do was let the test use octets in its comparison, rather than Perl's internal format. W00t! Please do give it a try to confirm, just for a sanity check. Show quoted text

> That's a pretty good idea. I will make some test commits and send the > files. Please wait a while because I will try make effective data for > test and make ... say, ready-to-expose data.

Perfect, thanks. Show quoted text

> BTW, current code contains a little bug on debug print: > -sub _dbpnt { print __PACKAGE__, ": $_[0]\n" } > +sub _dbpnt { print __PACKAGE__, ": $_[1]\n" }

Fixed, thanks. David

Wed Feb 27 04:23:50 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 27 Feb 2008 18:14:52 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I have tested r3486 and got good results for the files those names are single byte. But the test case for those having multibyte character in file path (I personally hate it but real is differ) when HTML or HTML::ColorDiff for Alternative are specified (below) was NG. --handler Alternative \ --alternative HTML \ or --handler Alternative \ --alternative HTML::ColorDiff \ It stops with following error: Wide character in crypt at /usr/lib/perl5/site_perl/5.8.5/SVN/Notify/Alternative.pm line 131. I didn't investigate further because I don't have enough time today. Thanks, Toshikazu

Wed Feb 27 15:21:57 2008 dwheeler [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Wed, 27 Feb 2008 12:21:13 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <dwheeler [...] cpan.org>

On Feb 27, 2008, at 01:23, kin via RT wrote: Show quoted text

> I have tested r3486 and got good results for the files those names > are single byte. But the test case for those having multibyte > character in file path (I personally hate it but real is differ) > when HTML > or HTML::ColorDiff for Alternative are specified (below) was NG. > > --handler Alternative \ > --alternative HTML \ > or > --handler Alternative \ > --alternative HTML::ColorDiff \ > > It stops with following error: > Wide character in crypt at /usr/lib/perl5/site_perl/5.8.5/SVN/Notify/ > Alternative.pm line 131. > > I didn't investigate further because I don't have enough time today.

Now we're getting somewhere! Fixed in r3487. David

Thu Feb 28 15:32:41 2008 dwheeler [...] cpan.org - Correspondence added

On Wed Feb 27 15:21:57 2008, DWHEELER wrote: Show quoted text

> Now we're getting somewhere! Fixed in r3487.

Can I take it from your silence that it now works for you? Have you had a chance to try it? Thanks, David

Fri Feb 29 04:36:28 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Fri, 29 Feb 2008 18:28:14 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I'm sorry for the delay of reply. Show quoted text

> > Now we're getting somewhere! Fixed in r3487.

> > Can I take it from your silence that it now works for you? Have you had > a chance to try it?

I was busy so I couldn't test and reply. Now I checked r3487 and confirmed it works all fine. I think we can release though I have not sent some test data. I hope they can be sent next week. Thanks, Toshikazu

Fri Feb 29 12:52:23 2008 david [...] justatheory.com - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Fri, 29 Feb 2008 09:52:02 -0800
To:	bug-SVN-Notify [...] rt.cpan.org
From:	"David E. Wheeler" <david [...] justatheory.com>

On Feb 29, 2008, at 01:36, kin via RT wrote: Show quoted text

> I was busy so I couldn't test and reply. > > Now I checked r3487 and confirmed it works all fine. I think we can > release > though I have not sent some test data. I hope they can be sent next > week.

W00t! Released! http://justatheory.com/computers/programming/perl/modules/svnnotify-2.70.html Thanks, and I look forward to getting those files and adding more tests. Best, David

Fri Mar 07 04:56:08 2008 kaneyuki [...] eva.hi-ho.ne.jp - Correspondence added

Subject:	Re: [rt.cpan.org #29730] (feature request) --svnlook-diff option
Date:	Fri, 07 Mar 2008 18:47:18 +0900
To:	bug-SVN-Notify [...] rt.cpan.org
From:	Toshikazu Kinkoh <kaneyuki [...] eva.hi-ho.ne.jp>

Hi David, I have created test data. Show quoted text

> commit message and source code. I'm assuming that you use eucJP for > your commit messages and file names, and shift-jis for your source > code. If not, please adjust accordingly and let me know what the > proper encodings are.

The commit messages and file names are UTF-8 in repository because I'm using svn with default settings. Source code is shift-jis. Show quoted text

> LANG=ja_JP.eucJP svnlook info /path/to/svnroot -r1234 > info.txt > LANG=ja_JP.eucJP svnlook changed /path/to/svnroot -r1234 > changed.txt > LANG=ja_JP.eucJP svnlook dirs-changed /path/to/svnroot -r1234 > dirs- > changed.txt > LANG=ja_JP.shift-jis svnlook diff /path/to/svnroot -r1234 > diff.txt

Here is the executed commands to generate the files attached: LANG=ja_JP.eucJP svnlook info /path/to/repos -r 30 > info.txt LANG=ja_JP.eucJP svnlook changed /path/to/repos -r 30 > changed.txt LANG=ja_JP.eucJP svnlook dirs-changed /path/to/repos -r 30 > dirs-changed.txt LANG=C svnlook diff /path/to/repos -r 31 > diff.txt I divided commit for diff (r31) and other commands (r30) because it would help test to focus one feature. r30 contains file path with multibyte character. r31 is diff output with shift-jis, and executed with LANG=C because ja_JP.shift-jis is not supported as I said. Please let me know if you have any inconvenience. Many thanks, Toshikazu

Download changed.txt
application/octet-stream 42b

Message body not shown because it is not plain text.

Download diff.txt
application/octet-stream 511b

Message body not shown because it is not plain text.

Download dirs-changed.txt
application/octet-stream 20b

Message body not shown because it is not plain text.

Download info.txt
application/octet-stream 438b

Message body not shown because it is not plain text.

Tue Aug 13 10:04:18 2013 dwheeler [...] cpan.org - Status changed from 'open' to 'stalled'