Re: [PATCH 1/3] gitweb: whitespace cleanup

Re: [PATCH 1/3] gitweb: whitespace cleanup

[Jakub Narebski]

Jakub Narebski <jnareb@gmail.com> wrote:
>
> Signed-off-by: Jakub Narebski <jnareb.com>

It should be of course

 Signed-off-by: Jakub Narebski <jnareb@gmail.com>


Somehow cover letter got lost, and patches did not appear on mailing
list archives.
If the mail will not appear in few hours, I'll resend it to mailing list only.

Probably because git-send-email should ensure LC_ALL=C for Date:
header generation...
-- 
Jakub Narebski
ShadeHawk on #git

Re: [PATCH 0/3] Minor gitweb modifications and cleanups

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> This series of patches is based on git.git 'next' branch
> 69d830d1a3a1236036bd0f84dd9794d7c8d34b3f
>
> My future gitweb-related batches will be based on this series.
>
> ---
>
>  gitweb/gitweb.cgi |   53 +++++++++++++++++++++++++++++++----------------------
>  gitweb/gitweb.css |    4 ++--
>  2 files changed, 33 insertions(+), 24 deletions(-)
>
> -- 
> Jakub Narebski
> Poland

All three applied cleanly (finally!), but with hand munging of
Date: field.

I think git-send-email should internally do LC_ALL=C or stop
using strftime() or both.

[PATCH] send-email: Use setlocale in addition to $ENV{LC_ALL} to set locale

[Jakub Narebski]

$ENV{LC_ALL} = 'C'; does not change locale used by strftime.
Use setlocale( LC_ALL, 'C' ); instead.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>

---

 git-send-email.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

7c796152570e28d3f95c17e93864c6abc8edef24
diff --git a/git-send-email.perl b/git-send-email.perl
index 7b1cca7..56949dd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -24,7 +24,8 @@ use Data::Dumper;
 
 # most mail servers generate the Date: header, but not all...
 $ENV{LC_ALL} = 'C';
-use POSIX qw/strftime/;
+use POSIX qw/strftime setlocale LC_ALL/;
+setlocale( &LC_ALL, 'C' );
 
 my $have_email_valid = eval { require Email::Valid; 1 };
 my $smtp;
-- 
1.3.0

Re: [PATCH] send-email: Use setlocale in addition to $ENV{LC_ALL} to set locale

[Jakub Narebski]

Jakub Narebski wrote:

> $ENV{LC_ALL} = 'C'; does not change locale used by strftime.
> Use setlocale( LC_ALL, 'C' ); instead.

>  # most mail servers generate the Date: header, but not all...
>  $ENV{LC_ALL} = 'C';
> -use POSIX qw/strftime/;
> +use POSIX qw/strftime setlocale LC_ALL/;
> +setlocale( &LC_ALL, 'C' );

Perhaps instead of 
  setlocale( &LC_ALL, 'C' );
we should use
  setlocale( &LC_ALL, '' );
(i.e. set the LC_ALL behaviour according to the locale environment
variables). I'm not that versed in locale, POSIX and Perl.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] send-email: Use setlocale in addition to $ENV{LC_ALL} to set locale

[Eric Wong]

Jakub Narebski <jnareb@gmail.com> wrote:
> Jakub Narebski wrote:
> 
> > $ENV{LC_ALL} = 'C'; does not change locale used by strftime.
> > Use setlocale( LC_ALL, 'C' ); instead.
> 
> >  # most mail servers generate the Date: header, but not all...
> >  $ENV{LC_ALL} = 'C';
> > -use POSIX qw/strftime/;
> > +use POSIX qw/strftime setlocale LC_ALL/;
> > +setlocale( &LC_ALL, 'C' );
> 
> Perhaps instead of 
>   setlocale( &LC_ALL, 'C' );
> we should use
>   setlocale( &LC_ALL, '' );
> (i.e. set the LC_ALL behaviour according to the locale environment
> variables). I'm not that versed in locale, POSIX and Perl.

I'm responsible for the $ENV{LC_ALL} = 'C' setting but I never actually
tested how things would work with a non-English locale (not being
well-versed in these things myself, either).

I've always wondered about why /usr/bin/822-date existed without
strftime on my Debian systems but never bothered asking, I guess this
could be a good reason why...

For reference, here's the /usr/bin/822-date script
(trailing whitespace fixed):

------- 8< -------

#!/usr/bin/perl --
# I hereby place this in the public domain - Ian Jackson, 1995.
# Changes by Klee Dienes also placed in public domain (1997).

# time structure:
# [ sec min hour mday mon year wday yday isdst ]

@ARGV && die "usage: 822-date\n";

$curtime = time;
@localtm = localtime ($curtime);
$localtms = localtime ($curtime);
@gmttm = gmtime ($curtime);
$gmttms = gmtime ($curtime);

if ($localtm[0] != $gmttm[0]) {
    die (sprintf ("local timezone differs from GMT by a non-minute interval\n"
		 . "local time: %s\n"
		 . "GMT time: %s\n", $localtms, $gmttms));
}

$localmin = $localtm[1] + $localtm[2] * 60;
$gmtmin = $gmttm[1] + $gmttm[2] * 60;

if ((($gmttm[6] + 1) % 7) == $localtm[6]) {
    $localmin += 1440;
} elsif ((($gmttm[6] - 1) % 7) == $localtm[6]) {
    $localmin -= 1440;
} elsif ($gmttm[6] == $localtm[6]) {
    1;
} else {
    die ("822-date: local time offset greater than or equal to 24 hours\n");
}

$offset = $localmin - $gmtmin;
$offhour = $offset / 60;
$offmin = abs ($offset % 60);

if (abs ($offhour) >= 24) {
    die ("822-date: local time offset greater than or equal to 24 hours\n");
}

printf
    (
     "%s, %2d %s %d %02d:%02d:%02d %s%02d%02d\n",
     (Sun,Mon,Tue,Wed,Thu,Fri,Sat)[$localtm[6]], # day of week
     $localtm[3],		# day of month
     (Jan,Feb,Mar,Apr,May,Jun,Jul,Aug,Sep,Oct,Nov,Dec)[$localtm[4]], # month
     $localtm[5]+1900,		# year
     $localtm[2],		# hour
     $localtm[1],		# minute
     $localtm[0],		# sec
     ($offset >= 0) ? '+' : '-',# TZ offset direction
     abs ($offhour),		# TZ offset hour
     $offmin,			# TZ offset minute
     ) || die "822-date: output error: $!\n";

------- 8< -------

-- 
Eric Wong

Re: [PATCH] send-email: Use setlocale in addition to $ENV{LC_ALL} to set locale

[Jakub Narebski]

Eric Wong wrote:

> Jakub Narebski <jnareb@gmail.com> wrote:
>> 
>>> $ENV{LC_ALL} = 'C'; does not change locale used by strftime.
>>> Use setlocale( &LC_ALL, 'C' ); instead.

> I'm responsible for the $ENV{LC_ALL} = 'C' setting but I never actually
> tested how things would work with a non-English locale (not being
> well-versed in these things myself, either).

It looks like I have broken locale badly somewhat, as I yesterday
sent patches successfully without the patch. I suspect that sendmail
removes incorrect Date: header and adds it's own... unless Date:
header contains characters outside US-ASCII.

It means that sendmail can deal with
  Date: wto, 20 cze 2006 13:14:11 +0200
instead of correct
  Date: Tue, 20 Jun 2006 13:14:11 +0200

but cannot deal with (iso-8859-2 encoded I think)
  Date: ¶ro, 21 cze 2006 09:48:02 +0200
from git-send-email.perl without patch, instead of correct
  Date: Wed, 21 Jun 2006 11:25:52 +0200
with the patch.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] send-email: Use setlocale in addition to $ENV{LC_ALL} to set locale

[Junio C Hamano]

I was reviewing old log and noticed this topic has never been
resolved.  Your proposal was to use POSIX::setlocale(), and
Eric's counter-proposal was to mimic what 822-date script does,
doing it by hand without mucking with locales, and the
discussion seemed to have died there.  Does this still need to
be addressed?

My gut feeling is that it would probably be less problematic if
we do not muck with locales at all (so drop POSIX::strftime as
well).

So maybe something like this (totally untested)?

-- >8 --
[PATCH] do not use locale specific strftime when preparing 2822 date

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff --git a/git-send-email.perl b/git-send-email.perl
index c5d9e73..bc42761 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -23,8 +23,43 @@ use Getopt::Long;
 use Data::Dumper;
 
 # most mail servers generate the Date: header, but not all...
-$ENV{LC_ALL} = 'C';
-use POSIX qw/strftime/;
+sub format_2822_time {
+	my ($time) = @_;
+	my @localtm = localtime($time);
+	my @gmttm = gmtime($time);
+	my $localmin = $localtm[1] + $localtm[2] * 60;
+	my $gmtmin = $gmttm[1] + $gmttm[2] * 60;
+	if ($localtm[0] != $gmttm[0]) {
+		die "local zone differs from GMT by a non-minute interval\n";
+	}
+	if ((($gmttm[6] + 1) % 7) == $localtm[6]) {
+		$localmin += 1440;
+	} elsif ((($gmttm[6] - 1) % 7) == $localtm[6]) {
+		$localmin -= 1440;
+	} elsif ($gmttm[6] != $localtm[6]) {
+		die "local time offset greater than or equal to 24 hours\n";
+	}
+	my $offset = $localmin - $gmtmin;
+	my $offhour = $offset / 60;
+	my $offmin = abs($offset % 60);
+	if (abs($offhour) >= 24) {
+		die ("local time offset greater than or equal to 24 hours\n");
+	}
+
+	return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
+		       qw(Sun Mon Tue Wed Thu Fri Sat)[$localtm[6]],
+		       $localtm[3],
+		       qw(Jan Feb Mar Apr May Jun
+			  Jul Aug Sep Oct Nov Dec)[$localtm[4]],
+		       $localtm[5]+1900,
+		       $localtm[2],
+		       $localtm[1],
+		       $localtm[0],
+		       ($offset >= 0) ? '+' : '-',
+		       abs($offhour),
+		       $offmin,
+		       );
+}
 
 my $have_email_valid = eval { require Email::Valid; 1 };
 my $smtp;
@@ -371,7 +405,7 @@ sub send_message
 	my @recipients = unique_email_list(@to);
 	my $to = join (",\n\t", @recipients);
 	@recipients = unique_email_list(@recipients,@cc,@bcclist);
-	my $date = strftime('%a, %d %b %Y %H:%M:%S %z', localtime($time++));
+	my $date = format_2822_time($time++);
 	my $gitversion = '@@GIT_VERSION@@';
 	if ($gitversion =~ m/..GIT_VERSION../) {
 	    $gitversion = `git --version`;