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

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

[Jeff King]

On Wed, Dec 31, 2025 at 12:42:17PM +0900, Dominique Martinet wrote:

> @@ -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;
> +		}

I don't have any thoughts on whether the patch is overall a good
direction or not, but I suspect this line will segfault if we ever see a
root commit. You probably want:

  if (commit->parents && commit->parents->next)

here.

-Peff

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

[Dominique Martinet]

Thank you both for taking the time to reply over new year

Junio C Hamano wrote on Wed, Dec 31, 2025 at 02:12:07PM +0900:
> 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.

Okay, I can see this being confusing to people not used to format-patch
even with a range, but I agree it'll be annoying more often than not in
general so I'm fine with this.

It makes it a bit cumbersome to print details about the commit(s) being
skipped though, so it's probably simpler to do a generic message like
"No patch generated. Note merge commits are skipped." like this?
-----
diff --git a/builtin/log.c b/builtin/log.c
index d4cf9c59c81a..1ff3c5a4c960 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1901,6 +1901,7 @@ int cmd_format_patch(int argc,
 	char *to_free = NULL;
 	struct setup_revision_opt s_r_opt;
 	size_t nr = 0, total, i;
+	int seen_merge = 0;
 	int use_stdout = 0;
 	int start_number = -1;
 	int just_numbers = 0;
@@ -2044,7 +2045,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 +2274,10 @@ int cmd_format_patch(int argc,
 		die(_("revision walk setup failed"));
 	rev.boundary = 1;
 	while ((commit = get_revision(&rev)) != NULL) {
+		if (commit->parent && commit->parents->next) {
+			seen_merge = 1;
+			continue;
+		}
 		if (commit->object.flags & BOUNDARY) {
 			boundary_count++;
 			origin = (boundary_count == 1) ? commit : NULL;
@@ -2287,9 +2291,12 @@ int cmd_format_patch(int argc,
 		REALLOC_ARRAY(list, nr);
 		list[nr - 1] = commit;
 	}
-	if (nr == 0)
+	if (nr == 0) {
+		if (seen_merge)
+			warning(_("No patch generated. Note merge commits are skipped."));
 		/* nothing to do */
 		goto done;
+	}
 	total = nr;
 	if (cover_letter == -1) {
 		if (cfg.config_cover_letter == COVER_AUTO)
-----

I'll send that as v2 with tests fixed/added when time permits.


Jeff King wrote on Fri, Jan 02, 2026 at 02:33:58AM -0500:
> On Wed, Dec 31, 2025 at 12:42:17PM +0900, Dominique Martinet wrote:
> 
> > @@ -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;
> > +		}
> 
> I don't have any thoughts on whether the patch is overall a good
> direction or not, but I suspect this line will segfault if we ever see a
> root commit. You probably want:
> 
>   if (commit->parents && commit->parents->next)

Thank you for pointing that out!

-- 
Dominique Martinet | Asmadeus

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

[Junio C Hamano]

Dominique Martinet <asmadeus@codewreck.org> writes:

> Okay, I can see this being confusing to people not used to format-patch
> even with a range, but I agree it'll be annoying more often than not in
> general so I'm fine with this.

Yup, nobody stays to be newbie forever ;-).

> It makes it a bit cumbersome to print details about the commit(s) being
> skipped though, so it's probably simpler to do a generic message like
> "No patch generated. Note merge commits are skipped." like this?

Or queue these merge commits in another commit list instead of a
single boolean "seen_merge".  The warning is issued only on the
error path, so as long as accumulation phase is cheap enough to
record information necessary to later create detailed messages, the
location you added a single warning() call can call a new helper
function that gives more details like commit log messages, etc., if
we wanted to.  Or seen_merge can become a counter and the warning
message can become a simpler "skipped %d merges".