On Wed, Sep 07, 2016 at 09:06:31AM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@xxxxxxxxxxxxxxxx> writes:
>
> > Currently, format-patch puts base-commit and prerequisite-patch-id
> > information below the patch, and below the email signature. Most mail
> > clients automatically trim everything below the signature marker as
> > unimportant when quoting a mail for a reply, which would make it
> > difficult for someone to reply, quote the base-commit, and say something
> > like "I don't have this commit, where did it come from?" or "Can you
> > please rebase this on ...".
> >
> > Might it make sense to move this information adjacent to the diffstat,
> > instead? Or, at least, above the email signature?
>
> I personally feel that it would be annoying to have them near
> diffstat, especially given that unbounded many prereq patches can be
> listed. It would not be too bad to flip the order between the call
> to print_signature() and print_bases(), though.
I can live with that; having it above the signature was a much bigger
concern for me than moving it above the patch.
> The extent of the change needed to (note: not even compile-tested)
> does does not look too bad, either.
>
> I did not carefully think what the right adjustment for the MIME
> case is, though.
Seems plausible to me.
> I would expect some tests that expect the current order of the tail
> end of the output to break, which you would need to adjust. And if
> there is no such test right now, you should add one, as your inquiry
> and this patch _sets_ a concrete expectation as to what should come
> before the signature line, which future updates should not break.
I can do that; arguably we should also have a test that nothing *except*
the git version appears after the signature line.
>
> builtin/log.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 92dc34d..d69d5e6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> diff_flush(&opts);
>
> fprintf(rev->diffopt.file, "\n");
> - print_signature(rev->diffopt.file);
> }
>
> static const char *clean_message_id(const char *msg_id)
> @@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> make_cover_letter(&rev, use_stdout,
> origin, nr, list, branch_name, quiet);
> print_bases(&bases, rev.diffopt.file);
> + print_signature(rev.diffopt.file);
> total++;
> start_number--;
> }
> @@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> if (!use_stdout)
> rev.shown_one = 0;
> if (shown) {
> + print_bases(&bases, rev.diffopt.file);
> if (rev.mime_boundary)
> fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
> mime_boundary_leader,
> rev.mime_boundary);
> else
> print_signature(rev.diffopt.file);
> - print_bases(&bases, rev.diffopt.file);
> }
> if (!use_stdout)
> fclose(rev.diffopt.file);