On Wed, Sep 07, 2016 at 09:06:31AM -0700, Junio C Hamano wrote:
> Josh Triplett <j...@joshtriplett.org> 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);

Reply via email to