git send-email improvements

git send-email improvements

[Pierre Habouzit]

The teaser
==========

This series has been sent using:
  git send-email --to git@vger.kernel.org --compose --annotate HEAD~3..


The series
==========

Here is a patch series to improve git send-email following our
discussions at GitTogether'08, despite my hate for perl.

The first patch is a minor nitpick, because leaking fd's sucks.

The second patch allow git-send-email to receive revision lists as
arguments. This doesn't allow complex arguments combinations as it
proces the revision lists one by one (IOW ^$sha1 $sha2 won't work as
expected _at all_) but this shouldn't be a problem since this command is
primarily used for interactive users. People wanting to use
git-send-email with complex revision lists through scripts MUST
git-format-patch first into a safe temporary directory and use
git-send-email on this afterwards.

The last patch adds the possibility to review patches into an editor
before sending them, which allow you (thanks to patch 2) to serialize,
review, annotate, and send patches in one command.


Further discussion
==================

I think one could make git send-email better doing this:

(1) make --compose and --annotate default, do not asking for a Subject
    if it's missing, neither should we ask for the in-reply-to if it's
    missing.

    Then spawn the editor with a first empty file that contains rougly a
    template looking like this:

        ----8<----
        GIT: Purge this buffer from any content if you don't want a series summary
        GIT:
        GIT: Lines beginning in "GIT: " will be removed.
        GIT: Consider including an overall diffstat or table of contents
        GIT: for the patch you are writing.
        GIT:
        GIT: Please fill a Subject if missing
        GIT: Leave the In-Reply-To field empty if not applicable.
        Subject:
        In-Reply-To:
        --> <we may want to add some more headers here: To/Cc/Bcc/...>

        GIT: put the content of the mail below this line


        GIT: [PATCH 1/10] ....  \
        GIT: [PATCH 2/10] ....   | this would contain all the Subject's
        [...]                    | from the commits that are beeing sent
        GIT: [PATCH 8/10] ....   | as a conveniency for people not
        GIT: [PATCH 9/10] ....   | having to cut&paste them
        GIT: [PATCH 10/10] .... /
        ---->8----

    I suggest we don't enable --compose when the series is reduced to
    one patch, as the usual way is to comment inline. This is probably
    arguable.

(2) Introduce a --batch option that basically:
    * turns --compose and --annotate off
    * turns any interactive feature off
    * complain (and fail) if it misses any information that is usually
      asked interactively

What do you think ?

[PATCH 1/3] git send-email: avoid leaking directory file descriptors.

[Pierre Habouzit]

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index bdbfac6..94ca5c8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -374,10 +374,9 @@ for my $f (@ARGV) {
 
 		push @files, grep { -f $_ } map { +$f . "/" . $_ }
 				sort readdir(DH);
-
+		closedir(DH);
 	} elsif (-f $f or -p $f) {
 		push @files, $f;
-
 	} else {
 		print STDERR "Skipping $f - not found.\n";
 	}
-- 
1.6.0.3.759.g40a2.dirty

[PATCH 2/3] git send-email: interpret unknown files as revision lists

[Pierre Habouzit]

Instead of skipping unkown files on the command line, pass them through
git format-patch into a safe temporary directory. This allow no
complicated rev-list option lists combining "--all" "--not" and so on, but
allow to use ranges which are quite enough for most of the use cases.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |    2 +-
 git-send-email.perl              |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 82f5056..cafff1a 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -8,7 +8,7 @@ git-send-email - Send a collection of patches as emails
 
 SYNOPSIS
 --------
-'git send-email' [options] <file|directory> [... file|directory]
+'git send-email' [options] <file|directory|rev-list>...
 
 
 DESCRIPTION
diff --git a/git-send-email.perl b/git-send-email.perl
index 94ca5c8..0d50ee2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -22,6 +22,7 @@ use Term::ReadLine;
 use Getopt::Long;
 use Data::Dumper;
 use Term::ANSIColor;
+use File::Temp qw/ tempdir /;
 use Git;
 
 package FakeTerm;
@@ -38,7 +39,7 @@ package main;
 
 sub usage {
 	print <<EOT;
-git send-email [options] <file | directory>...
+git send-email [options] <file | directory | rev-list >
 
   Composing:
     --from                  <str>  * Email From:
@@ -378,7 +379,8 @@ for my $f (@ARGV) {
 	} elsif (-f $f or -p $f) {
 		push @files, $f;
 	} else {
-		print STDERR "Skipping $f - not found.\n";
+		my $tempdir = tempdir(CLEANUP => 1);
+		push @files, $repo->command('format-patch', '-o', $tempdir, $f);
 	}
 }
 
-- 
1.6.0.3.759.g40a2.dirty

[PATCH 3/3] git send-email: add --annotate option

[Pierre Habouzit]

This allows to review every patch (and fix various aspects of them, or
comment them) in an editor just before being sent. Combined to the fact
that git send-email can now process revision lists, this makes git
send-email and efficient way to review and send patches interactively.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |   11 +++++++++++
 git-send-email.perl              |   26 ++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index cafff1a..9ee81d5 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -37,6 +37,11 @@ The --bcc option must be repeated for each user you want on the bcc list.
 +
 The --cc option must be repeated for each user you want on the cc list.
 
+--annotate::
+	Review each patch you're about to send in an editor. The setting
+	'sendemail.multiedit' defines if this will spawn one editor per patch
+	or one for all of them at once.
+
 --compose::
 	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
 	introductory message for the patch series.
@@ -204,6 +209,12 @@ sendemail.aliasfiletype::
 	Format of the file(s) specified in sendemail.aliasesfile. Must be
 	one of 'mutt', 'mailrc', 'pine', or 'gnus'.
 
+sendemail.multiedit::
+	If true (default), a single editor instance will be spawned to edit
+	files you have to edit (patches when '--annotate' is used, and the
+	summary when '--compose' is used). If false, files will be edited one
+	after the other, spawning a new editor each time.
+
 
 Author
 ------
diff --git a/git-send-email.perl b/git-send-email.perl
index 0d50ee2..65c254d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -48,6 +48,7 @@ git send-email [options] <file | directory | rev-list >
     --bcc                   <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
+    --annotate                     * Review each patch that will be sent in an editor.
     --compose                      * Open an editor for introduction.
 
   Sending:
@@ -130,7 +131,8 @@ my $compose_filename = ".msg.$$";
 
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
-	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
+	$initial_reply_to,$initial_subject,@files,
+	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
 
 my $envelope_sender;
 
@@ -151,6 +153,17 @@ if ($@) {
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
 
+# Handle interactive edition of files.
+my $multiedit;
+my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+sub do_edit {
+	if (defined($multiedit) && !$multiedit) {
+		map { system('sh', '-c', $editor.' "$@"', $editor, $_); } @_;
+	} else {
+		system('sh', '-c', $editor.' "$@"', $editor, @_);
+	}
+}
+
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
@@ -180,6 +193,7 @@ my %config_settings = (
     "aliasesfile" => \@alias_files,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
+    "multiedit" => \$multiedit,
 );
 
 # Handle Uncouth Termination
@@ -222,6 +236,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
+		    "annotate" => \$annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
@@ -499,7 +514,12 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', $editor.' "$@"', $editor, $compose_filename);
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
@@ -548,6 +568,8 @@ EOT
 	}
 
 	@files = ($compose_filename . ".final", @files);
+} elsif ($annotate) {
+	do_edit(@files);
 }
 
 # Variables we set as part of the loop over files
-- 
1.6.0.3.759.g40a2.dirty

Further enhancement proposal for git-send-email

[Pierre Habouzit]

Here is a three patch series (again).

[PATCH 1/3] git send-email: make the message file name more specific.
  -> quite independant, and should IMHO be taken.


[PATCH 2/3] git send-email: do not ask questions when --compose is used.
[PATCH 3/3] git send-email: turn --compose on when more than one patch.

  Those two patches enhance git-send-email by making ask less questions
  when --compose is used (as it can grab the subject, from and reply-to
  from the buffer).

  It also turns --compose on by default as soon as there is more than
  one patch, as I believe than commenting a patch series is more often
  done than not. It's is really trivial to "refuse" to comment the
  series by just erasing the full buffer content, which should not
  really be too anoying (or one can explicitely pass --no-compose for
  the same result).

  It's probable that those two changes may trigger some discussion
  though, but I just used that to send this series, and I can tell with
  this git-send-email is nearer what I would like it to be.

[PATCH 1/3] git send-email: make the message file name more specific.

[Pierre Habouzit]

This helps editors choosing their syntax hilighting properly.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 65c254d..4ca571f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -127,7 +127,7 @@ sub unique_email_list(@);
 sub cleanup_compose_files();
 
 # Constants (essentially)
-my $compose_filename = ".msg.$$";
+my $compose_filename = ".gitsendemail.msg.$$";
 
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
-- 
1.6.0.3.763.g0275.dirty

[PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Pierre Habouzit]

When --compose is used, we can grab the From/Subject/In-Reply-To from the
edited summary, let it be so and don't ask the user silly questions.

The summary templates gets quite revamped, and includes the list of
patches subjects that are going to be sent with this batch.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |  174 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 102 insertions(+), 72 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 4ca571f..5c189a7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -417,6 +417,105 @@ if (@files) {
 	usage();
 }
 
+sub get_patch_subject($) {
+	my $fn = shift;
+	open (my $fh, '<', $fn);
+	while (my $line = <$fh>) {
+		next unless ($line =~ /^Subject: (.*)$/);
+		close $fh;
+		return "GIT: $1\n";
+	}
+	close $fh;
+	die "No subject line in $fn ?";
+}
+
+if ($compose) {
+	# Note that this does not need to be secure, but we will make a small
+	# effort to have it be unique
+	open(C,">",$compose_filename)
+		or die "Failed to open for writing $compose_filename: $!";
+
+
+	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
+	my $tpl_subject = $initial_subject || '';
+	my $tpl_reply_to = $initial_reply_to || '';
+
+	print C <<EOT;
+From $tpl_sender # This line is ignored.
+GIT: Lines beginning in "GIT: " will be removed.
+GIT: Consider including an overall diffstat or table of contents
+GIT: for the patch you are writing.
+From: $tpl_sender
+Subject: $tpl_subject
+In-Reply-To: $tpl_reply_to
+
+GIT: Please enter your email below this line.
+
+EOT
+	for my $f (@files) {
+		print C get_patch_subject($f);
+	}
+	close(C);
+
+	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
+
+	open(C2,">",$compose_filename . ".final")
+		or die "Failed to open $compose_filename.final : " . $!;
+
+	open(C,"<",$compose_filename)
+		or die "Failed to open $compose_filename : " . $!;
+
+	my $need_8bit_cte = file_has_nonascii($compose_filename);
+	my $in_body = 0;
+	my $summary_empty = 1;
+	while(<C>) {
+		next if m/^GIT: /;
+		if ($in_body) {
+		} elsif (/^\n$/) {
+			$in_body = 1;
+			if ($need_8bit_cte) {
+				print C2 "MIME-Version: 1.0\n",
+					 "Content-Type: text/plain; ",
+					   "charset=utf-8\n",
+					 "Content-Transfer-Encoding: 8bit\n";
+			}
+		} elsif (/^MIME-Version:/i) {
+			$need_8bit_cte = 0;
+		} elsif (/^Subject:\s*(.+)\s*$/i) {
+			$initial_subject = $1;
+			my $subject = $initial_subject;
+			$_ = "Subject: " .
+				($subject =~ /[^[:ascii:]]/ ?
+				 quote_rfc2047($subject) :
+				 $subject) .
+				"\n";
+		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
+			$initial_reply_to = $1;
+			next;
+		} elsif (/^From:\s*(.+)\s*$/i) {
+			$sender = $1;
+			next;
+		}
+		$summary_empty = 0;
+		print C2 $_;
+	}
+	close(C);
+	close(C2);
+
+	if ($summary_empty) {
+		print "Summary email is empty, skpping it\n";
+		$compose = -1;
+	}
+} elsif ($annotate) {
+	do_edit(@files);
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
@@ -461,17 +560,6 @@ sub expand_aliases {
 @initial_cc = expand_aliases(@initial_cc);
 @bcclist = expand_aliases(@bcclist);
 
-if (!defined $initial_subject && $compose) {
-	while (1) {
-		$_ = $term->readline("What subject should the initial email start with? ", $initial_subject);
-		last if defined $_;
-		print "\n";
-	}
-
-	$initial_subject = $_;
-	$prompting++;
-}
-
 if ($thread && !defined $initial_reply_to && $prompting) {
 	while (1) {
 		$_= $term->readline("Message-ID to be used as In-Reply-To for the first email? ", $initial_reply_to);
@@ -498,64 +586,6 @@ if (!defined $smtp_server) {
 }
 
 if ($compose) {
-	# Note that this does not need to be secure, but we will make a small
-	# effort to have it be unique
-	open(C,">",$compose_filename)
-		or die "Failed to open for writing $compose_filename: $!";
-	print C "From $sender # This line is ignored.\n";
-	printf C "Subject: %s\n\n", $initial_subject;
-	printf C <<EOT;
-GIT: Please enter your email below.
-GIT: Lines beginning in "GIT: " will be removed.
-GIT: Consider including an overall diffstat or table of contents
-GIT: for the patch you are writing.
-
-EOT
-	close(C);
-
-	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-
-	if ($annotate) {
-		do_edit($compose_filename, @files);
-	} else {
-		do_edit($compose_filename);
-	}
-
-	open(C2,">",$compose_filename . ".final")
-		or die "Failed to open $compose_filename.final : " . $!;
-
-	open(C,"<",$compose_filename)
-		or die "Failed to open $compose_filename : " . $!;
-
-	my $need_8bit_cte = file_has_nonascii($compose_filename);
-	my $in_body = 0;
-	while(<C>) {
-		next if m/^GIT: /;
-		if (!$in_body && /^\n$/) {
-			$in_body = 1;
-			if ($need_8bit_cte) {
-				print C2 "MIME-Version: 1.0\n",
-					 "Content-Type: text/plain; ",
-					   "charset=utf-8\n",
-					 "Content-Transfer-Encoding: 8bit\n";
-			}
-		}
-		if (!$in_body && /^MIME-Version:/i) {
-			$need_8bit_cte = 0;
-		}
-		if (!$in_body && /^Subject: ?(.*)/i) {
-			my $subject = $1;
-			$_ = "Subject: " .
-				($subject =~ /[^[:ascii:]]/ ?
-				 quote_rfc2047($subject) :
-				 $subject) .
-				"\n";
-		}
-		print C2 $_;
-	}
-	close(C);
-	close(C2);
-
 	while (1) {
 		$_ = $term->readline("Send this email? (y|n) ");
 		last if defined $_;
@@ -567,9 +597,9 @@ EOT
 		exit(0);
 	}
 
-	@files = ($compose_filename . ".final", @files);
-} elsif ($annotate) {
-	do_edit(@files);
+	if ($compose > 0) {
+		@files = ($compose_filename . ".final", @files);
+	}
 }
 
 # Variables we set as part of the loop over files
-- 
1.6.0.3.763.g0275.dirty

[PATCH 3/3] git send-email: turn --compose on when more than one patch.

[Pierre Habouzit]

Automatically turn --compose on when there is more than one patch, and
that the output is a tty.

Do not print the list of files sent anymore in that case, as the list is
shown in the summary editor.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5c189a7..5cebb40 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -49,7 +49,7 @@ git send-email [options] <file | directory | rev-list >
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
     --annotate                     * Review each patch that will be sent in an editor.
-    --compose                      * Open an editor for introduction.
+    --[no-]compose                 * Open an editor for introduction.
 
   Sending:
     --envelope-sender       <str>  * Email envelope sender.
@@ -237,7 +237,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
-		    "compose" => \$compose,
+		    "compose!" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "suppress-from!" => \$suppress_from,
@@ -409,7 +409,11 @@ if ($validate) {
 }
 
 if (@files) {
-	unless ($quiet) {
+	if (!defined($compose) && -t STDOUT) {
+		# turn $compose on if there is more than one file
+		$compose = $#files;
+	}
+	unless ($quiet || $compose) {
 		print $_,"\n" for (@files);
 	}
 } else {
-- 
1.6.0.3.763.g0275.dirty

[PATCH] git send-email: allow any rev-list option as an argument.

[Pierre Habouzit]

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  One can consider to squash that on top of
  <1225450632-7230-3-git-send-email-madcoder@debian.org> to be able to pass
  all non path arguments before a possible '--' to git format-patch.

  The downside of this patch is that:

    git send-email -C -C -M origin/next

  will send the content of origin/next if it's an existing file. Of course a
  disambiguation can be:

    git send-email -C -C -M refs/heads/origin/next

  But again if this file also exists, one is basically screwed. I see no
  proper way to fix that, unless to change git-send-email behaviour at once.


  Though I believe this semantics to be better than the one in the previous
  patch, as it's often a good idea to pass -M -C -C to format-patch, which is
  currently impossible. It also allow revision lists to work as expected (wrt
  --all, --not and so on).

  Comments are welcomed.


 Documentation/git-send-email.txt |    2 +-
 git-send-email.perl              |   19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 9ee81d5..39d6da9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -8,7 +8,7 @@ git-send-email - Send a collection of patches as emails
 
 SYNOPSIS
 --------
-'git send-email' [options] <file|directory|rev-list>...
+'git send-email' [options] <file|directory|rev-list options>...
 
 
 DESCRIPTION
diff --git a/git-send-email.perl b/git-send-email.perl
index 5c189a7..8667e0b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -25,6 +25,8 @@ use Term::ANSIColor;
 use File::Temp qw/ tempdir /;
 use Git;
 
+Getopt::Long::Configure qw/ pass_through /;
+
 package FakeTerm;
 sub new {
 	my ($class, $reason) = @_;
@@ -39,7 +41,7 @@ package main;
 
 sub usage {
 	print <<EOT;
-git send-email [options] <file | directory | rev-list >
+git send-email [options] <file | directory | rev-list options >
 
   Composing:
     --from                  <str>  * Email From:
@@ -383,8 +385,12 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 
 # Now that all the defaults are set, process the rest of the command line
 # arguments and collect up the files that need to be processed.
-for my $f (@ARGV) {
-	if (-d $f) {
+my @rev_list_opts;
+while (my $f = pop @ARGV) {
+	if ($f eq "--") {
+		push @rev_list_opts, "--", @ARGV;
+		@ARGV = ();
+	} elsif (-d $f) {
 		opendir(DH,$f)
 			or die "Failed to opendir $f: $!";
 
@@ -394,11 +400,14 @@ for my $f (@ARGV) {
 	} elsif (-f $f or -p $f) {
 		push @files, $f;
 	} else {
-		my $tempdir = tempdir(CLEANUP => 1);
-		push @files, $repo->command('format-patch', '-o', $tempdir, $f);
+		push @rev_list_opts, $f;
 	}
 }
 
+if (@rev_list_opts) {
+	push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts);
+}
+
 if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
-- 
1.6.0.3.791.g15769.dirty

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Ian Hilt]

On Fri, Oct 31, 2008 at 01:36:48PM +0100, Pierre Habouzit wrote:
> +GIT: Please enter your email below this line.

At first glance I thought this meant to enter my email address here.
So, instead of "email" would "message" be better?  Although on second
glance I realized this is where the body of the message went.  Not sure
if this is worth changing.

	Ian

Re: [PATCH 3/3] git send-email: add --annotate option

[Ian Hilt]

On Fri, Oct 31, 2008 at 11:57:12AM +0100, Pierre Habouzit wrote:
> This allows to review every patch (and fix various aspects of them, or
> comment them) in an editor just before being sent. Combined to the fact
> that git send-email can now process revision lists, this makes git
> send-email and efficient way to review and send patches interactively.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  Documentation/git-send-email.txt |   11 +++++++++++
>  git-send-email.perl              |   26 ++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index cafff1a..9ee81d5 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -37,6 +37,11 @@ The --bcc option must be repeated for each user you want on the bcc list.
>  +
>  The --cc option must be repeated for each user you want on the cc list.
>  
> +--annotate::
> +	Review each patch you're about to send in an editor. The setting
> +	'sendemail.multiedit' defines if this will spawn one editor per patch
> +	or one for all of them at once.
> +
>  --compose::
>  	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
>  	introductory message for the patch series.
> @@ -204,6 +209,12 @@ sendemail.aliasfiletype::
>  	Format of the file(s) specified in sendemail.aliasesfile. Must be
>  	one of 'mutt', 'mailrc', 'pine', or 'gnus'.
>  
> +sendemail.multiedit::
> +	If true (default), a single editor instance will be spawned to edit
> +	files you have to edit (patches when '--annotate' is used, and the
> +	summary when '--compose' is used). If false, files will be edited one
> +	after the other, spawning a new editor each time.
> +
>  
>  Author
>  ------
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 0d50ee2..65c254d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -48,6 +48,7 @@ git send-email [options] <file | directory | rev-list >
>      --bcc                   <str>  * Email Bcc:
>      --subject               <str>  * Email "Subject:"
>      --in-reply-to           <str>  * Email "In-Reply-To:"
> +    --annotate                     * Review each patch that will be sent in an editor.
>      --compose                      * Open an editor for introduction.
>  
>    Sending:
> @@ -130,7 +131,8 @@ my $compose_filename = ".msg.$$";
>  
>  # Variables we fill in automatically, or via prompting:
>  my (@to,@cc,@initial_cc,@bcclist,@xh,
> -	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
> +	$initial_reply_to,$initial_subject,@files,
> +	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
>  
>  my $envelope_sender;
>  
> @@ -151,6 +153,17 @@ if ($@) {
>  # Behavior modification variables
>  my ($quiet, $dry_run) = (0, 0);
>  
> +# Handle interactive edition of files.

s/edition/editing/;

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Pierre Habouzit]

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

On Fri, Oct 31, 2008 at 09:33:38PM +0000, Ian Hilt wrote:
> On Fri, Oct 31, 2008 at 01:36:48PM +0100, Pierre Habouzit wrote:
> > +GIT: Please enter your email below this line.
> 
> At first glance I thought this meant to enter my email address here.
> So, instead of "email" would "message" be better?  Although on second
> glance I realized this is where the body of the message went.  Not sure
> if this is worth changing.

Well, this line sounds kind of awkward actually, so I was even thinking
about removing it.

Decent editors should probably have a plugin to put the cursor here and
be done with it.


In fact what looks odd is the GIT: stuff. a line looking like:

    --- write your message below this line ---

Looks 10x better, though need some code to strip it out if the user kept
it, and I'm lazy, GIT: stuff is automatically removed...


But if that's the only thing that you don't like in the series, I'm
glad, this is quite a minor issue ;)
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Ian Hilt]

On Fri, Oct 31, 2008 at 10:38:03PM +0100, Pierre Habouzit wrote:
> On Fri, Oct 31, 2008 at 09:33:38PM +0000, Ian Hilt wrote:
> > On Fri, Oct 31, 2008 at 01:36:48PM +0100, Pierre Habouzit wrote:
> > > +GIT: Please enter your email below this line.
> > 
> > At first glance I thought this meant to enter my email address here.
> > So, instead of "email" would "message" be better?  Although on second
> > glance I realized this is where the body of the message went.  Not sure
> > if this is worth changing.
> 
> Well, this line sounds kind of awkward actually, so I was even thinking
> about removing it.
> 
> Decent editors should probably have a plugin to put the cursor here and
> be done with it.
> 
> 
> In fact what looks odd is the GIT: stuff. a line looking like:
> 
>     --- write your message below this line ---
> 
> Looks 10x better, though need some code to strip it out if the user kept
> it, and I'm lazy, GIT: stuff is automatically removed...

Or, to follow the convention of git-status and git-commit, you could do
this with "# ".

So something like,

--->8---
From: Ian Hilt <ihilt@mcgregor-surmount.com>
Date: Fri, 31 Oct 2008 17:55:46 -0400
Subject: [PATCH] Use a hash instead of GIT: for line removal

Signed-off-by: Ian Hilt <ihilt@mcgregor-surmount.com>
---
 git-send-email.perl |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5cebb40..c6e21a8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -427,7 +427,7 @@ sub get_patch_subject($) {
 	while (my $line = <$fh>) {
 		next unless ($line =~ /^Subject: (.*)$/);
 		close $fh;
-		return "GIT: $1\n";
+		return "# $1\n";
 	}
 	close $fh;
 	die "No subject line in $fn ?";
@@ -446,14 +446,14 @@ if ($compose) {
 
 	print C <<EOT;
 From $tpl_sender # This line is ignored.
-GIT: Lines beginning in "GIT: " will be removed.
-GIT: Consider including an overall diffstat or table of contents
-GIT: for the patch you are writing.
+# Lines beginning in "# " will be removed.
+# Consider including an overall diffstat or table of contents
+# for the patch you are writing.
 From: $tpl_sender
 Subject: $tpl_subject
 In-Reply-To: $tpl_reply_to
 
-GIT: Please enter your email below this line.
+# --- write your message below this line ---
 
 EOT
 	for my $f (@files) {
@@ -479,7 +479,7 @@ EOT
 	my $in_body = 0;
 	my $summary_empty = 1;
 	while(<C>) {
-		next if m/^GIT: /;
+		next if m/^# /;
 		if ($in_body) {
 		} elsif (/^\n$/) {
 			$in_body = 1;
--->8---

> But if that's the only thing that you don't like in the series, I'm
> glad, this is quite a minor issue ;)

I've thought something like this would be a good thing.  An editor makes
things easier to fix than the command-line.

	Ian

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Ian Hilt]

On Fri, Oct 31, 2008 at 06:01:49PM -0400, Ian Hilt wrote:
> I've thought something like this would be a good thing.  An editor makes
> things easier to fix than the command-line.

Speaking of which, maybe let's add the To field to the list.

--->8---
From: Ian Hilt <ian.hilt@gmx.com>
Date: Fri, 31 Oct 2008 22:15:46 -0400
Subject: [PATCH] git-send-email.perl: add To field in editor

This allows the compose mode to add the To field in the editor.  However
it currently will not remove an address.

Signed-off-by: Ian Hilt <ian.hilt@gmx.com>
---

This is on top of the previous patch I sent.

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

diff --git a/git-send-email.perl b/git-send-email.perl
index 0944be7..ed95402 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -450,6 +450,12 @@ if ($compose) {
 
 
 	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
+	my @tpl_to;
+	if (@to) {
+		foreach my $i (0..$#to) {
+			$tpl_to[$i] = $to[$i];
+		}
+	}
 	my $tpl_subject = $initial_subject || '';
 	my $tpl_reply_to = $initial_reply_to || '';
 
@@ -459,6 +465,7 @@ From $tpl_sender # This line is ignored.
 # Consider including an overall diffstat or table of contents
 # for the patch you are writing.
 From: $tpl_sender
+To: @tpl_to
 Subject: $tpl_subject
 In-Reply-To: $tpl_reply_to
 
@@ -514,6 +521,9 @@ EOT
 		} elsif (/^From:\s*(.+)\s*$/i) {
 			$sender = $1;
 			next;
+		} elsif (/^To:\s*(.+)\s*$/i) {
+			push @to, $1;
+			next;
 		}
 		$summary_empty = 0;
 		print C2 $_;
@@ -522,7 +532,7 @@ EOT
 	close(C2);
 
 	if ($summary_empty) {
-		print "Summary email is empty, skpping it\n";
+		print "Summary email is empty, skipping it\n";
 		$compose = -1;
 	}
 } elsif ($annotate) {
-- 
1.6.0.3.523.g304d0

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Pierre Habouzit]

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

On Sat, Nov 01, 2008 at 02:26:49AM +0000, Ian Hilt wrote:
> On Fri, Oct 31, 2008 at 06:01:49PM -0400, Ian Hilt wrote:
> > I've thought something like this would be a good thing.  An editor makes
> > things easier to fix than the command-line.
> 
> Speaking of which, maybe let's add the To field to the list.

I didn't do it for a very good reason: the To field is tricker to parse
because very fast it's multiline, and must be split along the ',' when
parsed back and so on.

And even moreuseful than the To is the Cc list that git-send-email
bloats to death and that I would like to reduce very often.

But sadly that needs an expertise of perl I absolutely don't have. We
probably even want to depend on some MIME perl library that knows about
those kind of issues and do it for us well.

But yeah, I knew I left out those, and this was the reason.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Ian Hilt]

On Sat, Nov 01, 2008 at 12:04:39PM +0100, Pierre Habouzit wrote:
> I didn't do it for a very good reason: the To field is tricker to parse
> because very fast it's multiline, and must be split along the ',' when
> parsed back and so on.

Right.  So my patch is broken in that it doesn't parse the addresses
correctly.  This _should_ be easy to fix.  I knew my patch sucked, but I
wanted to get the idea out there.  For me, I don't like specifying all
that information on the command-line.  It would be nice to be able to
edit the To and Cc fields in the editor.

> And even moreuseful than the To is the Cc list that git-send-email
> bloats to death and that I would like to reduce very often.

You mean git-send-email adds too many addresses to the Cc list, or the
code for those fields is already bloated to death?

> But sadly that needs an expertise of perl I absolutely don't have. We
> probably even want to depend on some MIME perl library that knows about
> those kind of issues and do it for us well.

I'm confused here.  Why would a MIME library help?

> But yeah, I knew I left out those, and this was the reason.

Anyway, do you, or does anyone else, think it's even worth coding the
possibility for the user to edit the To and Cc fields?


	Ian

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Pierre Habouzit]

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

On Sat, Nov 01, 2008 at 01:00:33PM +0000, Ian Hilt wrote:
> On Sat, Nov 01, 2008 at 12:04:39PM +0100, Pierre Habouzit wrote:
> > I didn't do it for a very good reason: the To field is tricker to parse
> > because very fast it's multiline, and must be split along the ',' when
> > parsed back and so on.
> 
> Right.  So my patch is broken in that it doesn't parse the addresses
> correctly.  This _should_ be easy to fix.  I knew my patch sucked, but I
> wanted to get the idea out there.  For me, I don't like specifying all
> that information on the command-line.  It would be nice to be able to
> edit the To and Cc fields in the editor.
> 
> > And even moreuseful than the To is the Cc list that git-send-email
> > bloats to death and that I would like to reduce very often.
> 
> You mean git-send-email adds too many addresses to the Cc list, or the
> code for those fields is already bloated to death?

The former, I've not looked at the code I can't really say.

> > But sadly that needs an expertise of perl I absolutely don't have. We
> > probably even want to depend on some MIME perl library that knows about
> > those kind of issues and do it for us well.
> 
> I'm confused here.  Why would a MIME library help?

Hmm maybe I'm wrong, but the idea would be to do what mutt does and be
able to parse:

To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
  Superman <batman@nyc.us>,
  "Someone with a comma, inside its tag name" <a@b.com>

And that needs to know how to do that with perl, and _really_ I hate
perl enough for not being able to do that well. Splitting on ',' is just
not going to fly.

> > But yeah, I knew I left out those, and this was the reason.
> 
> Anyway, do you, or does anyone else, think it's even worth coding the
> possibility for the user to edit the To and Cc fields?

*YES*

I would love to see git-send-email work like mutt does: it fills the
field like it does now, and you are allowed to fix that, and it parses
the buffer back to guess what you wanted. It allow to drop most of the
interactive prompting that is so annoying (since it's not in-shell and
has no history and stuff like that, unlike my $EDITOR).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Francis Galiegue]

Le Saturday 01 November 2008 18:08:17 Pierre Habouzit, vous avez écrit :
[...]
>
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
>   Superman <batman@nyc.us>,
>   "Someone with a comma, inside its tag name" <a@b.com>
>

fg@erwin ~ $ cat t.txt
To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
<a@b.com>
fg@erwin ~ $ <t.txt perl -ne 's,^To:\s*,,i; @mails = m/\s*((?:"[^"]+")?
\s*<[^@]+@[^@]+>)\s*,?/g; END { print join("\n", @mails) . "\n"}'
<some.address@some.tld>
<random.joe@abc.def>
<batman@nyc.us>
"Someone with a comma, inside its tag name" <a@b.com>

That's regex, not especially perl ;)

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Pierre Habouzit]

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

On Sat, Nov 01, 2008 at 05:34:32PM +0000, Francis Galiegue wrote:
> Le Saturday 01 November 2008 18:08:17 Pierre Habouzit, vous avez écrit :
> [...]
> >
> > To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
> >   Superman <batman@nyc.us>,
> >   "Someone with a comma, inside its tag name" <a@b.com>
> >
> 
> fg@erwin ~ $ cat t.txt
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
> Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
> <a@b.com>
> fg@erwin ~ $ <t.txt perl -ne 's,^To:\s*,,i; @mails = m/\s*((?:"[^"]+")?
> \s*<[^@]+@[^@]+>)\s*,?/g; END { print join("\n", @mails) . "\n"}'
> <some.address@some.tld>
> <random.joe@abc.def>
> <batman@nyc.us>
> "Someone with a comma, inside its tag name" <a@b.com>
> 
> That's regex, not especially perl ;)

Your regex fails to parse:

"Someone with a comma, and an escape double quote \" in its name"
  <regex.cant.be.used.for.serious.parsing@i.told.you.so>

That's why I first hinted at some MIME library that has probably made
that right.

Not to mention that you don't fix the multiline issue, but that is quite
less of a problem, it merely needs an accumulator and a 1 line lookahead
in the parser code while in the header parts. Nothing unrealistic for me
to write.

But I just can't resolve myself to parse anything with regex because I
just know it's horribly broken and wrong.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Ian Hilt]

On Sat, 1 Nov 2008, Pierre Habouzit wrote:
> On Sat, Nov 01, 2008 at 01:00:33PM +0000, Ian Hilt wrote:
> > I'm confused here.  Why would a MIME library help?
> 
> Hmm maybe I'm wrong, but the idea would be to do what mutt does and be
> able to parse:
> 
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
>   Superman <batman@nyc.us>,
>   "Someone with a comma, inside its tag name" <a@b.com>

No, you're absolutely right for this reason: MIME-encoded addresses.

Ugh.

Although, to my knowledge, git-send-email doesn't understand that now. 
So unless we want to support MIME-encoded addresses, this won't be
necessary.

> And that needs to know how to do that with perl, and _really_ I hate
> perl enough for not being able to do that well. Splitting on ',' is just
> not going to fly.

Yea, it's going to be a bit trickier than that.

> > > But yeah, I knew I left out those, and this was the reason.
> > 
> > Anyway, do you, or does anyone else, think it's even worth coding the
> > possibility for the user to edit the To and Cc fields?
> 
> *YES*
> 
> I would love to see git-send-email work like mutt does: it fills the
> field like it does now, and you are allowed to fix that, and it parses
> the buffer back to guess what you wanted. It allow to drop most of the
> interactive prompting that is so annoying (since it's not in-shell and
> has no history and stuff like that, unlike my $EDITOR).

Hmm, I'll look into this; especially the MIME library.  I'm not a perl
monk, but I can give it a shot, no?


	Ian

Re: [PATCH 2/3] git send-email: do not ask questions when --compose is used.

[Francis Galiegue]

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

Le Saturday 01 November 2008 18:43:52 Pierre Habouzit, vous avez écrit :
[...]

> Your regex fails to parse:
>
> "Someone with a comma, and an escape double quote \" in its name"

Easy fix: replace "[^"]+" with "[^"]+(?:\\"[^"]*)*".

>   <regex.cant.be.used.for.serious.parsing@i.told.you.so>

Oh yes. Regexes _are_ the way to do serious parsing. All MIME packages you 
will find floating around use regexes to parse mail headers correctly.

Granted, adhering to the RFC822 to the letter is rather hard. But I have a 
sample program here that can not only parse the escaped double quote, but 
also take account for the multiple line stuff and multiple headers of the 
same type where email addresse are valid (To:, Cc:, Bcc:). See attachment. 
Feel free to use the code.

----

fg@erwin ~ $ cat t.txt
To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
<a@b.com>
To: bbr@one2team.com,
 u1@whatever.org,
  u2@wherever.ru,
   u3@blah.com
fg@erwin ~ $ perl t.pl <t.txt
Found mail: John Doe <some.address@some.tld>
Found mail: Random Joe <random.joe@abc.def>
Found mail: Superman <batman@nyc.us>
Found mail: "Someone with a comma, inside its tag name" <a@b.com>
Found mail: bbr@one2team.com
Found mail: u1@whatever.org
Found mail: u2@wherever.ru
Found mail: u3@blah.com
----


-- 
fge

[-- Attachment #2: t.pl --]
[-- Type: application/x-perl, Size: 1101 bytes --]

Re: [PATCH 1/3] git send-email: avoid leaking directory file descriptors.

[Jeff King]

On Fri, Oct 31, 2008 at 11:57:10AM +0100, Pierre Habouzit wrote:

> +		closedir(DH);

Ugh. This is a great reason to use a scoped variable (like "my $dh),
which will close automatically. Once upon a time I think you _had_ to
use globs for this, but I think it has not been the case for some time
(and I think we only support back to perl 5.6 these days). Can any perl
gurus comment?

-Peff

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Jeff King]

On Fri, Oct 31, 2008 at 05:52:05PM +0100, Pierre Habouzit wrote:

> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> 
>   One can consider to squash that on top of
>   <1225450632-7230-3-git-send-email-madcoder@debian.org> to be able to pass
>   all non path arguments before a possible '--' to git format-patch.

Personally, I think the other patch is not useful without this. I often
pull out funny subsets of patches if I know it is safe to do so (e.g., I
collect small, unrelated bugfixes directly onto a single branch, but I
send them separately).

With this patch, I might even find send-email usable. :)

-Peff

Re: [PATCH 1/3] git send-email: make the message file name more specific.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> This helps editors choosing their syntax hilighting properly.

Even though I agree this is the right direction to go, unfortunately this
can break people's existing setup.

Having said that, if we were to do this, let's do it the right way and put
these "temporary" files under $GIT_DIR.

>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  git-send-email.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 65c254d..4ca571f 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -127,7 +127,7 @@ sub unique_email_list(@);
>  sub cleanup_compose_files();
>  
>  # Constants (essentially)
> -my $compose_filename = ".msg.$$";
> +my $compose_filename = ".gitsendemail.msg.$$";
>  
>  # Variables we fill in automatically, or via prompting:
>  my (@to,@cc,@initial_cc,@bcclist,@xh,
> -- 
> 1.6.0.3.763.g0275.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] git send-email: add --annotate option

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> This allows to review every patch (and fix various aspects of them, or
> comment them) in an editor just before being sent. Combined to the fact
> that git send-email can now process revision lists, this makes git
> send-email and efficient way to review and send patches interactively.

Without your patches, you run format-patch (with or without cover), you
use the editor of your choice to massage them and feed the resulting files
to send-email.

Only because you wanted to allow format-patch parameters to be given to
send-email, you now need to also allow the messages to be massaged before
they are sent out.

Is it only me who finds that this series creates its own problem and then
has to solve it?  What are we getting in return?

Re: [PATCH 1/3] git send-email: make the message file name more specific.

[Pierre Habouzit]

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

On Sun, Nov 02, 2008 at 06:18:08AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > This helps editors choosing their syntax hilighting properly.
> 
> Even though I agree this is the right direction to go, unfortunately this
> can break people's existing setup.

Well for now vim (I don't know if emacs has syntax highlight for it)
does:
    autocmd BufNewFile,BufRead .msg.[0-9]*
	  \ if getline(1) =~ '^From.*# This line is ignored.$' |
	  \   setf gitsendemail |
	  \ endif

Even if you're illiterate in vim script language, you should grok what
it does, because of the fact that .msg.nnnn is hardly something one can
recognize. I believe it's highly unlikely to break anything.

What do other people think ?

> Having said that, if we were to do this, let's do it the right way and put
> these "temporary" files under $GIT_DIR.

Agreed, I should have done that.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Pierre Habouzit]

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

On Sun, Nov 02, 2008 at 04:35:23AM +0000, Jeff King wrote:
> On Fri, Oct 31, 2008 at 05:52:05PM +0100, Pierre Habouzit wrote:
> 
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> > 
> >   One can consider to squash that on top of
> >   <1225450632-7230-3-git-send-email-madcoder@debian.org> to be able to pass
> >   all non path arguments before a possible '--' to git format-patch.
> 
> Personally, I think the other patch is not useful without this. I often
> pull out funny subsets of patches if I know it is safe to do so (e.g., I
> collect small, unrelated bugfixes directly onto a single branch, but I
> send them separately).
> 
> With this patch, I might even find send-email usable. :)

Well it still messes the file/reference name conflict with no way to
prevent it because of the backward compatibility, and even if unlikely
it's still possible.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/3] git send-email: add --annotate option

[Pierre Habouzit]

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

On Sun, Nov 02, 2008 at 06:23:55AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > This allows to review every patch (and fix various aspects of them, or
> > comment them) in an editor just before being sent. Combined to the fact
> > that git send-email can now process revision lists, this makes git
> > send-email and efficient way to review and send patches interactively.
> 
> Without your patches, you run format-patch (with or without cover), you
> use the editor of your choice to massage them and feed the resulting files
> to send-email.
> 
> Only because you wanted to allow format-patch parameters to be given to
> send-email, you now need to also allow the messages to be massaged before
> they are sent out.
> 
> Is it only me who finds that this series creates its own problem and then
> has to solve it?  What are we getting in return?

Actually my problem is that the current workflow is:

    $ git format-patch [rev-list]

    $ vim *.patch

    # massage patches

    $ git send-email [argument list too long to copy] --compose *.patch

    # struggle in vim to reopen the patches I'm about to comment to copy
    # the Subject lines and other similar stuff

    # answer to a lot of silly questions that git-s-e should guess from
    # the cover.

*also* I often have other patches in my repository, and this send-email
sometimes globs _too many_ patches and this is a big problem for me.
Basically that and all the '#' bits, and the number of commands to type
are what make me dislike git-send-email (but still use it since there
are no good alternatives yet that automate the task).

With my patch series, the workflow is as follows:

    $ git send-email --to <where> --annotate [rev-list]

    # as vim can open many files at once, I have the cover opened _and_
    # all the patches at once, or only the patch if there's one single
    # patch I can massage everything I want.

    # answer 'y' to the _single_ question git-s-e asks.
    # or 'n' if something doesn't fly.

Not only the command line is considerably shorter (even the --to can be
omited actually, but unlike --in-reply-to, it rarely changes and it's in
the history so...), but more importantly I can see what I will send, no
more '*.patch' that will bite me hard. I don't have to struggle opening
all the patches I'm interested in reading while I comment them in the
cover, and so on.


I mean you're mistaken when you say:
  ] Only because you wanted to allow format-patch parameters to be
  ] given to send-email, you now need to also allow the messages to be
  ] massaged before they are sent out.

Your causality is backwards. I _DO_ want git-send-email to allow me to
do the cover _and_ the massaging at once. It's actually the first patch
I wrote locally even if I reordered the series before sending for some
reason I don't remember. *Then* if you do that, there's little point in
having to perform git-format-patch in the first place, hence I wanted
the feature to let git-format-patch be run by git-send-email directly.

I don't know for others, but with those series, git-send-email is
_REALLY_ what I would have wanted it to be from day 1. The sole little
issues I can see are:
 * the To:/Cc:/Bcc:/other headers parsing directly from the cover, for
   that someone better skilled than me shall add a last patch to do that
   properly.

 * when you only edit one single patch, it doesn't do the From/To/Cc/...
   parsing and you'll get all the silly interactive questions again.
   That should probably addressed, but to be frank I care about this one
   less, because I send single patches directly from mutt. So it's not
   really my itch to scratch[0] ;)


  [0] WHO SAID I'M LAZY ? Yeah you in the back, I HEAR YA!
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Jeff King]

On Sun, Nov 02, 2008 at 10:39:07AM +0100, Pierre Habouzit wrote:

> Well it still messes the file/reference name conflict with no way to
> prevent it because of the backward compatibility, and even if unlikely
> it's still possible.

Hmm. As Junio mentioned, this is really an easier way of doing:

  git format-patch -o tmp "$@"
  $EDITOR tmp/*
  git send-email tmp

So I guess a wrapper program would suffice, that just called send-email.
But of course then you would have to think of a new name, and explain
the confusion between it and send-email.

-Peff

Re: [PATCH 1/3] git send-email: make the message file name more specific.

[Ian Hilt]

On Sun, 2 Nov 2008, Pierre Habouzit wrote:
> On Sun, Nov 02, 2008 at 06:18:08AM +0000, Junio C Hamano wrote:
> > Having said that, if we were to do this, let's do it the right way and put
> > these "temporary" files under $GIT_DIR.
> 
> Agreed, I should have done that.

Perhaps like this:

	my $compose_filename = $repo->repo_path() . "/sendemail.msg.$$";

where $repo is a repository instance.


	Ian

Re: [PATCH 1/3] git send-email: make the message file name more specific.

[Pierre Habouzit]

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

On Sun, Nov 02, 2008 at 09:34:53PM +0000, Ian Hilt wrote:
> On Sun, 2 Nov 2008, Pierre Habouzit wrote:
> > On Sun, Nov 02, 2008 at 06:18:08AM +0000, Junio C Hamano wrote:
> > > Having said that, if we were to do this, let's do it the right way and put
> > > these "temporary" files under $GIT_DIR.
> > 
> > Agreed, I should have done that.
> 
> Perhaps like this:
> 
> 	my $compose_filename = $repo->repo_path() . "/sendemail.msg.$$";
> 
> where $repo is a repository instance.

$repo is a repository instance, I'm waiting for all the comments to fade
up to take them into account and resend a proper series.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Pierre Habouzit]

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

On Sun, Nov 02, 2008 at 06:02:21PM +0000, Jeff King wrote:
> On Sun, Nov 02, 2008 at 10:39:07AM +0100, Pierre Habouzit wrote:
> 
> > Well it still messes the file/reference name conflict with no way to
> > prevent it because of the backward compatibility, and even if unlikely
> > it's still possible.
> 
> Hmm. As Junio mentioned, this is really an easier way of doing:
> 
>   git format-patch -o tmp "$@"
>   $EDITOR tmp/*
>   git send-email tmp
> 
> So I guess a wrapper program would suffice, that just called send-email.
> But of course then you would have to think of a new name, and explain
> the confusion between it and send-email.

Well that defeats the purpose of fixing send-email to me. I really would
like to see this fixed properly like it should. I mean it makes sense to
me to use _three_ commands where one should be enough. Not to mention
that introducing a new command is just completely against the spirit of
*simplifying* the current UI ;)

Actually I see a few possibilities.

(1) The first one is to pass a --[no]-format-patch flag to
    git-send-email which says that it should understand arguments as
    format-patch arguments.  You add to that a sendemail.format-patch
    setting that would default to false for backward compatibility sake,
    that would allow the user to force --format-patch as a default.

    This would e.g. cleanly allow:  git send-email --format-patch -3 HEAD.

    I would understand if people dislike the setting: it basically
    modifies the behaviour of a git command a lot, which has been
    frowned upon in the past. Even though I would argue than using
    git-send-email in scripting is quite bad, for something that you can
    probably replace with:

    while read patchname; do mail some@where.org < $patchname; done < git format-patch "$@"

    But if people think it's too dangerous, replacing it with a short
    switch so that it's not too painful to use would fly for me,
    something like -F or whatever.


(2) Another way is to add a --pass-to-format-patch kind of option that
    would take its arguments and pass it to git-format-patch. Like in:
    git send-email --pass-to-format-patch "-3 HEAD". (Of course a short
    switch would help ;p).

(3) Use -- for mandatory separating <format-patch> arguments like this:

	git send-email [send-email options] -- -3 HEAD

    or if you want to send patches that would modify only a given path:

        git send-email [s-e options] -- origin/next.. -- git-gui

    that would run internally:

        git format-patch origin/next.. -- git-gui


I would say that I dislike (2) a LOT because it's a pain to use: needs a
lot of quoting, and it gets worse if you want to pass things with spaces
in it to format-patch.

(2) has the small drawback of not being 100% backward compatible: with
the current use of perl Getoptions, -- is used to stop options
processing, and people _may_ have used it to do `git s-e -- --my.patch`
and such a use would break. However this is highly unlikely to cause
issues in real life I think (unlike the problem of refs against filename
clashes).

In (1) people may dislike the idea of a setting, I've not strong
feelings about it, I won't mind if it gets rejected, a short switch will
do just fine then.


As a summary, I'd say that I like both (1) and (3) because those are
handy, short, and either completely or mostly backward compatible. My
way would be to go down (1) and add a alias.s-e = !git send-email -F in
my .gitconfig.

What do you think ?

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/3] git send-email: add --annotate option

[Matthieu Moy]

Pierre Habouzit <madcoder@debian.org> writes:

> I don't know for others, but with those series, git-send-email is
> _REALLY_ what I would have wanted it to be from day 1.

Same for me. I didn't really understand why git was asking me to run
two separate commands to send a patch. Having git-send-email do
everything and ask me only the required is really the way I expected
it to be.

(probably not a coincidence that at least bzr and darcs have a "send"
command that does just this)

-- 
Matthieu

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> As a summary, I'd say that I like both (1) and (3) because those are
> handy, short, and either completely or mostly backward compatible. My
> way would be to go down (1) and add a alias.s-e = !git send-email -F in
> my .gitconfig.
>
> What do you think ?

I wonder if we can do this even without an explicit -F.

What command line arguments does send-email take, and what options would
we want to give the underlying format-patch?  Can't you sift them without
ambiguity?

The current syntax is:

    git send-email <flags>... <file|dir>...

I am wondering if we can just extend it to:

    git send-email <flags>... <<file|dir>...|rev>

E.g. we should be able to parse this out:

    git send-email --to git@vger.kernel.org -M --suppress-cc=all origin

and notice "--to git@vger.kernel.org" and "--suppress-cc" are for
send-email, guess "-M" (or anything that is outside the current
send-email's vocabulary) is meant for format-patch, and if there is no
file or directory called "origin" then decide that the user wants to run
format-patch, and act as a front-end as if the user did:

	git format-patch -o tmp.$$ -M origin &&
        ... perhaps do your --annotate and --compose here by launching
        ... the editor...
        git send-email --to git@vger.kernel.org --suppress-cc=all tmp.$$ &&
	rm -fr tmp.$$

If you happen to have a file or a directory called origin, it would be
safer for users if the command errored out asking for disambiguation.  The
user can either say "./origin" or "origin^0" to disambiguate between them.

Hmm?

Re: [PATCH] git send-email: allow any rev-list option as an argument.

[Pierre Habouzit]

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

On Tue, Nov 04, 2008 at 01:04:27AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > As a summary, I'd say that I like both (1) and (3) because those are
> > handy, short, and either completely or mostly backward compatible. My
> > way would be to go down (1) and add a alias.s-e = !git send-email -F in
> > my .gitconfig.
> >
> > What do you think ?
> 
> I wonder if we can do this even without an explicit -F.
> 
> What command line arguments does send-email take, and what options would
> we want to give the underlying format-patch?  Can't you sift them without
> ambiguity?
> 
> The current syntax is:
> 
>     git send-email <flags>... <file|dir>...
> 
> I am wondering if we can just extend it to:
> 
>     git send-email <flags>... <<file|dir>...|rev>
> 
> E.g. we should be able to parse this out:
> 
>     git send-email --to git@vger.kernel.org -M --suppress-cc=all origin
> 
> and notice "--to git@vger.kernel.org" and "--suppress-cc" are for
> send-email, guess "-M" (or anything that is outside the current
> send-email's vocabulary) is meant for format-patch, and if there is no
> file or directory called "origin" then decide that the user wants to run
> format-patch, and act as a front-end as if the user did:
> 
> 	git format-patch -o tmp.$$ -M origin &&
>         ... perhaps do your --annotate and --compose here by launching
>         ... the editor...
>         git send-email --to git@vger.kernel.org --suppress-cc=all tmp.$$ &&
> 	rm -fr tmp.$$
> 
> If you happen to have a file or a directory called origin, it would be
> safer for users if the command errored out asking for disambiguation.  The
> user can either say "./origin" or "origin^0" to disambiguate between them.

Oh right you can disambiguate references using ^0 so maybe my proposal
works after all, though it has to check for each file name if it's not a
reference _also_. I like it. I will rework my patch series now then,
since most of the discussed points of them have been addressed in the
thread.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[take 2] git send-email updates

[Pierre Habouzit]

  [PATCH 1/5] git send-email: make the message file name more specific.
    self described

  [PATCH 2/5] git send-email: interpret unknown files as revision lists
    All unknown arguments are passed to git-format-patch at once,
    checking for possible file/rev conflicts and dying in that case,
    like Junio suggested.

  [PATCH 3/5] git send-email: add --annotate option
    same as before.

  [PATCH 4/5] git send-email: ask less questions when --compose is used.
    same as before, with an update wrt empty bodies. Still doesn't grok
    To/Cc/Bcc. I would be really glad if a patch to deal with it was
    appended to that series, but a patch that deals with Header
    continuations well.

  [PATCH 5/5] git send-email: turn --compose on when more than one patch.
    This patch is probably controversial. I propose it not because I'm
    lazy, I now have a 'git send' alias for the task that expands to
    'send-email -C -C -M -n --annotate --compose --to'. I propose it
    because I believe it's a good thing to make people write about their
    stuff when there is a series and not a single patch. If they still
    don't want to, they just have to clear the mail buffer at once.

    The drawback is that it _may_ break some scripts, those people would
    have to pass --no-compose to their send-email call to fix the
    scripts.

    I wouldn't complain if the patch gets dropped.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[PATCH 1/5] git send-email: make the message file name more specific.

[Pierre Habouzit]

This helps editors choosing their syntax hilighting properly.

Also make the file live under the git directory.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 94ca5c8..aaace02 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -124,9 +124,6 @@ my $auth;
 sub unique_email_list(@);
 sub cleanup_compose_files();
 
-# Constants (essentially)
-my $compose_filename = ".msg.$$";
-
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
 	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
@@ -149,6 +146,7 @@ if ($@) {
 
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
+my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
-- 
1.5.6.5

[PATCH 2/5] git send-email: interpret unknown files as revision lists

[Pierre Habouzit]

Filter out all the arguments git-send-email doesn't like to a
git format-patch command, that dumps its content to a safe directory.

Barf when a file/revision conflict occurs.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |    2 +-
 git-send-email.perl              |   28 ++++++++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 82f5056..4654d4f 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -8,7 +8,7 @@ git-send-email - Send a collection of patches as emails
 
 SYNOPSIS
 --------
-'git send-email' [options] <file|directory> [... file|directory]
+'git send-email' [options] <file|directory|rev-list options>...
 
 
 DESCRIPTION
diff --git a/git-send-email.perl b/git-send-email.perl
index aaace02..c29868a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -22,8 +22,11 @@ use Term::ReadLine;
 use Getopt::Long;
 use Data::Dumper;
 use Term::ANSIColor;
+use File::Temp qw/ tempdir /;
 use Git;
 
+Getopt::Long::Configure qw/ pass_through /;
+
 package FakeTerm;
 sub new {
 	my ($class, $reason) = @_;
@@ -38,7 +41,7 @@ package main;
 
 sub usage {
 	print <<EOT;
-git send-email [options] <file | directory>...
+git send-email [options] <file | directory | rev-list options >
 
   Composing:
     --from                  <str>  * Email From:
@@ -363,10 +366,22 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 
 ($sender) = expand_aliases($sender) if defined $sender;
 
+sub check_file_rev_conflict($) {
+	my $f = shift;
+	if ($repo->command('rev-parse', '--verify', '--quiet', $f)) {
+		die("revision/filename conflict on `$f'");
+	}
+}
+
 # Now that all the defaults are set, process the rest of the command line
 # arguments and collect up the files that need to be processed.
-for my $f (@ARGV) {
-	if (-d $f) {
+my @rev_list_opts;
+while (my $f = pop @ARGV) {
+	if ($f eq "--") {
+		push @rev_list_opts, "--", @ARGV;
+		@ARGV = ();
+	} elsif (-d $f) {
+		check_file_rev_conflict($f);
 		opendir(DH,$f)
 			or die "Failed to opendir $f: $!";
 
@@ -374,12 +389,17 @@ for my $f (@ARGV) {
 				sort readdir(DH);
 		closedir(DH);
 	} elsif (-f $f or -p $f) {
+		check_file_rev_conflict($f);
 		push @files, $f;
 	} else {
-		print STDERR "Skipping $f - not found.\n";
+		push @rev_list_opts, $f;
 	}
 }
 
+if (@rev_list_opts) {
+	push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts);
+}
+
 if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
-- 
1.5.6.5

[PATCH 3/5] git send-email: add --annotate option

[Pierre Habouzit]

This allows to review every patch (and fix various aspects of them, or
comment them) in an editor just before being sent. Combined to the fact
that git send-email can now process revision lists, this makes git
send-email and efficient way to review and send patches interactively.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |   11 +++++++++++
 git-send-email.perl              |   26 ++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 4654d4f..39d6da9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -37,6 +37,11 @@ The --bcc option must be repeated for each user you want on the bcc list.
 +
 The --cc option must be repeated for each user you want on the cc list.
 
+--annotate::
+	Review each patch you're about to send in an editor. The setting
+	'sendemail.multiedit' defines if this will spawn one editor per patch
+	or one for all of them at once.
+
 --compose::
 	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
 	introductory message for the patch series.
@@ -204,6 +209,12 @@ sendemail.aliasfiletype::
 	Format of the file(s) specified in sendemail.aliasesfile. Must be
 	one of 'mutt', 'mailrc', 'pine', or 'gnus'.
 
+sendemail.multiedit::
+	If true (default), a single editor instance will be spawned to edit
+	files you have to edit (patches when '--annotate' is used, and the
+	summary when '--compose' is used). If false, files will be edited one
+	after the other, spawning a new editor each time.
+
 
 Author
 ------
diff --git a/git-send-email.perl b/git-send-email.perl
index c29868a..d0c5a41 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -50,6 +50,7 @@ git send-email [options] <file | directory | rev-list options >
     --bcc                   <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
+    --annotate                     * Review each patch that will be sent in an editor.
     --compose                      * Open an editor for introduction.
 
   Sending:
@@ -129,7 +130,8 @@ sub cleanup_compose_files();
 
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
-	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
+	$initial_reply_to,$initial_subject,@files,
+	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
 
 my $envelope_sender;
 
@@ -151,6 +153,17 @@ if ($@) {
 my ($quiet, $dry_run) = (0, 0);
 my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
+# Handle interactive edition of files.
+my $multiedit;
+my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+sub do_edit {
+	if (defined($multiedit) && !$multiedit) {
+		map { system('sh', '-c', $editor.' "$@"', $editor, $_); } @_;
+	} else {
+		system('sh', '-c', $editor.' "$@"', $editor, @_);
+	}
+}
+
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
@@ -180,6 +193,7 @@ my %config_settings = (
     "aliasesfile" => \@alias_files,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
+    "multiedit" => \$multiedit,
 );
 
 # Handle Uncouth Termination
@@ -222,6 +236,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
+		    "annotate" => \$annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
@@ -515,7 +530,12 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', $editor.' "$@"', $editor, $compose_filename);
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
@@ -564,6 +584,8 @@ EOT
 	}
 
 	@files = ($compose_filename . ".final", @files);
+} elsif ($annotate) {
+	do_edit(@files);
 }
 
 # Variables we set as part of the loop over files
-- 
1.5.6.5

[PATCH 4/5] git send-email: ask less questions when --compose is used.

[Pierre Habouzit]

When --compose is used, we can grab the From/Subject/In-Reply-To from the
edited summary, let it be so and don't ask the user silly questions.

The summary templates gets quite revamped, and includes the list of
patches subjects that are going to be sent with this batch.

When having a body full of empty lines, the summary isn't sent. Document
that in the git-send-email manpage fully.

Note: It doesn't deal with To/Cc/Bcc yet.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |    9 ++
 git-send-email.perl              |  177 ++++++++++++++++++++++---------------
 2 files changed, 114 insertions(+), 72 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 39d6da9..e06db6b 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -45,6 +45,15 @@ The --cc option must be repeated for each user you want on the cc list.
 --compose::
 	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
 	introductory message for the patch series.
++
+When compose is in used, git send-email gets less interactive will use the
+values of the headers you set there. If the body of the email (what you type
+after the headers and a blank line) only contains blank (or GIT: prefixed)
+lines, the summary won't be sent, but git-send-email will still use the
+Headers values if you don't removed them.
++
+If it wasn't able to see a header in the summary it will ask you about it
+interactively after quitting your editor.
 
 --from::
 	Specify the sender of the emails.  This will default to
diff --git a/git-send-email.perl b/git-send-email.perl
index d0c5a41..fd72127 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -433,6 +433,108 @@ if (@files) {
 	usage();
 }
 
+sub get_patch_subject($) {
+	my $fn = shift;
+	open (my $fh, '<', $fn);
+	while (my $line = <$fh>) {
+		next unless ($line =~ /^Subject: (.*)$/);
+		close $fh;
+		return "GIT: $1\n";
+	}
+	close $fh;
+	die "No subject line in $fn ?";
+}
+
+if ($compose) {
+	# Note that this does not need to be secure, but we will make a small
+	# effort to have it be unique
+	open(C,">",$compose_filename)
+		or die "Failed to open for writing $compose_filename: $!";
+
+
+	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
+	my $tpl_subject = $initial_subject || '';
+	my $tpl_reply_to = $initial_reply_to || '';
+
+	print C <<EOT;
+From $tpl_sender # This line is ignored.
+GIT: Lines beginning in "GIT: " will be removed.
+GIT: Consider including an overall diffstat or table of contents
+GIT: for the patch you are writing.
+GIT:
+GIT: Clear the body content if you don't wish to send a summary.
+From: $tpl_sender
+Subject: $tpl_subject
+In-Reply-To: $tpl_reply_to
+
+EOT
+	for my $f (@files) {
+		print C get_patch_subject($f);
+	}
+	close(C);
+
+	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
+
+	open(C2,">",$compose_filename . ".final")
+		or die "Failed to open $compose_filename.final : " . $!;
+
+	open(C,"<",$compose_filename)
+		or die "Failed to open $compose_filename : " . $!;
+
+	my $need_8bit_cte = file_has_nonascii($compose_filename);
+	my $in_body = 0;
+	my $summary_empty = 1;
+	while(<C>) {
+		next if m/^GIT: /;
+		if ($in_body) {
+			$summary_empty = 0 unless (/^\n$/);
+		} elsif (/^\n$/) {
+			$in_body = 1;
+			if ($need_8bit_cte) {
+				print C2 "MIME-Version: 1.0\n",
+					 "Content-Type: text/plain; ",
+					   "charset=utf-8\n",
+					 "Content-Transfer-Encoding: 8bit\n";
+			}
+		} elsif (/^MIME-Version:/i) {
+			$need_8bit_cte = 0;
+		} elsif (/^Subject:\s*(.+)\s*$/i) {
+			$initial_subject = $1;
+			my $subject = $initial_subject;
+			$_ = "Subject: " .
+				($subject =~ /[^[:ascii:]]/ ?
+				 quote_rfc2047($subject) :
+				 $subject) .
+				"\n";
+		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
+			$initial_reply_to = $1;
+			next;
+		} elsif (/^From:\s*(.+)\s*$/i) {
+			$sender = $1;
+			next;
+		} elsif (/^(?:To|Cc|Bcc):/i) {
+			print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
+			next;
+		}
+		print C2 $_;
+	}
+	close(C);
+	close(C2);
+
+	if ($summary_empty) {
+		print "Summary email is empty, skpping it\n";
+		$compose = -1;
+	}
+} elsif ($annotate) {
+	do_edit(@files);
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
@@ -477,17 +579,6 @@ sub expand_aliases {
 @initial_cc = expand_aliases(@initial_cc);
 @bcclist = expand_aliases(@bcclist);
 
-if (!defined $initial_subject && $compose) {
-	while (1) {
-		$_ = $term->readline("What subject should the initial email start with? ", $initial_subject);
-		last if defined $_;
-		print "\n";
-	}
-
-	$initial_subject = $_;
-	$prompting++;
-}
-
 if ($thread && !defined $initial_reply_to && $prompting) {
 	while (1) {
 		$_= $term->readline("Message-ID to be used as In-Reply-To for the first email? ", $initial_reply_to);
@@ -514,64 +605,6 @@ if (!defined $smtp_server) {
 }
 
 if ($compose) {
-	# Note that this does not need to be secure, but we will make a small
-	# effort to have it be unique
-	open(C,">",$compose_filename)
-		or die "Failed to open for writing $compose_filename: $!";
-	print C "From $sender # This line is ignored.\n";
-	printf C "Subject: %s\n\n", $initial_subject;
-	printf C <<EOT;
-GIT: Please enter your email below.
-GIT: Lines beginning in "GIT: " will be removed.
-GIT: Consider including an overall diffstat or table of contents
-GIT: for the patch you are writing.
-
-EOT
-	close(C);
-
-	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-
-	if ($annotate) {
-		do_edit($compose_filename, @files);
-	} else {
-		do_edit($compose_filename);
-	}
-
-	open(C2,">",$compose_filename . ".final")
-		or die "Failed to open $compose_filename.final : " . $!;
-
-	open(C,"<",$compose_filename)
-		or die "Failed to open $compose_filename : " . $!;
-
-	my $need_8bit_cte = file_has_nonascii($compose_filename);
-	my $in_body = 0;
-	while(<C>) {
-		next if m/^GIT: /;
-		if (!$in_body && /^\n$/) {
-			$in_body = 1;
-			if ($need_8bit_cte) {
-				print C2 "MIME-Version: 1.0\n",
-					 "Content-Type: text/plain; ",
-					   "charset=utf-8\n",
-					 "Content-Transfer-Encoding: 8bit\n";
-			}
-		}
-		if (!$in_body && /^MIME-Version:/i) {
-			$need_8bit_cte = 0;
-		}
-		if (!$in_body && /^Subject: ?(.*)/i) {
-			my $subject = $1;
-			$_ = "Subject: " .
-				($subject =~ /[^[:ascii:]]/ ?
-				 quote_rfc2047($subject) :
-				 $subject) .
-				"\n";
-		}
-		print C2 $_;
-	}
-	close(C);
-	close(C2);
-
 	while (1) {
 		$_ = $term->readline("Send this email? (y|n) ");
 		last if defined $_;
@@ -583,9 +616,9 @@ EOT
 		exit(0);
 	}
 
-	@files = ($compose_filename . ".final", @files);
-} elsif ($annotate) {
-	do_edit(@files);
+	if ($compose > 0) {
+		@files = ($compose_filename . ".final", @files);
+	}
 }
 
 # Variables we set as part of the loop over files
-- 
1.5.6.5

[PATCH 5/5] git send-email: turn --compose on when more than one patch.

[Pierre Habouzit]

Automatically turn --compose on when there is more than one patch, and
that the output is a tty.

Do not print the list of files sent anymore in that case, as the list is
shown in the summary editor.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index fd72127..3c7818f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -51,7 +51,7 @@ git send-email [options] <file | directory | rev-list options >
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
     --annotate                     * Review each patch that will be sent in an editor.
-    --compose                      * Open an editor for introduction.
+    --[no-]compose                 * Open an editor for introduction.
 
   Sending:
     --envelope-sender       <str>  * Email envelope sender.
@@ -237,7 +237,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
-		    "compose" => \$compose,
+		    "compose!" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "suppress-from!" => \$suppress_from,
@@ -425,7 +425,11 @@ if ($validate) {
 }
 
 if (@files) {
-	unless ($quiet) {
+	if (!defined($compose) && -t STDOUT) {
+		# turn $compose on if there is more than one file
+		$compose = $#files;
+	}
+	unless ($quiet || $compose) {
 		print $_,"\n" for (@files);
 	}
 } else {
-- 
1.5.6.5

Re: [PATCH 4/5] git send-email: ask less questions when --compose is used.

[Francis Galiegue]

Le Tuesday 04 November 2008 17:24:17, vous avez écrit :
> +       if ($summary_empty) {
> +               print "Summary email is empty, --> skpping <-- it\n";
> 

Typo...

-- 
fge

Re: [PATCH 2/5] git send-email: interpret unknown files as revision lists

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index aaace02..c29868a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -22,8 +22,11 @@ use Term::ReadLine;
>  use Getopt::Long;
>  use Data::Dumper;
>  use Term::ANSIColor;
> +use File::Temp qw/ tempdir /;

We seem to use File::Temp::tempdir already elsewhere, but they are in
archimport, cvsexportcommit and cvsserver, all of which are rather rarely
used ones.  I think this is Perl 5.6.1 addition.  Is everybody Ok with
this dependency?  Just double checking.

> @@ -363,10 +366,22 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
>  
>  ($sender) = expand_aliases($sender) if defined $sender;
>  
> +sub check_file_rev_conflict($) {
> +	my $f = shift;
> +	if ($repo->command('rev-parse', '--verify', '--quiet', $f)) {
> +		die("revision/filename conflict on `$f'");

Perhaps wording this a bit more to the point?  This is triggered when 
'$f' can be both a filename or a revision, so...

	File '$f' exists but it could also be the range of commits
        to produce patches for.  Please disambiguate by...

	* Saying "./$f" if you mean a file; or
        * Giving -F option if you mean a range.

Earlier I suggested that "origin^0" is a way for the user to disambiguate
favouring a rev, but such a filename can exist, so we cannot blindly
suggest to say "$f^0" here.  I think adding -F (or --format-patch) option
to send-email to explicitly disable file/directory interpretation would be
a cleaner solution for this (and it would allow you to drive this from a
script without worrying about what garbage files you happen to have in the
working tree).

Re: [PATCH 4/5] git send-email: ask less questions when --compose is used.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> +	print C <<EOT;
> +From $tpl_sender # This line is ignored.
> +GIT: Lines beginning in "GIT: " will be removed.
> +GIT: Consider including an overall diffstat or table of contents
> +GIT: for the patch you are writing.
> +GIT:
> +GIT: Clear the body content if you don't wish to send a summary.
> +From: $tpl_sender
> +Subject: $tpl_subject
> +In-Reply-To: $tpl_reply_to
> +

Somebody already suggested this but I really think GIT: lines should be at
the end and use '# ' prefix instead.

With the ability to give --cover-letter option to underlying format-patch
do you still need this?

> +	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> +
> +	if ($annotate) {
> +		do_edit($compose_filename, @files);
> +	} else {
> +		do_edit($compose_filename);
> +	}

Don't we want to abort the whole process when the user kills the editor
instead of normal exit (iow, do_edit() which is system() reports that the
editor was killed)?

Re: [PATCH 5/5] git send-email: turn --compose on when more than one patch.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> Automatically turn --compose on when there is more than one patch, and
> that the output is a tty.

I do not think this is a good idea.  I suspect I am not the only person
who uses "format-patch --cover-letter", edit the files to review and
prepare, and runs send-email to fire them off.

Re: [PATCH 5/5] git send-email: turn --compose on when more than one patch.

[Jeff King]

On Tue, Nov 04, 2008 at 03:54:54PM -0800, Junio C Hamano wrote:

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > Automatically turn --compose on when there is more than one patch, and
> > that the output is a tty.
> 
> I do not think this is a good idea.  I suspect I am not the only person
> who uses "format-patch --cover-letter", edit the files to review and
> prepare, and runs send-email to fire them off.

Maybe a config option to turn this behavior on? It seems specific to
different workflows (i.e., whether or not you are using "git send-email
$REVS" or using format-patch first).

-Peff

Re: [PATCH 5/5] git send-email: turn --compose on when more than one patch.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 04, 2008 at 03:54:54PM -0800, Junio C Hamano wrote:
>
>> Pierre Habouzit <madcoder@debian.org> writes:
>> 
>> > Automatically turn --compose on when there is more than one patch, and
>> > that the output is a tty.
>> 
>> I do not think this is a good idea.  I suspect I am not the only person
>> who uses "format-patch --cover-letter", edit the files to review and
>> prepare, and runs send-email to fire them off.
>
> Maybe a config option to turn this behavior on? It seems specific to
> different workflows (i.e., whether or not you are using "git send-email
> $REVS" or using format-patch first).

Yeah, if send-email did not have --compose to begin with, we could just
say "don't use --compose; use --cover-letter when you use send-email to
front-end format-patch instead", but some people perhaps are used to run
format-patch separately without --cover-letter and then create the cover
letter from scratch with --compose (which seems a bit more work to me,
though).

So I am not opposed to a sendemail.foo configuration option.

Re: [PATCH 2/5] git send-email: interpret unknown files as revision lists

[Pierre Habouzit]

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

On Tue, Nov 04, 2008 at 11:54:26PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:

> > @@ -363,10 +366,22 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
> >  
> >  ($sender) = expand_aliases($sender) if defined $sender;
> >  
> > +sub check_file_rev_conflict($) {
> > +	my $f = shift;
> > +	if ($repo->command('rev-parse', '--verify', '--quiet', $f)) {
> > +		die("revision/filename conflict on `$f'");
> 
> Perhaps wording this a bit more to the point?  This is triggered when 
> '$f' can be both a filename or a revision, so...
> 
> 	File '$f' exists but it could also be the range of commits
>         to produce patches for.  Please disambiguate by...
> 
> 	* Saying "./$f" if you mean a file; or
>         * Giving -F option if you mean a range.
> 
> Earlier I suggested that "origin^0" is a way for the user to disambiguate
> favouring a rev, but such a filename can exist, so we cannot blindly
> suggest to say "$f^0" here.  I think adding -F (or --format-patch) option
> to send-email to explicitly disable file/directory interpretation would be
> a cleaner solution for this (and it would allow you to drive this from a
> script without worrying about what garbage files you happen to have in the
> working tree).

Okay, still having --[no-]format-patch is probably a good idea indeed
for scripts. Will do.

On Tue, Nov 04, 2008 at 11:54:39PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > +	print C <<EOT;
> > +From $tpl_sender # This line is ignored.
> > +GIT: Lines beginning in "GIT: " will be removed.
> > +GIT: Consider including an overall diffstat or table of contents
> > +GIT: for the patch you are writing.
> > +GIT:
> > +GIT: Clear the body content if you don't wish to send a summary.
> > +From: $tpl_sender
> > +Subject: $tpl_subject
> > +In-Reply-To: $tpl_reply_to
> > +
> 
> Somebody already suggested this but I really think GIT: lines should be at
> the end and use '# ' prefix instead.

This will break previous editor syntax hilighting stuff even more, and
has the drawback that you can't put shell sniplets in here. I think it's
why GIT: was chosen. But maybe we just don't care.

> With the ability to give --cover-letter option to underlying format-patch
> do you still need this?
> 
> > +	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> > +
> > +	if ($annotate) {
> > +		do_edit($compose_filename, @files);
> > +	} else {
> > +		do_edit($compose_filename);
> > +	}
> 
> Don't we want to abort the whole process when the user kills the editor
> instead of normal exit (iow, do_edit() which is system() reports that the
> editor was killed)?

Probably, I kept what was done as is, but we probably want do_edit() to
die() if the user killed it.

On mer, nov 05, 2008 at 07:03:42 +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> > On Tue, Nov 04, 2008 at 03:54:54PM -0800, Junio C Hamano wrote:
> Yeah, if send-email did not have --compose to begin with, we could just
> say "don't use --compose; use --cover-letter when you use send-email to
> front-end format-patch instead", but some people perhaps are used to run
> format-patch separately without --cover-letter and then create the cover
> letter from scratch with --compose (which seems a bit more work to me,
> though).
> 
> So I am not opposed to a sendemail.foo configuration option.

Will do


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/5] git send-email: interpret unknown files as revision lists

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Tue, Nov 04, 2008 at 11:54:26PM +0000, Junio C Hamano wrote:
>
>> Somebody already suggested this but I really think GIT: lines should be at
>> the end and use '# ' prefix instead.
>
> This will break previous editor syntax hilighting stuff even more, and
> has the drawback that you can't put shell sniplets in here. I think it's
> why GIT: was chosen. But maybe we just don't care.

Ah, please scratch that "I really think" --- my mistake.

I did not check nor realize "GIT:" is what send-email already does.

Re: [PATCH 2/5] git send-email: interpret unknown files as revision lists

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> Okay, still having --[no-]format-patch is probably a good idea indeed
> for scripts. Will do.
> ...
> Probably, I kept what was done as is, but we probably want do_edit() to
> die() if the user killed it.
> ...
>> So I am not opposed to a sendemail.foo configuration option.
>
> Will do

Thanks.

[take 2] git send-email updates

[Pierre Habouzit]

The last patch is dropped for now (the automatic --compose stuff)
because I'm not sure which option to add, and that I don't care enough
about it to spend more time on it.

I think I've incorporated most of the stuff people asked about in this
series.

 [PATCH 1/4] git send-email: make the message file name more specific.
 [PATCH 2/4] git send-email: interpret unknown files as revision lists
 [PATCH 3/4] git send-email: add --annotate option
 [PATCH 4/4] git send-email: ask less questions when --compose is used.

[PATCH 1/4] git send-email: make the message file name more specific.

[Pierre Habouzit]

This helps editors choosing their syntax hilighting properly.

Also make the file live under the git directory.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 git-send-email.perl |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 94ca5c8..aaace02 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -124,9 +124,6 @@ my $auth;
 sub unique_email_list(@);
 sub cleanup_compose_files();
 
-# Constants (essentially)
-my $compose_filename = ".msg.$$";
-
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
 	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
@@ -149,6 +146,7 @@ if ($@) {
 
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
+my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
-- 
1.6.0.4.859.g7ecd.dirty

[PATCH 2/4] git send-email: interpret unknown files as revision lists

[Pierre Habouzit]

Filter out all the arguments git-send-email doesn't like to a
git format-patch command, that dumps its content to a safe directory.

Barf when a file/revision conflict occurs, allow it to be overriden
--[no-]format-patch.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |    8 +++++-
 git-send-email.perl              |   47 +++++++++++++++++++++++++++++++++----
 t/t9001-send-email.sh            |    8 ++++++
 3 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 82f5056..0beaad4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -8,7 +8,7 @@ git-send-email - Send a collection of patches as emails
 
 SYNOPSIS
 --------
-'git send-email' [options] <file|directory> [... file|directory]
+'git send-email' [options] <file|directory|rev-list options>...
 
 
 DESCRIPTION
@@ -183,6 +183,12 @@ Administering
 --[no-]validate::
 	Perform sanity checks on patches.
 	Currently, validation means the following:
+
+--[no-]format-patch::
+	When an argument may be understood either as a reference or as a file name,
+	choose to understand it as a format-patch argument ('--format-patch')
+	or as a file name ('--no-format-patch'). By default, when such a conflict
+	occurs, git send-email will fail.
 +
 --
 		*	Warn of patches that contain lines longer than 998 characters; this
diff --git a/git-send-email.perl b/git-send-email.perl
index aaace02..6f5a613 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -22,8 +22,12 @@ use Term::ReadLine;
 use Getopt::Long;
 use Data::Dumper;
 use Term::ANSIColor;
+use File::Temp qw/ tempdir /;
+use Error qw(:try);
 use Git;
 
+Getopt::Long::Configure qw/ pass_through /;
+
 package FakeTerm;
 sub new {
 	my ($class, $reason) = @_;
@@ -38,7 +42,7 @@ package main;
 
 sub usage {
 	print <<EOT;
-git send-email [options] <file | directory>...
+git send-email [options] <file | directory | rev-list options >
 
   Composing:
     --from                  <str>  * Email From:
@@ -73,6 +77,8 @@ git send-email [options] <file | directory>...
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
+    --[no-]format-patch            * understand any non optional arguments as
+                                     `git format-patch` ones.
 
 EOT
 	exit(1);
@@ -146,6 +152,7 @@ if ($@) {
 
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
+my $format_patch;
 my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
 # Variables with corresponding config settings
@@ -229,6 +236,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
 		    "validate!" => \$validate,
+		    "format-patch!" => \$format_patch,
 	 );
 
 unless ($rc) {
@@ -363,23 +371,52 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 
 ($sender) = expand_aliases($sender) if defined $sender;
 
+# returns 1 if the conflict must be solved using it as a format-patch argument
+sub check_file_rev_conflict($) {
+	my $f = shift;
+	try {
+		$repo->command('rev-parse', '--verify', '--quiet', $f);
+		if (defined($format_patch)) {
+			print "foo\n";
+			return $format_patch;
+		}
+		die(<<EOF);
+File '$f' exists but it could also be the range of commits
+to produce patches for.  Please disambiguate by...
+
+    * Saying "./$f" if you mean a file; or
+    * Giving --format-patch option if you mean a range.
+EOF
+	} catch Git::Error::Command with {
+		return 0;
+	}
+}
+
 # Now that all the defaults are set, process the rest of the command line
 # arguments and collect up the files that need to be processed.
-for my $f (@ARGV) {
-	if (-d $f) {
+my @rev_list_opts;
+while (my $f = pop @ARGV) {
+	if ($f eq "--") {
+		push @rev_list_opts, "--", @ARGV;
+		@ARGV = ();
+	} elsif (-d $f and !check_file_rev_conflict($f)) {
 		opendir(DH,$f)
 			or die "Failed to opendir $f: $!";
 
 		push @files, grep { -f $_ } map { +$f . "/" . $_ }
 				sort readdir(DH);
 		closedir(DH);
-	} elsif (-f $f or -p $f) {
+	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
 		push @files, $f;
 	} else {
-		print STDERR "Skipping $f - not found.\n";
+		push @rev_list_opts, $f;
 	}
 }
 
+if (@rev_list_opts) {
+	push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts);
+}
+
 if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 561ae7d..b4bddd1 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -292,4 +292,12 @@ test_expect_success '--compose adds MIME for utf8 subject' '
 	grep "^Subject: =?utf-8?q?utf8-s=C3=BCbj=C3=ABct?=" msgtxt1
 '
 
+test_expect_success 'detects ambiguous reference/file conflict' '
+	echo master > master &&
+	git add master &&
+	git commit -m"add master" &&
+	test_must_fail git send-email --dry-run master > errors &&
+	grep disambiguate errors
+'
+
 test_done
-- 
1.6.0.4.859.g7ecd.dirty

[PATCH 3/4] git send-email: add --annotate option

[Pierre Habouzit]

This allows to review every patch (and fix various aspects of them, or
comment them) in an editor just before being sent. Combined to the fact
that git send-email can now process revision lists, this makes git
send-email and efficient way to review and send patches interactively.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |   11 +++++++++++
 git-send-email.perl              |   26 ++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 0beaad4..66d5f4c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -37,6 +37,11 @@ The --bcc option must be repeated for each user you want on the bcc list.
 +
 The --cc option must be repeated for each user you want on the cc list.
 
+--annotate::
+	Review each patch you're about to send in an editor. The setting
+	'sendemail.multiedit' defines if this will spawn one editor per patch
+	or one for all of them at once.
+
 --compose::
 	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
 	introductory message for the patch series.
@@ -210,6 +215,12 @@ sendemail.aliasfiletype::
 	Format of the file(s) specified in sendemail.aliasesfile. Must be
 	one of 'mutt', 'mailrc', 'pine', or 'gnus'.
 
+sendemail.multiedit::
+	If true (default), a single editor instance will be spawned to edit
+	files you have to edit (patches when '--annotate' is used, and the
+	summary when '--compose' is used). If false, files will be edited one
+	after the other, spawning a new editor each time.
+
 
 Author
 ------
diff --git a/git-send-email.perl b/git-send-email.perl
index 6f5a613..ccb3b18 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -51,6 +51,7 @@ git send-email [options] <file | directory | rev-list options >
     --bcc                   <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
+    --annotate                     * Review each patch that will be sent in an editor.
     --compose                      * Open an editor for introduction.
 
   Sending:
@@ -132,7 +133,8 @@ sub cleanup_compose_files();
 
 # Variables we fill in automatically, or via prompting:
 my (@to,@cc,@initial_cc,@bcclist,@xh,
-	$initial_reply_to,$initial_subject,@files,$author,$sender,$smtp_authpass,$compose,$time);
+	$initial_reply_to,$initial_subject,@files,
+	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
 
 my $envelope_sender;
 
@@ -155,6 +157,17 @@ my ($quiet, $dry_run) = (0, 0);
 my $format_patch;
 my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
+# Handle interactive edition of files.
+my $multiedit;
+my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+sub do_edit {
+	if (defined($multiedit) && !$multiedit) {
+		map { system('sh', '-c', $editor.' "$@"', $editor, $_); } @_;
+	} else {
+		system('sh', '-c', $editor.' "$@"', $editor, @_);
+	}
+}
+
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
@@ -184,6 +197,7 @@ my %config_settings = (
     "aliasesfile" => \@alias_files,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
+    "multiedit" => \$multiedit,
 );
 
 # Handle Uncouth Termination
@@ -226,6 +240,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "identity=s" => \$identity,
+		    "annotate" => \$annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
@@ -532,7 +547,12 @@ EOT
 	close(C);
 
 	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-	system('sh', '-c', $editor.' "$@"', $editor, $compose_filename);
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
 
 	open(C2,">",$compose_filename . ".final")
 		or die "Failed to open $compose_filename.final : " . $!;
@@ -581,6 +601,8 @@ EOT
 	}
 
 	@files = ($compose_filename . ".final", @files);
+} elsif ($annotate) {
+	do_edit(@files);
 }
 
 # Variables we set as part of the loop over files
-- 
1.6.0.4.859.g7ecd.dirty

[PATCH 4/4] git send-email: ask less questions when --compose is used.

[Pierre Habouzit]

When --compose is used, we can grab the From/Subject/In-Reply-To from the
edited summary, let it be so and don't ask the user silly questions.

The summary templates gets quite revamped, and includes the list of
patches subjects that are going to be sent with this batch.

When having a body full of empty lines, the summary isn't sent. Document
that in the git-send-email manpage fully.

Note: It doesn't deal with To/Cc/Bcc yet.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 Documentation/git-send-email.txt |    9 ++
 git-send-email.perl              |  187 +++++++++++++++++++++++---------------
 2 files changed, 123 insertions(+), 73 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 66d5f4c..acf8bf4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -45,6 +45,15 @@ The --cc option must be repeated for each user you want on the cc list.
 --compose::
 	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
 	introductory message for the patch series.
++
+When compose is in used, git send-email gets less interactive will use the
+values of the headers you set there. If the body of the email (what you type
+after the headers and a blank line) only contains blank (or GIT: prefixed)
+lines, the summary won't be sent, but git-send-email will still use the
+Headers values if you don't removed them.
++
+If it wasn't able to see a header in the summary it will ask you about it
+interactively after quitting your editor.
 
 --from::
 	Specify the sender of the emails.  This will default to
diff --git a/git-send-email.perl b/git-send-email.perl
index ccb3b18..9039cfd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -162,9 +162,17 @@ my $multiedit;
 my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
-		map { system('sh', '-c', $editor.' "$@"', $editor, $_); } @_;
+		map {
+			system('sh', '-c', $editor.' "$@"', $editor, $_);
+			if (($? & 127) || ($? >> 8)) {
+				die("the editor exited uncleanly, aborting everything");
+			}
+		} @_;
 	} else {
 		system('sh', '-c', $editor.' "$@"', $editor, @_);
+		if (($? & 127) || ($? >> 8)) {
+			die("the editor exited uncleanly, aborting everything");
+		}
 	}
 }
 
@@ -450,6 +458,108 @@ if (@files) {
 	usage();
 }
 
+sub get_patch_subject($) {
+	my $fn = shift;
+	open (my $fh, '<', $fn);
+	while (my $line = <$fh>) {
+		next unless ($line =~ /^Subject: (.*)$/);
+		close $fh;
+		return "GIT: $1\n";
+	}
+	close $fh;
+	die "No subject line in $fn ?";
+}
+
+if ($compose) {
+	# Note that this does not need to be secure, but we will make a small
+	# effort to have it be unique
+	open(C,">",$compose_filename)
+		or die "Failed to open for writing $compose_filename: $!";
+
+
+	my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
+	my $tpl_subject = $initial_subject || '';
+	my $tpl_reply_to = $initial_reply_to || '';
+
+	print C <<EOT;
+From $tpl_sender # This line is ignored.
+GIT: Lines beginning in "GIT: " will be removed.
+GIT: Consider including an overall diffstat or table of contents
+GIT: for the patch you are writing.
+GIT:
+GIT: Clear the body content if you don't wish to send a summary.
+From: $tpl_sender
+Subject: $tpl_subject
+In-Reply-To: $tpl_reply_to
+
+EOT
+	for my $f (@files) {
+		print C get_patch_subject($f);
+	}
+	close(C);
+
+	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}
+
+	open(C2,">",$compose_filename . ".final")
+		or die "Failed to open $compose_filename.final : " . $!;
+
+	open(C,"<",$compose_filename)
+		or die "Failed to open $compose_filename : " . $!;
+
+	my $need_8bit_cte = file_has_nonascii($compose_filename);
+	my $in_body = 0;
+	my $summary_empty = 1;
+	while(<C>) {
+		next if m/^GIT: /;
+		if ($in_body) {
+			$summary_empty = 0 unless (/^\n$/);
+		} elsif (/^\n$/) {
+			$in_body = 1;
+			if ($need_8bit_cte) {
+				print C2 "MIME-Version: 1.0\n",
+					 "Content-Type: text/plain; ",
+					   "charset=utf-8\n",
+					 "Content-Transfer-Encoding: 8bit\n";
+			}
+		} elsif (/^MIME-Version:/i) {
+			$need_8bit_cte = 0;
+		} elsif (/^Subject:\s*(.+)\s*$/i) {
+			$initial_subject = $1;
+			my $subject = $initial_subject;
+			$_ = "Subject: " .
+				($subject =~ /[^[:ascii:]]/ ?
+				 quote_rfc2047($subject) :
+				 $subject) .
+				"\n";
+		} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
+			$initial_reply_to = $1;
+			next;
+		} elsif (/^From:\s*(.+)\s*$/i) {
+			$sender = $1;
+			next;
+		} elsif (/^(?:To|Cc|Bcc):/i) {
+			print "To/Cc/Bcc fields are not interpreted yet, they have been ignored\n";
+			next;
+		}
+		print C2 $_;
+	}
+	close(C);
+	close(C2);
+
+	if ($summary_empty) {
+		print "Summary email is empty, skipping it\n";
+		$compose = -1;
+	}
+} elsif ($annotate) {
+	do_edit(@files);
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
@@ -494,17 +604,6 @@ sub expand_aliases {
 @initial_cc = expand_aliases(@initial_cc);
 @bcclist = expand_aliases(@bcclist);
 
-if (!defined $initial_subject && $compose) {
-	while (1) {
-		$_ = $term->readline("What subject should the initial email start with? ", $initial_subject);
-		last if defined $_;
-		print "\n";
-	}
-
-	$initial_subject = $_;
-	$prompting++;
-}
-
 if ($thread && !defined $initial_reply_to && $prompting) {
 	while (1) {
 		$_= $term->readline("Message-ID to be used as In-Reply-To for the first email? ", $initial_reply_to);
@@ -531,64 +630,6 @@ if (!defined $smtp_server) {
 }
 
 if ($compose) {
-	# Note that this does not need to be secure, but we will make a small
-	# effort to have it be unique
-	open(C,">",$compose_filename)
-		or die "Failed to open for writing $compose_filename: $!";
-	print C "From $sender # This line is ignored.\n";
-	printf C "Subject: %s\n\n", $initial_subject;
-	printf C <<EOT;
-GIT: Please enter your email below.
-GIT: Lines beginning in "GIT: " will be removed.
-GIT: Consider including an overall diffstat or table of contents
-GIT: for the patch you are writing.
-
-EOT
-	close(C);
-
-	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
-
-	if ($annotate) {
-		do_edit($compose_filename, @files);
-	} else {
-		do_edit($compose_filename);
-	}
-
-	open(C2,">",$compose_filename . ".final")
-		or die "Failed to open $compose_filename.final : " . $!;
-
-	open(C,"<",$compose_filename)
-		or die "Failed to open $compose_filename : " . $!;
-
-	my $need_8bit_cte = file_has_nonascii($compose_filename);
-	my $in_body = 0;
-	while(<C>) {
-		next if m/^GIT: /;
-		if (!$in_body && /^\n$/) {
-			$in_body = 1;
-			if ($need_8bit_cte) {
-				print C2 "MIME-Version: 1.0\n",
-					 "Content-Type: text/plain; ",
-					   "charset=utf-8\n",
-					 "Content-Transfer-Encoding: 8bit\n";
-			}
-		}
-		if (!$in_body && /^MIME-Version:/i) {
-			$need_8bit_cte = 0;
-		}
-		if (!$in_body && /^Subject: ?(.*)/i) {
-			my $subject = $1;
-			$_ = "Subject: " .
-				($subject =~ /[^[:ascii:]]/ ?
-				 quote_rfc2047($subject) :
-				 $subject) .
-				"\n";
-		}
-		print C2 $_;
-	}
-	close(C);
-	close(C2);
-
 	while (1) {
 		$_ = $term->readline("Send this email? (y|n) ");
 		last if defined $_;
@@ -600,9 +641,9 @@ EOT
 		exit(0);
 	}
 
-	@files = ($compose_filename . ".final", @files);
-} elsif ($annotate) {
-	do_edit(@files);
+	if ($compose > 0) {
+		@files = ($compose_filename . ".final", @files);
+	}
 }
 
 # Variables we set as part of the loop over files
-- 
1.6.0.4.859.g7ecd.dirty

Re: [take 2] git send-email updates

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> The last patch is dropped for now (the automatic --compose stuff)
> because I'm not sure which option to add, and that I don't care enough
> about it to spend more time on it.
>
> I think I've incorporated most of the stuff people asked about in this
> series.
>
>  [PATCH 1/4] git send-email: make the message file name more specific.
>  [PATCH 2/4] git send-email: interpret unknown files as revision lists
>  [PATCH 3/4] git send-email: add --annotate option
>  [PATCH 4/4] git send-email: ask less questions when --compose is used.

Thanks.

It is somewhat unfortunate that an explicit --no-format-patch works
exactly the same way as not giving the option at all.  I would have
expected that it would guess and warn if you did not give either, and it
would not even guess (i.e. file is mbox, dir is maildir) and error out if
there is a leftover option in @rev_list_opts if the user explicitly asked
the command not act as a frontend to format patch.

I will queue the series in 'pu' because I suspect you would like a chance
to amend out a "print foo" from the second commit ;-)

Re: [take 2] git send-email updates

[Pierre Habouzit]

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

On Tue, Nov 11, 2008 at 08:30:51PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > The last patch is dropped for now (the automatic --compose stuff)
> > because I'm not sure which option to add, and that I don't care enough
> > about it to spend more time on it.
> >
> > I think I've incorporated most of the stuff people asked about in this
> > series.
> >
> >  [PATCH 1/4] git send-email: make the message file name more specific.
> >  [PATCH 2/4] git send-email: interpret unknown files as revision lists
> >  [PATCH 3/4] git send-email: add --annotate option
> >  [PATCH 4/4] git send-email: ask less questions when --compose is used.
> 
> Thanks.
> 
> It is somewhat unfortunate that an explicit --no-format-patch works
> exactly the same way as not giving the option at all.

Unless I'm mistaken in my code, and I may really be, it doesn't.
--format-patch says that in case of conflicts, the "revision" kind of
argument wins, --no-format-patch says that the "file" one wins, without
any it dies with an error. It's really a tristate, but maybe I missed
your point ?

> I would have expected that it would guess and warn if you did not give
> either, and it would not even guess (i.e. file is mbox, dir is
> maildir) and error out if there is a leftover option in @rev_list_opts
> if the user explicitly asked the command not act as a frontend to
> format patch.

Oh you mean that if one use --no-format-patch you don't wan't _any_
option to be passed to format-patch ? Hmmm I don't know, both what I did
and that are sane, I don't really know what to chose. But if we're going
to go down this road, _your_ --no-format-patch and --format-patch don't
quite do the opposite, as --format-patch still allows files to be passed
to it.

If we're really doing this, then maybe we want a 5-state kind of option:
  1 disallow any file name ;
  2 if conflict, chose the revision ;
  3 barf if any conflict arises (default) ;
  4 if conflict chose the file ;
  5 disallow any kind of revision argument.

My proposal implements 2 as --format-patch, 3 as default, and 4 as
--no-format-patch. You propose basically (5) for --no-format-patch
instead, well I say this makes sense, but it's somehow "sad" not to have
(1) too in that case.

But in the end, I believe this _may_ quite be slightly over-engineered in
the end ;) I would gladly implement the combination people like most, as
soon as I can pass format-patch option a way or the other, I'm happy :)

> I will queue the series in 'pu' because I suspect you would like a chance
> to amend out a "print foo" from the second commit ;-)

*ooops*

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [take 2] git send-email updates

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> Oh you mean that if one use --no-format-patch you don't wan't _any_
> option to be passed to format-patch?

The option name --no-format-patch sounded like "I do not want you to act
as a frontend, ever", i.e. if you type master..next by mistake on the
command line, the command would barf when the option is given.  Not even
"pass to format-patch", but "do not run format-patch to begin with".

It is not a big deal especially for interactive use (and that is why I
said "somewhat" unfortunate).

> If we're really doing this, then maybe we want a 5-state kind of option:
>   1 disallow any file name ;
>   2 if conflict, chose the revision ;
>   3 barf if any conflict arises (default) ;
>   4 if conflict chose the file ;
>   5 disallow any kind of revision argument.
>
> My proposal implements 2 as --format-patch, 3 as default, and 4 as
> --no-format-patch. You propose basically (5) for --no-format-patch
> instead, well I say this makes sense, but it's somehow "sad" not to have
> (1) too in that case.

Actually, "send-email --format-patch master..fixes Documentation/" may be
a useful command to send out only documentation fixes.  For such a usage,
Documentation/ should not be taken as a maildir.  If we would want to
support such usage (and I'd say why not), a token can fall into one (or
two) of three categories:

    - can it be a rev?

    - is it a tracked path (either blob or a leading dir)?

    - is it a file/dir that is not tracked?

The first two would be format-patch candidate.  The last one is the
traditional mail source.  Because the latter two are disjoint set, and
because it does not matter if you have a tracked file 'master' and a
branch 'master' in your repo (either will be passed to format-patch
anyway), the actual disambiguity is reduced, but it still is different
from what you have in your patch, I suspect.

As to options, how about doing this:

    --no-format-patch means never ever run format-patch, behave exactly as
    before;

    --format-patch means what you have in your patch.  guess and favor 
    format-patch parameter when ambiguous;

    without either option, guess and favor mbox/maildir but still run
    format-patch if remaining parameters and options need to
    (e.g. "send-email my-cover-letter origin/master..master" will find
    my-cover-letter which is not tracked and take it as mbox, and grab
    patches from commits between origin/master..master, and send all of
    them).

Re: [PATCH 2/4] git send-email: interpret unknown files as revision lists

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> +test_expect_success 'detects ambiguous reference/file conflict' '
> +	echo master > master &&
> +	git add master &&
> +	git commit -m"add master" &&
> +	test_must_fail git send-email --dry-run master > errors &&
> +	grep disambiguate errors
> +'

I've queued the series in 'pu', but with a fix to this test, without which
it did not pass (and I had to rewind and rebuild 'pu').

Re* [take 2] git send-email updates

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Actually, "send-email --format-patch master..fixes Documentation/" may be
> a useful command to send out only documentation fixes.  For such a usage,
> Documentation/ should not be taken as a maildir.  If we would want to
> support such usage (and I'd say why not), a token can fall into one (or
> two) of three categories:
>
>     - can it be a rev?
>
>     - is it a tracked path (either blob or a leading dir)?
>
>     - is it a file/dir that is not tracked?
>
> The first two would be format-patch candidate.  The last one is the
> traditional mail source.  Because the latter two are disjoint set, and
> because it does not matter if you have a tracked file 'master' and a
> branch 'master' in your repo (either will be passed to format-patch
> anyway), the actual disambiguity is reduced, but it still is different
> from what you have in your patch, I suspect.
>
> As to options, how about doing this:
>
>     --no-format-patch means never ever run format-patch, behave exactly as
>     before;
>
>     --format-patch means what you have in your patch.  guess and favor 
>     format-patch parameter when ambiguous;
>
>     without either option, guess and favor mbox/maildir but still run
>     format-patch if remaining parameters and options need to
>     (e.g. "send-email my-cover-letter origin/master..master" will find
>     my-cover-letter which is not tracked and take it as mbox, and grab
>     patches from commits between origin/master..master, and send all of
>     them).

This patch on top of your [2/4] illustrates what I had in mind (it also
removes the "print foo" while at it).

 git-send-email.perl |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git c/git-send-email.perl w/git-send-email.perl
index 6f5a613..9aa3500 100755
--- c/git-send-email.perl
+++ w/git-send-email.perl
@@ -152,7 +152,7 @@ if ($@) {
 
 # Behavior modification variables
 my ($quiet, $dry_run) = (0, 0);
-my $format_patch;
+my $format_patch = 'unspecified';
 my $compose_filename = $repo->repo_path() . "/.gitsendemail.msg.$$";
 
 # Variables with corresponding config settings
@@ -243,6 +243,15 @@ unless ($rc) {
     usage();
 }
 
+if ($format_patch && $format_patch eq 'unspecified') {
+	# No --format-patch nor --no-format-patch on the command line
+	$format_patch = 0;
+} elsif (!$format_patch) {
+	$format_patch = undef;
+} else {
+	$format_patch = 1;
+}
+
 # Now, let's fill any that aren't set in with defaults:
 
 sub read_config {
@@ -374,11 +383,27 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 # returns 1 if the conflict must be solved using it as a format-patch argument
 sub check_file_rev_conflict($) {
 	my $f = shift;
+
+	if (!defined $format_patch) {
+		# The command line explicitly forbids acting as a wrapper
+		return 0;
+	}
+
+	# If it is a tracked path it can't be tracking the e-mails you
+	# are going to send out to describe the change to this repository.
+	eval {
+		$repo->command(['ls-files', '--error-unmatch', $f],
+			       { STDERR => 0 });
+	};
+	if (!$@) {
+		return 1;
+	}
+
+	# Can it be interpreted as a rev?
 	try {
 		$repo->command('rev-parse', '--verify', '--quiet', $f);
-		if (defined($format_patch)) {
-			print "foo\n";
-			return $format_patch;
+		if ($format_patch) {
+			return 1;
 		}
 		die(<<EOF);
 File '$f' exists but it could also be the range of commits
@@ -408,6 +433,8 @@ while (my $f = pop @ARGV) {
 		closedir(DH);
 	} elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
 		push @files, $f;
+	} elsif (!defined $format_patch) {
+		die("--no-format-patch was given but $f is not a valid send-email argument");
 	} else {
 		push @rev_list_opts, $f;
 	}

Re: [take 2] git send-email updates

[Pierre Habouzit]

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

On Wed, Nov 12, 2008 at 12:14:20AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > Oh you mean that if one use --no-format-patch you don't wan't _any_
> > option to be passed to format-patch?
> 
> The option name --no-format-patch sounded like "I do not want you to act
> as a frontend, ever", i.e. if you type master..next by mistake on the
> command line, the command would barf when the option is given.  Not even
> "pass to format-patch", but "do not run format-patch to begin with".
> 
> It is not a big deal especially for interactive use (and that is why I
> said "somewhat" unfortunate).
> 
> > If we're really doing this, then maybe we want a 5-state kind of option:
> >   1 disallow any file name ;
> >   2 if conflict, chose the revision ;
> >   3 barf if any conflict arises (default) ;
> >   4 if conflict chose the file ;
> >   5 disallow any kind of revision argument.
> >
> > My proposal implements 2 as --format-patch, 3 as default, and 4 as
> > --no-format-patch. You propose basically (5) for --no-format-patch
> > instead, well I say this makes sense, but it's somehow "sad" not to have
> > (1) too in that case.
> 
> Actually, "send-email --format-patch master..fixes Documentation/" may be
> a useful command to send out only documentation fixes.  For such a usage,
> Documentation/ should not be taken as a maildir.  If we would want to
> support such usage (and I'd say why not), a token can fall into one (or
> two) of three categories:

You can do that doing:

git send-email --format-patch master..fixes -- Documentation/

I've kept the `--` usual meaning, and it's sent to git-format-patch
verbatim and it'll work, so it's not required to change the meaning of
the options for that.

[sorry for the late reply, I've been somehow busy lately]

The sole conflict we have is when there is a path/rev conflict *before*
the `--` because of the legacy of git-send-email. I believe that
--format-patch should still allow to send patches passed on the command
line, this way.

> As to options, how about doing this:
> 
>     --no-format-patch means never ever run format-patch, behave exactly as
>     before;
> 
>     --format-patch means what you have in your patch.  guess and favor 
>     format-patch parameter when ambiguous;
> 
>     without either option, guess and favor mbox/maildir but still run
>     format-patch if remaining parameters and options need to
>     (e.g. "send-email my-cover-letter origin/master..master" will find
>     my-cover-letter which is not tracked and take it as mbox, and grab
>     patches from commits between origin/master..master, and send all of
>     them).

That's sane and I like it.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Re* [take 2] git send-email updates

[Pierre Habouzit]

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

On Thu, Nov 13, 2008 at 12:01:46AM +0000, Junio C Hamano wrote:
> This patch on top of your [2/4] illustrates what I had in mind (it also
> removes the "print foo" while at it).

I like it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]