send-email: clarify missing subject error - #2375

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Explain the required Subject: prefix when a message file has no subject.

Yes, get_patch_subject() is called from places where the caller
wants to determine whether the given file has at least one line that
begins with 'Subject:'.  In that case, the subroutine returns the
rest of that line; if it does not find any such line, it issues an
error message and dies.

As a side note, the check is curiously case-sensitive.  It is also
curious that the scanning does not stop at the first blank line that
terminates the email headers.  However, do not change this behavior
without studying the possible ramifications.  People have learned to
use various inputs that are not exactly what is output by 'git
format-patch', so rejecting a 'malformed' file simply because it
differs from typical 'git format-patch' output will break someone's
established workflow.

> Terminate the error with a newline so Perl does not append its internal
> source location.

Very nice spotting.  For an end-user facing script, the source
location is not useful.  The user is not debugging and fixing the
send-email script after all ;-).

> diff --git a/git-send-email.perl b/git-send-email.perl
> index bb8ddd1eef..4d76d53c49 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -863,7 +863,8 @@ sub get_patch_subject {
>  		return "GIT: $1\n";
>  	}
>  	close $fh;
> -	die sprintf(__("No subject line in %s?"), $fn);
> +	die sprintf(__("No subject line in %s. " .
> +		"The first line must start with \"Subject: \"\n"), $fn);
>  }

An input file to the 'git send-email' program is often the output
of 'git format-patch'.  Such a file begins with a UNIX 'From '
line, followed by email headers such as 'From:', 'Date:', and
'Subject:'.  The 'Subject:' line cannot be the first line of
the file in this case, yet it is a valid input.

The only condition that this subroutine flags as an error is when
the file lacks a subject line.  "No 'Subject:' line in '%s'\n" is a
clear message to display and is an improvement over the original.

However, the fact that the first line does not start with
"Subject:" is irrelevant to the basis of the subroutine's
decision to issue an error, I think.

Thanks.

> An input file to the 'git send-email' program is often the output
> of 'git format-patch'.  Such a file begins with a UNIX 'From '
> line, followed by email headers such as 'From:', 'Date:', and
> 'Subject:'.  The 'Subject:' line cannot be the first line of
> the file in this case, yet it is a valid input.
>
> The only condition that this subroutine flags as an error is when
> the file lacks a subject line.  "No 'Subject:' line in '%s'\n" is a
> clear message to display and is an improvement over the original.
>
> However, the fact that the first line does not start with
> "Subject:" is irrelevant to the basis of the subroutine's
> decision to issue an error, I think.

Yeah, that makes sense, so maybe we don't need to focus on it being
the first line, but Subject needs to be there somewhere before the
body.


Harald

Harald Nordgren <haraldnordgren@gmail.com> writes:

>> An input file to the 'git send-email' program is often the output
>> of 'git format-patch'.  Such a file begins with a UNIX 'From '
>> line, followed by email headers such as 'From:', 'Date:', and
>> 'Subject:'.  The 'Subject:' line cannot be the first line of
>> the file in this case, yet it is a valid input.
>>
>> The only condition that this subroutine flags as an error is when
>> the file lacks a subject line.  "No 'Subject:' line in '%s'\n" is a
>> clear message to display and is an improvement over the original.
>>
>> However, the fact that the first line does not start with
>> "Subject:" is irrelevant to the basis of the subroutine's
>> decision to issue an error, I think.
>
> Yeah, that makes sense, so maybe we don't need to focus on it being
> the first line, but Subject needs to be there somewhere before the
> body.

Yeah, the curious thing is that the subroutine with the loop is
happy as long as it finds "^Subject: " somewhere, not necessarily
before the first blank line.

That is why I said "No 'Subject: ' line in '%s'\n" is clear enough
and an improvement over the original.  Anything else will add lie to
it.

Thanks.

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_expect_success $PREREQ 'missing subject omits Perl location' '
> +	cat >no-subject.patch <<-\EOF &&
> +	This is the body.
> +	EOF
> +	test_must_fail git send-email \
> +		--dry-run \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		no-subject.patch 2>actual &&
> +	cat >expect <<-\EOF &&
> +	No '\''Subject:'\'' line in '\''no-subject.patch'\''
> +	EOF

OK.  We require the message to exactly be this one (not starting
with this substring), which makes sure we are not getting the line
numbers from die.  Good.

Will queue.  This round looks perfect.  Let's mark it for 'next'.

Thanks.