Re: [PATCH] Move format-patch base commit and prerequisites before email signature

[Junio C Hamano <gitster@pobox.com>]

Josh Triplett <josh@joshtriplett.org> writes:

> Any text below the "-- " for the email signature gets treated as part of
> the signature, and many mail clients will trim it from the quoted text
> for a reply.  Move it above the signature, so people can reply to it
> more easily.
>
> Add tests for the exact format of the email signature, and add tests to
> ensure the email signature appears last.
>
> (Patch by Junio Hamano; tests by Josh Triplett.)
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>
> Does the above seem reasonable, for a patch that incorporates the
> proposed patch from Message-Id
> xmqqh99rpud4.fsf@gitster.mtv.corp.google.com and adds tests?

Other than that I'd probably retitle it, your problem description
looks perfect.  I am still not sure if the code does a reasonable
thing in MIME case, though.

Thanks for tying the loose ends anyway.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b0579dd..a4af275 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
>  	git format-patch --ignore-if-in-upstream HEAD
>  '
>  
> +git_version="$(git --version | sed "s/.* //")"
> +
> +signature() {
> +	printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
> +}

Hmph.  I would actually have expected that you would force a fixed
and an easily noticeable string via format.signature for the purpose
of the test, but I guess this test covers a lot more than what the
purpose of the main part of the patch does (i.e. enforces that the
default signature must be made from the version string of Git).  It
is not a bad thing to test, but it probably does not belong to this
change.  If you _were_ to split the patch in two, that is where I
probably would split, i.e. "we didn't test what the default signature
looks like, or we didn't make sure --signature option overrides the
default signature, so let's test it" as the preliminary preparation,
followed by "having base info after sig is inconvenient, let's move
it and make sure base info stays before sig with additional test" as
the second (and primary) patch.

But a single patch is fine.

Thanks.