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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2016, 18:34 UTC
Message-ID: <xmqqshtags0o.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160908011200.qzvbdt4wjwiji4h5@x>

Josh Triplett <josh@joshtriplett.org> writes:

Show 15 quoted lines

> 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.

Show 13 quoted lines

> 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.