format-patch base-commit: moving to above the patch?

format-patch base-commit: moving to above the patch?

[Josh Triplett]

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?

Re: format-patch base-commit: moving to above the patch?

[Junio C Hamano]

Josh Triplett <josh@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.  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.

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.


 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);

Re: format-patch base-commit: moving to above the patch?

[Josh Triplett]

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