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

[Josh Triplett <josh@joshtriplett.org>]

On Thu, Sep 08, 2016 at 11:34:15AM -0700, Junio C Hamano wrote:
> 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,

Ah, true, I should have titled it "format-patch: move base commit ...".

> your problem description
> looks perfect.  I am still not sure if the code does a reasonable
> thing in MIME case, though.

It *looks* correct to me.

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

One of the git tests already did that.  I just modified that test to
test the exact signature format and that it appears at the end, rather
than just grepping to check that the signature string appears somewhere.
Then when doing so, I realized that I should check the default case too
(at which point that test change probably should have gone in a separate
patch).

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

If any other change ends up being necessary, I'll split the patch in v2.

- Josh Triplett