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

[Dominique Martinet <asmadeus@codewreck.org>]

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