[PATCH] send-email: Refuse to send cover-letter template subject

[PATCH] send-email: Refuse to send cover-letter template subject

[Thomas Rast]

Every so often, someone sends out an unedited cover-letter template.
Add a simple check to send-email that refuses to send if the subject
contains "*** SUBJECT HERE ***", with an option --force to override.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-send-email.perl   |   15 +++++++++++++++
 t/t9001-send-email.sh |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3d6a982..293ee46 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -82,6 +82,7 @@
     --[no-]validate                * Perform patch sanity checks. Default on.
     --[no-]format-patch            * understand any non optional arguments as
                                      `git format-patch` ones.
+    --force                        * Send even if safety checks would prevent it.
 
 EOT
 	exit(1);
@@ -159,6 +160,7 @@
 my ($quiet, $dry_run) = (0, 0);
 my $format_patch;
 my $compose_filename;
+my $force = 0;
 
 # Handle interactive edition of files.
 my $multiedit;
@@ -268,6 +270,7 @@
 		    "thread!" => \$thread,
 		    "validate!" => \$validate,
 		    "format-patch!" => \$format_patch,
+		    "force" => \$force,
 	 );
 
 unless ($rc) {
@@ -638,6 +641,18 @@ sub get_patch_subject($) {
 	return undef;
 }
 
+
+if (!$force) {
+	for my $f (@files) {
+		if (get_patch_subject($f) =~ /\*\*\* SUBJECT HERE \*\*\*/) {
+			die "Refusing to send because the patch\n\t$f\n"
+				. "has the template subject '*** SUBJECT HERE ***'. "
+				. "Pass --force if you really want to send.\n";
+		}
+	}
+
+}
+
 my $prompting = 0;
 if (!defined $sender) {
 	$sender = $repoauthor || $repocommitter || '';
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index ce26ea4..af5e73e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -621,4 +621,40 @@ test_expect_success 'in-reply-to but no threading' '
 	grep "In-Reply-To: <in-reply-id@example.com>"
 '
 
+# Note that the patches in this test are deliberately out of order; we
+# want to make sure it works even if the cover-letter is not in the
+# first mail.
+test_expect_success 'refusing to send cover letter template' '
+	clean_fake_sendmail &&
+	rm -fr outdir &&
+	git-format-patch --cover-letter -2 -o outdir &&
+	test_must_fail git send-email \
+	  --from="Example <nobody@example.com>" \
+	  --to=nobody@example.com \
+	  --smtp-server="$(pwd)/fake.sendmail" \
+	  outdir/0002-*.patch \
+	  outdir/0000-*.patch \
+	  outdir/0001-*.patch \
+          2>errors >out &&
+	grep "SUBJECT HERE" errors &&
+	test -z "$(ls msgtxt*)"
+'
+
+test_expect_success '--force sends cover letter template anyway' '
+	clean_fake_sendmail &&
+	rm -fr outdir &&
+	git-format-patch --cover-letter -2 -o outdir &&
+	git send-email \
+	  --force \
+	  --from="Example <nobody@example.com>" \
+	  --to=nobody@example.com \
+	  --smtp-server="$(pwd)/fake.sendmail" \
+	  outdir/0002-*.patch \
+	  outdir/0000-*.patch \
+	  outdir/0001-*.patch \
+          2>errors >out &&
+	! grep "SUBJECT HERE" errors &&
+	test -n "$(ls msgtxt*)"
+'
+
 test_done
-- 
1.6.3.2.335.gc04d

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Every so often, someone sends out an unedited cover-letter template.
> Add a simple check to send-email that refuses to send if the subject
> contains "*** SUBJECT HERE ***", with an option --force to override.

Good ;-).  More valuable to detect would be an empty "blurb" section
(i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
nothing said in the message other than the shortstat).

One twist is that I am reasonably sure that somebody will soon fix the
shortstat part of the cover letter with something more reasonable
(i.e. not categorized by author, but instead retaining the order of
patches, and for an added bonus, append names of authors other than
yourself at the end of the line in parentheses, or something).

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Thomas Rast]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Every so often, someone sends out an unedited cover-letter template.
> > Add a simple check to send-email that refuses to send if the subject
> > contains "*** SUBJECT HERE ***", with an option --force to override.
> 
> Good ;-).  More valuable to detect would be an empty "blurb" section
> (i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
> nothing said in the message other than the shortstat).

Wouldn't that be AI complete?  As in, whatever regex (or similar) you
might cook up could probably be matched by *someone's* style of patch
descriptions, and then it suddenly becomes rather hard to tell them
apart.

I was really only trying to prevent the case where the user formatted
several times in a row because he noticed mistakes in the patches, and
forgot to re-edit the cover letter.  At least that's how I once sent
out one with a real description but the template subject.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Thomas Rast]

[-- Attachment #1: Type: Text/Plain, Size: 1516 bytes --]

Thomas Rast wrote:
> Junio C Hamano wrote:
> > Thomas Rast <trast@student.ethz.ch> writes:
> > 
> > > Every so often, someone sends out an unedited cover-letter template.
> > > Add a simple check to send-email that refuses to send if the subject
> > > contains "*** SUBJECT HERE ***", with an option --force to override.
> > 
> > Good ;-).  More valuable to detect would be an empty "blurb" section
> > (i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
> > nothing said in the message other than the shortstat).
> 
> Wouldn't that be AI complete?  As in, whatever regex (or similar) you
> might cook up could probably be matched by *someone's* style of patch
> descriptions, and then it suddenly becomes rather hard to tell them
> apart.

For a contrived example, compare

-- 8< --
Thomas Rast (1):
  Documentation: teach stash/pop workflow instead of stash/apply
-- >8 --

and

-- 8< --
Patch (1):
  Some explanation of what patch number 1 does
-- >8 --

Except for the " (1):", the whole contents of the shortstat are
given by the patches.  So I don't think it's possible to tell them
apart without figuring out what the shortlog would have said for the
actual patches, and comparing that to the contents.  I'd rather not
parse the patches for author info and subject.

What do you want me to do about the patch?  Should I extend it so that
it traps both "*** BLURB HERE ***" and "*** SUBJECT HERE ***"?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Thomas Rast wrote:
>> Junio C Hamano wrote:
>> > Thomas Rast <trast@student.ethz.ch> writes:
>> > 
>> > > Every so often, someone sends out an unedited cover-letter template.
>> > > Add a simple check to send-email that refuses to send if the subject
>> > > contains "*** SUBJECT HERE ***", with an option --force to override.
>> > 
>> > Good ;-).  More valuable to detect would be an empty "blurb" section
>> > (i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
>> > nothing said in the message other than the shortstat).
> ...
> What do you want me to do about the patch?  Should I extend it so that
> it traps both "*** BLURB HERE ***" and "*** SUBJECT HERE ***"?

Oh, nothing.

I did not mean to say "if you do not add this unrelated thing, your patch
is not acceptable."  Please take my "oh I wish there is another feature to
do this similar but different thing" as a hint for possible follow-up
patches, and they do not necessarily have to be done by you but can be
done by anybody, or for that matter they do not have to be done by anybody
like this particular case where it turns out to be "why bother?  it cannot
be done sensibly and usefully".

Thanks, and sorry.  I'll try to be more explicit next time.

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Markus Heidelberg]

Junio C Hamano, 09.06.2009:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Every so often, someone sends out an unedited cover-letter template.
> > Add a simple check to send-email that refuses to send if the subject
> > contains "*** SUBJECT HERE ***", with an option --force to override.
> 
> Good ;-).  More valuable to detect would be an empty "blurb" section
> (i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
> nothing said in the message other than the shortstat).

My mail "[PATCH 0/6] fixes for git-send-email" had an empty blurb
section because it didn't need any additional description.
I wouldn't want git to refuse sending it.

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

>> Good ;-).  More valuable to detect would be an empty "blurb" section
>> (i.e. not "unedited *** BLURB HERE ***" string, but literally, there is
>> nothing said in the message other than the shortstat).
>
> My mail "[PATCH 0/6] fixes for git-send-email" had an empty blurb
> section because it didn't need any additional description.
> I wouldn't want git to refuse sending it.

I actually wouldn't have wanted to see that [0/6] with empty description.

If the list of 6 patch titles give clear enough description of the theme
of the whole series, you would not need [0/6] to set the stage, and that
is the best kind of series.  For your send-email patches, you certainly
chose very good titles for them.

Re: [PATCH] send-email: Refuse to send cover-letter template subject

[Markus Heidelberg]

Junio C Hamano, 09.06.2009:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> >
> > My mail "[PATCH 0/6] fixes for git-send-email" had an empty blurb
> > section because it didn't need any additional description.
> > I wouldn't want git to refuse sending it.
> 
> I actually wouldn't have wanted to see that [0/6] with empty description.

I thought the diffstat is a nice overview as well. So there is at least
some value in a blurb-less cover letter. But I guess they are debatable.

> If the list of 6 patch titles give clear enough description of the theme
> of the whole series, you would not need [0/6] to set the stage,

I prefer --thread --no-chain-reply-to for more than 2 patches, so it
seems I sometimes only abuse the 0/X as a common anchor for 1..X/X. I'd
find it strange to use 1/X for that.

> and that is the best kind of series.

Hmm, so then you prefer no 0/X at all.