[RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?

[RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?

[Dominique Martinet]

Git format-patch historically silently ignores merge commits, because
a merge commit simply cannot be fully described by a simple patch.

This can be surprising for users, especially when coupled with newer
merge-heavy workflows such as encouraged by jj: trying to generate a
patch from a jj commit that was a git merge with "content" will not
generate anything, without any message.

This RFC patch illustrates how we could easily print a warning, but
perhaps the warning would only make sense if no other commit has been
formatted?
I don't think it hurts all that much to print all the time but I can see
it being annoying in some use-cases, so it'd likely deserve a config
knob if we inconditionally print that...
Also perhaps pretty-printing the merge commit a bit better like printing
the subject...

Please let me know what you think would make sense here and I'll send a
more proper patch (tests..)
Thanks!

Reported-by: Julien Moutinho <julm@sourcephile.fr>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 builtin/log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index d4cf9c59c81a..b21274461cd3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2044,7 +2044,6 @@ int cmd_format_patch(int argc,
 	rev.expand_tabs_in_log_default = 0;
 	rev.verbose_header = 1;
 	rev.diff = 1;
-	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
 	rev.diffopt.no_free = 1;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
@@ -2274,6 +2273,11 @@ int cmd_format_patch(int argc,
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
 	while ((commit = get_revision(&rev)) != NULL) {
+		if (commit->parents->next) {
+			warning(_("skipped merge commit %s"),
+				oid_to_hex(&commit->object.oid));
+			continue;
+		}
 		if (commit->object.flags & BOUNDARY) {
 			boundary_count++;
 			origin = (boundary_count == 1) ? commit : NULL;
-- 
2.52.0

Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?

[Junio C Hamano]

Dominique Martinet <asmadeus@codewreck.org> writes:

> This RFC patch illustrates how we could easily print a warning, but
> perhaps the warning would only make sense if no other commit has been
> formatted?

Yeah, when nothing is shown but the given range is not empty, it
would not be too annoying to give an advice message.

On the other hand, I do not think it is a good idea to say anything
extra when the user gave a range "trunk..mytopic" that has repeated
back-merges from trunk into mytopic, to format what s/he worked on
the mytopic branch.  They _expect_ these back-merges to be ignored,
and it would be purely an unwanted noise.

Thanks.