threads / rfc / 64701

RFC patchbuiltin/format-patch: print a warning for skipped merge commits?

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

## tl;dr

6 messages between Dec 31, 2025 and Feb 1, 2026. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Dominique Martinet· Dec 31, 2025, 03:42 UTC · lore

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(-)
Show changes to builtin/log.c +5 −1
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
Junio C Hamano· Dec 31, 2025, 05:12 UTC · re: Dominique Martinet · lore

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

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.
Jeff King· Jan 2, 2026, 07:33 UTC · re: Dominique Martinet · lore

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

On Wed, Dec 31, 2025 at 12:42:17PM +0900, Dominique Martinet wrote:
Show 9 quoted lines
> @@ -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
Dominique Martinet· Jan 3, 2026, 12:24 UTC · re: Jeff King · lore

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

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:
Show 13 quoted lines
> 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? -----

Show changes to builtin/log.c +9 −2
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
Junio C Hamano· Jan 4, 2026, 02:27 UTC · re: Dominique Martinet · lore

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

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

Dominique Martinet· Feb 1, 2026, 08:38 UTC · re: Junio C Hamano · lore

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

(It took me a while to get back to this...)
Junio C Hamano wrote on Sun, Jan 04, 2026 at 11:27:01AM +0900:
Show 6 quoted lines
> 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 ;-).

I think I actually fell in this newbie category myself: I hadn't realized how hard it is to specify exactly only a merge commit in normal git usage.

In "confusing case" I was consulted about, Julien had used jj to (involuntarily) make a merge commit that merged two parent commits like this: $ jj log ○ mkwklvul Julien Moutinho 1 hour ago openvpn* 0ef9a661 │ nixos/openvpn: format with nixfmt-rfc-style @ qsusmwyp Julien Moutinho 1 hour ago 14ef78b3 ├─╮ nixos/openvpn: add netns support ○ │ vltlsmty Julien Moutinho 23 hours ago services.netns git_head() 204ac248 ├─╯ nixos/netns: init module to manage network namespaces ◆ ktsspxvy Yohann Boniface 1 day ago master@NixOS 04245c47 │ (empty) arduino-cli: 1.3.1 -> 1.4.0, use finalAttrs (#469365)

So specifying `git format-patch 14ef78b3^-` wouldn't generate any commit; but in the normal merge case using foo^- will grab commits that are in the second parent that aren't in the first, so to get only the merge commit you'd need to write `foo^- --not foo^2` or some other more complex expression... At which point I'm not sure this warning has much benefit, and the real problem would more be that jj let him create such an useless merge commit with non-trivial content..

If it was just very rarely useful I might still be tempted to send the patch, but it also breaks patch counting with e.g. `git format-patch -3` (test t4014-format-patch.sh "format-patch doesn't consider merge commits"): the -3 is done by common revision `.max_count` limit, so dropping `rev.max_parents = 1` makes format-patch skip through merge commits without re-adding further commits, and I'm not convinced this is all worth it.

So, thank you for the quick replies (over new year festivities no less!), but let's leave it at this unless something compelling comes up...

Thanks,
-- 
Dominique Martinet | Asmadeus

← back to recent threads