Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Junio C Hamano]

Ryan Anderson <rda@google.com> writes:

> Signed-off-by: Ryan Anderson <rda@google.com>
>
> ---
>
> 64ea8c0210c2e9d1711a870460eca326778a4ffc
>  t/t9001-send-email.sh |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
>  create mode 100755 t/t9001-send-email.sh

Adds test, alright, but I do not see the fix.  Is this a thinko?

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

>> 64ea8c0210c2e9d1711a870460eca326778a4ffc
>>  t/t9001-send-email.sh |   34 ++++++++++++++++++++++++++++++++++
>>  1 files changed, 34 insertions(+), 0 deletions(-)
>>  create mode 100755 t/t9001-send-email.sh
>
> Adds test, alright, but I do not see the fix.  Is this a thinko?

On top of yours, I think this covers the CC: trouble your test
triggers.

-- >8 -
send-email: fix cc address fed to underlying sendmail

---
diff --git a/git-send-email.perl b/git-send-email.perl
index d418d6c..d61ef8e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -448,9 +448,11 @@ foreach my $t (@files) {
 					else {
 						$author_not_sender = $2;
 					}
-					printf("(mbox) Adding cc: %s from line '%s'\n",
-						$2, $_) unless $quiet;
-					push @cc, $2;
+					my $cc = extract_valid_address($2);
+					printf("(mbox) Adding cc: %s from ".
+					       "line '%s'\n",
+						$cc, $_) unless $quiet;
+					push @cc, $cc;
 				}
 
 			} else {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 276cbac..a61da1e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -13,10 +13,14 @@ test_expect_success \
 
 test_expect_success \
     'Setup helper tool' \
-    'echo "#!/bin/sh" > fake.sendmail
-     echo "shift" >> fake.sendmail
-     echo "echo \"\$*\" > commandline" >> fake.sendmail
-     echo "cat > msgtxt" >> fake.sendmail
+    '(echo "#!/bin/sh"
+      echo shift
+      echo for a
+      echo do
+      echo "  echo \"!\$a!\""
+      echo "done >commandline"
+      echo "cat > msgtxt"
+      ) >fake.sendmail
      chmod +x ./fake.sendmail
      git add fake.sendmail
      GIT_AUTHOR_NAME="A" git commit -a -m "Second."'
@@ -26,9 +30,12 @@ test_expect_success \
     'git format-patch -n HEAD^1
      git send-email -from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" ./0001*txt'
 
+cat >expected <<\EOF
+!nobody@example.com!
+!author@example.com!
+EOF
 test_expect_success \
     'Verify commandline' \
-    'cline=$(cat commandline)
-     [ "$cline" == "nobody@example.com author@example.com" ]'
+    'diff commandline expected'
 
 test_done

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> On top of yours, I think this covers the CC: trouble your test
> triggers.

Sorry, I did not look closely enough.  You are trying to keep
the address human friendly as long as possible so that you can
place them on the headers, so the previous one was bogus.

*BLUSH*

I think this is lower impact.  On the other hand, it appears
that at least whatever pretends to be /usr/lib/sendmail on my
box seems to grok 'A <author@example.com>' just fine, so maybe
the test was bogus (in which case you should just change the
expected command line parameters to include the human name).

I dunno.

-- >8 --
From c95682409346f7acc220ac64f453933d5a59ec3f Mon Sep 17 00:00:00 2001
From: Junio C Hamano <junkio@cox.net>
Date: Mon, 29 May 2006 23:53:13 -0700
Subject: [PATCH] send-email: do not pass bogus address to local sendmail binary

This makes t9001 test happy.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 git-send-email.perl   |    4 +++-
 t/t9001-send-email.sh |   19 +++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index d418d6c..ac84553 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -387,7 +387,9 @@ X-Mailer: git-send-email $gitversion
 		my $pid = open my $sm, '|-';
 		defined $pid or die $!;
 		if (!$pid) {
-			exec($smtp_server,'-i',@recipients) or die $!;
+			exec($smtp_server,'-i',
+			     map { extract_valid_address($_) }
+			     @recipients) or die $!;
 		}
 		print $sm "$header\n$message";
 		close $sm or die $?;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 276cbac..a61da1e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -13,10 +13,14 @@ test_expect_success \
 
 test_expect_success \
     'Setup helper tool' \
-    'echo "#!/bin/sh" > fake.sendmail
-     echo "shift" >> fake.sendmail
-     echo "echo \"\$*\" > commandline" >> fake.sendmail
-     echo "cat > msgtxt" >> fake.sendmail
+    '(echo "#!/bin/sh"
+      echo shift
+      echo for a
+      echo do
+      echo "  echo \"!\$a!\""
+      echo "done >commandline"
+      echo "cat > msgtxt"
+      ) >fake.sendmail
      chmod +x ./fake.sendmail
      git add fake.sendmail
      GIT_AUTHOR_NAME="A" git commit -a -m "Second."'
@@ -26,9 +30,12 @@ test_expect_success \
     'git format-patch -n HEAD^1
      git send-email -from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" ./0001*txt'
 
+cat >expected <<\EOF
+!nobody@example.com!
+!author@example.com!
+EOF
 test_expect_success \
     'Verify commandline' \
-    'cline=$(cat commandline)
-     [ "$cline" == "nobody@example.com author@example.com" ]'
+    'diff commandline expected'
 
 test_done
-- 
1.3.3.g5029f

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -387,7 +387,9 @@ X-Mailer: git-send-email $gitversion
>                 my $pid = open my $sm, '|-';
>                 defined $pid or die $!;
>                 if (!$pid) {
> -                       exec($smtp_server,'-i',@recipients) or die $!;

This construction (perl pipe+fork) will not work on ActiveState Perl
(it does not even parse the construct).
Last time the problem arised it was suggested to replace readers
with "qx{command}". Regretfully there were no writer case back
then. I'd suggest using IPC::Open2 for portability. Like this:

  use IPC::Open2;
  my $fw;
  my $pid = open2(">&1", $fw, "perl", "-w");
  print $fw "exit 0\n";
  close($fw);'

But I wont. It was never portable in windows, no matter how hard
I tried. The best result was getting output from "cat -v", but "cat"
froze afterwards anyway, as "wc" or "perl" did. Besides, it the
command often freezes that poor imitation of xterm windows has.

There is also 2-argument "open" with damn careful quoting,
if anyone cares (dont think anyone does, for windows).
How about disabling the test on cygwin? (patch attached).

[-- Attachment #2: 0001-disable-send-email-test-for-cygwin.txt --]
[-- Type: text/plain, Size: 731 bytes --]

---
 t/t9001-send-email.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index a61da1e..c3a3737 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -25,6 +25,11 @@ test_expect_success \
      git add fake.sendmail
      GIT_AUTHOR_NAME="A" git commit -a -m "Second."'
 
+if test "$(uname -o)" = Cygwin; then
+    say "git-send-mail tests disabled on Windows"
+    # because of windows being such a crap
+else
+
 test_expect_success \
     'Extract patches and send' \
     'git format-patch -n HEAD^1
@@ -38,4 +43,6 @@ test_expect_success \
     'Verify commandline' \
     'diff commandline expected'
 
+fi
+
 test_done
-- 
1.3.3.g7994

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Christopher Faylor]

On Tue, May 30, 2006 at 03:23:30PM +0200, Alex Riesen wrote:
>>--- a/git-send-email.perl
>>+++ b/git-send-email.perl
>>@@ -387,7 +387,9 @@ X-Mailer: git-send-email $gitversion
>>                my $pid = open my $sm, '|-';
>>                defined $pid or die $!;
>>                if (!$pid) {
>>-                       exec($smtp_server,'-i',@recipients) or die $!;
>
>This construction (perl pipe+fork) will not work on ActiveState Perl
>(it does not even parse the construct).
>Last time the problem arised it was suggested to replace readers
>with "qx{command}". Regretfully there were no writer case back
>then. I'd suggest using IPC::Open2 for portability. Like this:
>
> use IPC::Open2;
> my $fw;
> my $pid = open2(">&1", $fw, "perl", "-w");
> print $fw "exit 0\n";
> close($fw);'
>
>But I wont. It was never portable in windows, no matter how hard
>I tried. The best result was getting output from "cat -v", but "cat"
>froze afterwards anyway, as "wc" or "perl" did. Besides, it the
>command often freezes that poor imitation of xterm windows has.

I assume that "the poor imitation of xterm" is referring to cygwin's
xterm here.  It's really too bad that you can't get into the mindset of
reporting problems to the cygwin mailing list when you notice them.

I can't comment on the proposed patch since, AFAIK, using cat, wc, and
(cygwin's) perl should all work just fine but I don't think it is ever
correct to complain about a platform in released software.

cgf

>---
> t/t9001-send-email.sh |    7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
>diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>index a61da1e..c3a3737 100755
>--- a/t/t9001-send-email.sh
>+++ b/t/t9001-send-email.sh
>@@ -25,6 +25,11 @@ test_expect_success \
>      git add fake.sendmail
>      GIT_AUTHOR_NAME="A" git commit -a -m "Second."'
> 
>+if test "$(uname -o)" = Cygwin; then
>+    say "git-send-mail tests disabled on Windows"
>+    # because of windows being such a crap
>+else
>+

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Alex Riesen]

On 5/30/06, Christopher Faylor <me@cgf.cx> wrote:
> >froze afterwards anyway, as "wc" or "perl" did. Besides, it the
> >command often freezes that poor imitation of xterm windows has.
>
> I assume that "the poor imitation of xterm" is referring to cygwin's
> xterm here.  It's really too bad that you can't get into the mindset of
> reporting problems to the cygwin mailing list when you notice them.

Actually, I was referring to windows console. And no, I don't think you
could do something about it.

But honestly, I don't think it's worth supporting windows in general
and cygwin in particular. And before anyone (again) asks why am
_I_ doing it: I'd have to do my job with Perforce otherwise (as if
windows wasn't bad enough...)

> I can't comment on the proposed patch since, AFAIK, using cat, wc, and
> (cygwin's) perl should all work just fine but I don't think it is ever
> correct to complain about a platform in released software.

If you actually read the message, you'd probably notice ActiveState Perl.

I have no idea why have you taken my post as an attempt to insult cygwin;
IF I had that in mind I'd dedicate a whole long post just to that.

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Christopher Faylor]

On Tue, May 30, 2006 at 06:00:20PM +0200, Alex Riesen wrote:
>On 5/30/06, Christopher Faylor <me@cgf.cx> wrote:
>>>froze afterwards anyway, as "wc" or "perl" did. Besides, it the
>>>command often freezes that poor imitation of xterm windows has.
>>
>>I assume that "the poor imitation of xterm" is referring to cygwin's
>>xterm here.  It's really too bad that you can't get into the mindset of
>>reporting problems to the cygwin mailing list when you notice them.
>
>Actually, I was referring to windows console. And no, I don't think you
>could do something about it.
>
>But honestly, I don't think it's worth supporting windows in general
>and cygwin in particular. And before anyone (again) asks why am
>_I_ doing it: I'd have to do my job with Perforce otherwise (as if
>windows wasn't bad enough...)

I think you've made your opinion on this issue pretty clear.

>>I can't comment on the proposed patch since, AFAIK, using cat, wc, and
>>(cygwin's) perl should all work just fine but I don't think it is ever
>>correct to complain about a platform in released software.
>
>If you actually read the message, you'd probably notice ActiveState Perl.

I actually did read the message.  Do you really want to employ this type
of hackneyed usenet technique here?

Referring to the Windows console as "xterm" (if that is what you were
actually doing) means that your bug report and patch were unclear.

You were also talking about "cat" and "wc" hanging.  Neither is a
Windows command.  Since you've previously complained that you had
to use Cygwin, I think it is safe to assume that you were talking
about Cygwin commands.

>I have no idea why have you taken my post as an attempt to insult cygwin;

There you go.  Kick it up a notch.  I asked you (and have been asking
you) to report problems with Cygwin in the Cygwin forums.  I don't think
that someone mentioning that software has bugs is an "insult".  You
obviously aren't insulting git by fixing bugs that you find.

>IF I had that in mind I'd dedicate a whole long post just to that.

I could write a really long essay about inappropriate mailing list
hostility, too.  Neither would be on-topic here, of course.

Anyway, I think that comments should be factual and not contain negative
opinions about Windows, Solaris, or whatever.

cgf

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Ryan Anderson]

On Tue, May 30, 2006 at 06:00:20PM +0200, Alex Riesen wrote:
> If you actually read the message, you'd probably notice ActiveState Perl.
> 
> I have no idea why have you taken my post as an attempt to insult cygwin;
> IF I had that in mind I'd dedicate a whole long post just to that.

FWIW, it was probably this:
	if test "$(uname -o)"= Cygwin; then

(I only mention becuase I was about to apply this, then I saw that line,
and now I'm confused, is this a fix for ActiveState, or Cygwin?)

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On 5/30/06, Ryan Anderson <ryan@michonline.com> wrote:
> On Tue, May 30, 2006 at 06:00:20PM +0200, Alex Riesen wrote:
> > If you actually read the message, you'd probably notice ActiveState Perl.
> >
> > I have no idea why have you taken my post as an attempt to insult cygwin;
> > IF I had that in mind I'd dedicate a whole long post just to that.
>
> FWIW, it was probably this:
>         if test "$(uname -o)"= Cygwin; then
>
> (I only mention becuase I was about to apply this, then I saw that line,
> and now I'm confused, is this a fix for ActiveState, or Cygwin?)
>

Right. My bad. Should be "$(perl -e 'print $^O')" = MSWin32. That ($^O)
is actually how it is checked in git-annotate.perl (open_pipe).

Christopher, my apologies if that was that. I actually am hostile to
Windows and everything around it, and have my reasons for this.
Still, it does not justify the way how I did that patch.

[-- Attachment #2: send-mail-test.patch --]
[-- Type: text/x-patch, Size: 679 bytes --]

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index a61da1e..7afc358 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -25,6 +25,11 @@ test_expect_success \
      git add fake.sendmail
      GIT_AUTHOR_NAME="A" git commit -a -m "Second."'
 
+if test "$(perl -e 'print $^O')" = MSWin32; then
+    say "git-send-mail tests disabled on ActiveState Perl + Windows"
+    # because of windows being such a crap
+else
+
 test_expect_success \
     'Extract patches and send' \
     'git format-patch -n HEAD^1
@@ -38,4 +43,6 @@ test_expect_success \
     'Verify commandline' \
     'diff commandline expected'
 
+fi
+
 test_done

Re: [PATCH 4/4] Add a basic test case for git send-email, and fix some real bugs discovered.

[Ryan Anderson]

On Mon, May 29, 2006 at 10:57:44PM -0700, Junio C Hamano wrote:
> Ryan Anderson <rda@google.com> writes:
> 
> > Signed-off-by: Ryan Anderson <rda@google.com>
> >
> > ---
> >
> > 64ea8c0210c2e9d1711a870460eca326778a4ffc
> >  t/t9001-send-email.sh |   34 ++++++++++++++++++++++++++++++++++
> >  1 files changed, 34 insertions(+), 0 deletions(-)
> >  create mode 100755 t/t9001-send-email.sh
> 
> Adds test, alright, but I do not see the fix.  Is this a thinko?

I apparently screwed this patch up (and I think I lost it, in the
process.)

Let me reconstruct, I fixed the problems in a different way (I reworked
unique_email_address(@) into  unique_email_address($@), to pass a flag
stating whether to returned the cleaned email address or not, that
should come in a few minutes.)