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

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

[Dominique Martinet]

(It took me a while to get back to this...)

Junio C Hamano wrote on Sun, Jan 04, 2026 at 11:27:01AM +0900:
> 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