{"thread":{"id":"64701","subject":"[RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","startedAt":"2025-12-31T03:50:19Z","lastAt":"2026-02-01T08:39:28Z","messageCount":6,"participants":["Dominique Martinet","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532860","messageId":"20251231034217.2498648-1-asmadeus@codewreck.org","threadId":"64701","inReplyTo":null,"subject":"[RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Dominique Martinet","fromEmail":"asmadeus@codewreck.org","sentAt":"2025-12-31T03:42:17Z","receivedAt":"2025-12-31T03:50:19Z","isPatch":true,"sender":{"key":"asmadeus@codewreck.org","avatar":"https://gravatar.com/avatar/aabdbc6cf0e33511e534f52bb700932d3f5b7b53093e45dfbe9e123e114ee807?d=mp&s=160"},"body":"Git format-patch historically silently ignores merge commits, because\na merge commit simply cannot be fully described by a simple patch.\n\nThis can be surprising for users, especially when coupled with newer\nmerge-heavy workflows such as encouraged by jj: trying to generate a\npatch from a jj commit that was a git merge with \"content\" will not\ngenerate anything, without any message.\n\nThis RFC patch illustrates how we could easily print a warning, but\nperhaps the warning would only make sense if no other commit has been\nformatted?\nI don't think it hurts all that much to print all the time but I can see\nit being annoying in some use-cases, so it'd likely deserve a config\nknob if we inconditionally print that...\nAlso perhaps pretty-printing the merge commit a bit better like printing\nthe subject...\n\nPlease let me know what you think would make sense here and I'll send a\nmore proper patch (tests..)\nThanks!\n\nReported-by: Julien Moutinho \nSigned-off-by: Dominique Martinet \n---\n builtin/log.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d4cf9c59c81a..b21274461cd3 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2044,7 +2044,6 @@ int cmd_format_patch(int argc,\n \trev.expand_tabs_in_log_default = 0;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n-\trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.no_free = 1;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n@@ -2274,6 +2273,11 @@ int cmd_format_patch(int argc,\n \t\tdie(_(\"revision walk setup failed\"));\n \trev.boundary = 1;\n \twhile ((commit = get_revision(&rev)) != NULL) {\n+\t\tif (commit->parents->next) {\n+\t\t\twarning(_(\"skipped merge commit %s\"),\n+\t\t\t\toid_to_hex(&commit->object.oid));\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (commit->object.flags & BOUNDARY) {\n \t\t\tboundary_count++;\n \t\t\torigin = (boundary_count == 1) ? commit : NULL;\n-- \n2.52.0\n\n"},{"id":"532861","messageId":"xmqqo6nfdyl4.fsf@gitster.g","threadId":"64701","inReplyTo":"20251231034217.2498648-1-asmadeus@codewreck.org","subject":"Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-31T05:12:07Z","receivedAt":"2025-12-31T05:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dominique Martinet writes:\n\n> This RFC patch illustrates how we could easily print a warning, but\n> perhaps the warning would only make sense if no other commit has been\n> formatted?\n\nYeah, when nothing is shown but the given range is not empty, it\nwould not be too annoying to give an advice message.\n\nOn the other hand, I do not think it is a good idea to say anything\nextra when the user gave a range \"trunk..mytopic\" that has repeated\nback-merges from trunk into mytopic, to format what s/he worked on\nthe mytopic branch. They _expect_ these back-merges to be ignored,\nand it would be purely an unwanted noise.\n\nThanks.\n"},{"id":"532903","messageId":"20260102073358.GC2581074@coredump.intra.peff.net","threadId":"64701","inReplyTo":"20251231034217.2498648-1-asmadeus@codewreck.org","subject":"Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-02T07:33:58Z","receivedAt":"2026-01-02T07:33:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 31, 2025 at 12:42:17PM +0900, Dominique Martinet wrote:\n\n> @@ -2274,6 +2273,11 @@ int cmd_format_patch(int argc,\n> \t\tdie(_(\"revision walk setup failed\"));\n> \trev.boundary = 1;\n> \twhile ((commit = get_revision(&rev)) != NULL) {\n> +\t\tif (commit->parents->next) {\n> +\t\t\twarning(_(\"skipped merge commit %s\"),\n> +\t\t\t\toid_to_hex(&commit->object.oid));\n> +\t\t\tcontinue;\n> +\t\t}\n\nI don't have any thoughts on whether the patch is overall a good\ndirection or not, but I suspect this line will segfault if we ever see a\nroot commit. You probably want:\n\n if (commit->parents && commit->parents->next)\n\nhere.\n\n-Peff\n"},{"id":"532951","messageId":"aVkKmcER2K8D9U4T@codewreck.org","threadId":"64701","inReplyTo":"20260102073358.GC2581074@coredump.intra.peff.net","subject":"Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Dominique Martinet","fromEmail":"asmadeus@codewreck.org","sentAt":"2026-01-03T12:24:57Z","receivedAt":"2026-01-03T12:25:24Z","isPatch":true,"sender":{"key":"asmadeus@codewreck.org","avatar":"https://gravatar.com/avatar/aabdbc6cf0e33511e534f52bb700932d3f5b7b53093e45dfbe9e123e114ee807?d=mp&s=160"},"body":"Thank you both for taking the time to reply over new year\n\nJunio C Hamano wrote on Wed, Dec 31, 2025 at 02:12:07PM +0900:\n> Dominique Martinet writes:\n> > This RFC patch illustrates how we could easily print a warning, but\n> > perhaps the warning would only make sense if no other commit has been\n> > formatted?\n> \n> Yeah, when nothing is shown but the given range is not empty, it\n> would not be too annoying to give an advice message.\n> \n> On the other hand, I do not think it is a good idea to say anything\n> extra when the user gave a range \"trunk..mytopic\" that has repeated\n> back-merges from trunk into mytopic, to format what s/he worked on\n> the mytopic branch. They _expect_ these back-merges to be ignored,\n> and it would be purely an unwanted noise.\n\nOkay, I can see this being confusing to people not used to format-patch\neven with a range, but I agree it'll be annoying more often than not in\ngeneral so I'm fine with this.\n\nIt makes it a bit cumbersome to print details about the commit(s) being\nskipped though, so it's probably simpler to do a generic message like\n\"No patch generated. Note merge commits are skipped.\" like this?\n-----\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d4cf9c59c81a..1ff3c5a4c960 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1901,6 +1901,7 @@ int cmd_format_patch(int argc,\n \tchar *to_free = NULL;\n \tstruct setup_revision_opt s_r_opt;\n \tsize_t nr = 0, total, i;\n+\tint seen_merge = 0;\n \tint use_stdout = 0;\n \tint start_number = -1;\n \tint just_numbers = 0;\n@@ -2044,7 +2045,6 @@ int cmd_format_patch(int argc,\n \trev.expand_tabs_in_log_default = 0;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n-\trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.no_free = 1;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n@@ -2274,6 +2274,10 @@ int cmd_format_patch(int argc,\n \t\tdie(_(\"revision walk setup failed\"));\n \trev.boundary = 1;\n \twhile ((commit = get_revision(&rev)) != NULL) {\n+\t\tif (commit->parent && commit->parents->next) {\n+\t\t\tseen_merge = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (commit->object.flags & BOUNDARY) {\n \t\t\tboundary_count++;\n \t\t\torigin = (boundary_count == 1) ? commit : NULL;\n@@ -2287,9 +2291,12 @@ int cmd_format_patch(int argc,\n \t\tREALLOC_ARRAY(list, nr);\n \t\tlist[nr - 1] = commit;\n \t}\n-\tif (nr == 0)\n+\tif (nr == 0) {\n+\t\tif (seen_merge)\n+\t\t\twarning(_(\"No patch generated. Note merge commits are skipped.\"));\n \t\t/* nothing to do */\n \t\tgoto done;\n+\t}\n \ttotal = nr;\n \tif (cover_letter == -1) {\n \t\tif (cfg.config_cover_letter == COVER_AUTO)\n-----\n\nI'll send that as v2 with tests fixed/added when time permits.\n\n\nJeff King wrote on Fri, Jan 02, 2026 at 02:33:58AM -0500:\n> On Wed, Dec 31, 2025 at 12:42:17PM +0900, Dominique Martinet wrote:\n> \n> > @@ -2274,6 +2273,11 @@ int cmd_format_patch(int argc,\n> > \t\tdie(_(\"revision walk setup failed\"));\n> > \trev.boundary = 1;\n> > \twhile ((commit = get_revision(&rev)) != NULL) {\n> > +\t\tif (commit->parents->next) {\n> > +\t\t\twarning(_(\"skipped merge commit %s\"),\n> > +\t\t\t\toid_to_hex(&commit->object.oid));\n> > +\t\t\tcontinue;\n> > +\t\t}\n> \n> I don't have any thoughts on whether the patch is overall a good\n> direction or not, but I suspect this line will segfault if we ever see a\n> root commit. You probably want:\n> \n> if (commit->parents && commit->parents->next)\n\nThank you for pointing that out!\n\n-- \nDominique Martinet | Asmadeus\n"},{"id":"532962","messageId":"xmqqy0mep0y2.fsf@gitster.g","threadId":"64701","inReplyTo":"aVkKmcER2K8D9U4T@codewreck.org","subject":"Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-04T02:27:01Z","receivedAt":"2026-01-04T02:27:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dominique Martinet writes:\n\n> Okay, I can see this being confusing to people not used to format-patch\n> even with a range, but I agree it'll be annoying more often than not in\n> general so I'm fine with this.\n\nYup, nobody stays to be newbie forever ;-).\n\n> It makes it a bit cumbersome to print details about the commit(s) being\n> skipped though, so it's probably simpler to do a generic message like\n> \"No patch generated. Note merge commits are skipped.\" like this?\n\nOr queue these merge commits in another commit list instead of a\nsingle boolean \"seen_merge\". The warning is issued only on the\nerror path, so as long as accumulation phase is cheap enough to\nrecord information necessary to later create detailed messages, the\nlocation you added a single warning() call can call a new helper\nfunction that gives more details like commit log messages, etc., if\nwe wanted to. Or seen_merge can become a counter and the warning\nmessage can become a simpler \"skipped %d merges\".\n"},{"id":"534939","messageId":"aX8RIq0ZUSoIue8G@codewreck.org","threadId":"64701","inReplyTo":"xmqqy0mep0y2.fsf@gitster.g","subject":"Re: [RFC PATCH] builtin/format-patch: print a warning for skipped merge commits?","fromName":"Dominique Martinet","fromEmail":"asmadeus@codewreck.org","sentAt":"2026-02-01T08:38:58Z","receivedAt":"2026-02-01T08:39:28Z","isPatch":true,"sender":{"key":"asmadeus@codewreck.org","avatar":"https://gravatar.com/avatar/aabdbc6cf0e33511e534f52bb700932d3f5b7b53093e45dfbe9e123e114ee807?d=mp&s=160"},"body":"(It took me a while to get back to this...)\n\nJunio C Hamano wrote on Sun, Jan 04, 2026 at 11:27:01AM +0900:\n> Dominique Martinet writes:\n> > Okay, I can see this being confusing to people not used to format-patch\n> > even with a range, but I agree it'll be annoying more often than not in\n> > general so I'm fine with this.\n> \n> Yup, nobody stays to be newbie forever ;-).\n\nI think I actually fell in this newbie category myself: I hadn't\nrealized how hard it is to specify exactly only a merge commit in normal\ngit usage.\n\nIn \"confusing case\" I was consulted about, Julien had used jj to\n(involuntarily) make a merge commit that merged two parent commits like\nthis:\n$ jj log\n○ mkwklvul Julien Moutinho 1 hour ago openvpn* 0ef9a661\n│ nixos/openvpn: format with nixfmt-rfc-style\n@ qsusmwyp Julien Moutinho 1 hour ago 14ef78b3\n├─╮ nixos/openvpn: add netns support\n○ │ vltlsmty Julien Moutinho 23 hours ago services.netns git_head() 204ac248\n├─╯ nixos/netns: init module to manage network namespaces\n◆ ktsspxvy Yohann Boniface 1 day ago master@NixOS 04245c47\n│ (empty) arduino-cli: 1.3.1 -> 1.4.0, use finalAttrs (#469365)\n\nSo specifying `git format-patch 14ef78b3^-` wouldn't generate any\ncommit;\nbut in the normal merge case using foo^- will grab commits that are in\nthe second parent that aren't in the first, so to get only the merge\ncommit you'd need to write `foo^- --not foo^2` or some other more\ncomplex expression...\nAt which point I'm not sure this warning has much benefit, and the real\nproblem would more be that jj let him create such an useless merge\ncommit with non-trivial content..\n\n\nIf it was just very rarely useful I might still be tempted to send the\npatch, but it also breaks patch counting with e.g. `git format-patch -3`\n(test t4014-format-patch.sh \"format-patch doesn't consider merge\ncommits\"):\nthe -3 is done by common revision `.max_count` limit, so dropping\n`rev.max_parents = 1` makes format-patch skip through merge commits\nwithout re-adding further commits, and I'm not convinced this is all\nworth it.\n\nSo, thank you for the quick replies (over new year festivities no\nless!), but let's leave it at this unless something compelling comes\nup...\n\n\nThanks,\n-- \nDominique Martinet | Asmadeus\n"}]}