{"thread":{"id":"62446","subject":"git format-patch escaping issues in the patch format","startedAt":"2024-11-04T19:24:24Z","lastAt":"2024-11-05T15:07:50Z","messageCount":10,"participants":["Christoph Anton Mitterer","Kristoffer Haugsbakk","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"506557","messageId":"ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org","threadId":"62446","inReplyTo":null,"subject":"git format-patch escaping issues in the patch format","fromName":"Christoph Anton Mitterer","fromEmail":"calestyo@scientia.org","sentAt":"2024-11-04T19:24:14Z","receivedAt":"2024-11-04T19:24:24Z","isPatch":false,"sender":{"key":"calestyo@scientia.org","avatar":"https://gravatar.com/avatar/2dd3cd5191e4ec4a5e1e0a0861a64378636270f4007bab080a27b27a607bb267?d=mp&s=160"},"body":"Hey there.\n\nFor some special use case, I wanted to write a parser for the patch\nformat created by git format-patch, especially where I can separate\nheaders, commit message and the actual unified diffs.\n\nThere seems unfortunately only little (written) definition of that\nformat, git-format-patch(1) merely says it's in UNIX mailbox format\n(which itself is, AFAIK, not really formally defined).\n\n\nAnyway, it seems to turn out, that no escaping is done for the commit\nmessage in the patch format and that this can cause actual breakage\nwith valid commit messages.\n\nConsider the following example:\n1. I create a fresh repo, add a test file and use a commit message,\n which contains a From line (even with the \"magic\" timestamp) and\n some made up commit id (0000...)\n\n ~/test$ git init foo; cd foo\n Initialized empty Git repository in /home/calestyo/test/foo/.git/\n ~/test/foo$ echo a >f; git add f\n ~/test/foo$ git commit -m \"msg1\n \n From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n --\n ---\"\n [master (root-commit) c08debc] msg1\n 1 file changed, 1 insertion(+)\n create mode 100644 f\n \n \n2. The format-patch for that looks already suspicious:\n - The From line is not escaped (as some variants of mbox would do,\n some properly some, causing corruption by the escaping with >\n itself).\n - What the format may think of as a separator after the commit\n message (namely the ---) cannot be used as that either, as a ---\n in the commit message is again not escaped.\n \n ~/test/foo$ git format-patch --root; cat 0001-msg1.patch; rm -f 0001-msg1.patch\n 0001-msg1.patch\n From c08debcc502c78786ec71d50686ff0445a13b654 Mon Sep 17 00:00:00 2001\n From: Christoph Anton Mitterer \n Date: Mon, 4 Nov 2024 19:58:45 +0100\n Subject: [PATCH] msg1\n \n From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n --\n ---\n ---\n f | 1 +\n 1 file changed, 1 insertion(+)\n create mode 100644 f\n \n diff --git a/f b/f\n new file mode 100644\n index 0000000..7898192\n --- /dev/null\n +++ b/f\n @@ -0,0 +1 @@\n +a\n -- \n 2.45.2\n \n \n3. Adding a 2nd commit, this time using the unified diff from the above\n patch as commit message body(!).\n \n ~/test/foo$ echo b >>f; git add f\n ~/test/foo$ git commit -m \"msg2\n \n diff --git a/f b/f\n new file mode 100644\n index 0000000..7898192\n --- /dev/null\n +++ b/f\n @@ -0,0 +1 @@\n +a\n -- \n 2.45.2\"\n [master 6bbe38c] msg2\n 1 file changed, 1 insertion(+)\n ~/test/foo$ git format-patch --root\n 0001-msg1.patch\n 0002-msg2.patch\n \n \n4. To no surprise, git itself of course knows the difference between\n commit message and actual patch, as show e.g. by the following,\n where the commit message is indented (by git):\n\n $ git log --patch | cat\n commit 6bbe38c33680239ac9767e0e5095f9f32ad41ade\n Author: Christoph Anton Mitterer \n Date: Mon Nov 4 20:00:20 2024 +0100\n \n msg2\n \n diff --git a/f b/f\n new file mode 100644\n index 0000000..7898192\n --- /dev/null\n +++ b/f\n @@ -0,0 +1 @@\n +a\n --\n 2.45.2\n \n diff --git a/f b/f\n index 7898192..422c2b7 100644\n --- a/f\n +++ b/f\n @@ -1 +1,2 @@\n a\n +b\n \n commit c08debcc502c78786ec71d50686ff0445a13b654\n Author: Christoph Anton Mitterer \n Date: Mon Nov 4 19:58:45 2024 +0100\n \n msg1\n \n From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n --\n ---\n \n diff --git a/f b/f\n new file mode 100644\n index 0000000..7898192\n --- /dev/null\n +++ b/f\n @@ -0,0 +1 @@\n +a\n \n\n5. Next I try whether git am can use the patches created above in a\n fresh repo:\n \n ~/test/foo$ cd ..; git init bar; cd bar\n Initialized empty Git repository in /home/calestyo/test/bar/.git/\n ~/test/bar$ git am ../foo/0001-msg1.patch\n Patch is empty.\n hint: When you have resolved this problem, run \"git am --continue\".\n hint: If you prefer to skip this patch, run \"git am --skip\" instead.\n hint: To record the empty patch as an empty commit, run \"git am --allow-empty\".\n hint: To restore the original branch and stop patching, run \"git am --abort\".\n hint: Disable this message with \"git config advice.mergeConflict false\"\n \n That already fails for the first patch, the reason probably being my\n From 0000...\n line in the commit message.\n \n \n6. So trying again with simply that From 000.. line removed\n \n ~/test/bar$ sed -i '/^From 00000/d' ../foo/0001-msg1.patch\n ~/test/bar$ git am ../foo/0001-msg1.patch\n fatal: previous rebase directory .git/rebase-apply still exists but mbox given.\n \n and again on a freshly created repo:\n \n ~/test/bar$ cd ..; rm -rf bar; git init bar; cd bar\n Initialized empty Git repository in /home/calestyo/test/bar/.git/\n ~/test/bar$ git am ../foo/0001-msg1.patch\n Applying: msg1\n applying to an empty history\n \n Ah, now it works, so it was indeed the (unusual but still valid commit message).\n \n \n7. Now that 0001-msg1.patch is applied, let's try the 2nd patch:\n \n ~/test/bar$ git am ../foo/0002-msg2.patch\n Applying: msg2\n error: f: already exists in index\n Patch failed at 0001 msg2\n hint: Use 'git am --show-current-patch=diff' to see the failed patch\n hint: When you have resolved this problem, run \"git am --continue\".\n hint: If you prefer to skip this patch, run \"git am --skip\" instead.\n hint: To restore the original branch and stop patching, run \"git am --abort\".\n hint: Disable this message with \"git config advice.mergeConflict false\"\n ~/test/bar$ \n \n And again, ... the reason most likely git not being able to get that\n the \"first diff\" is not actually a diff but part of the commit message.\n \n\nbtw and shamelessly off-topic question:\nAny chance that git format-patch / am will ever support keeping track\nof the branch/merge history of generated / applied patches?\nThat would be really neat.\n\n\nThanks,\nChris.\n\nPS: Not subscribed, so please keep me CCed in case you want me to read\n any possible replies :-)\n"},{"id":"506567","messageId":"305dc9f7-4bdb-40c5-92f4-7438a9ecd482@app.fastmail.com","threadId":"62446","inReplyTo":"ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-04T22:15:14Z","receivedAt":"2024-11-04T22:15:36Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Nov 4, 2024, at 20:24, Christoph Anton Mitterer wrote:\n> Hey there.\n>\n> For some special use case, I wanted to write a parser for the patch\n> format created by git format-patch, especially where I can separate\n> headers, commit message and the actual unified diffs.\n>\n> There seems unfortunately only little (written) definition of that\n> format, git-format-patch(1) merely says it's in UNIX mailbox format\n> (which itself is, AFAIK, not really formally defined).\n\nIt seems to me (totally naïve) that you would do something like\n\n1. Blank line terminates headers\n2. Then there might be some optional commit-headers which can override\n things (`From`)\n3. Commit message\n4. `---`\n5. Look for a regex `^diff` line\n • Now the indentation will tell you when it ends\n6. `^Range-diff` and `^Interdiff` can also make an appearance in this\n section\n\n> Anyway, it seems to turn out, that no escaping is done for the commit\n> message in the patch format and that this can cause actual breakage\n> with valid commit messages.\n>\n> Consider the following example:\n> 1. I create a fresh repo, add a test file and use a commit message,\n> which contains a From line (even with the \"magic\" timestamp) and\n> some made up commit id (0000...)\n>\n> ~/test$ git init foo; cd foo\n> Initialized empty Git repository in /home/calestyo/test/foo/.git/\n> ~/test/foo$ echo a >f; git add f\n> ~/test/foo$ git commit -m \"msg1\n>\n> From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n> --\n> ---\"\n> [master (root-commit) c08debc] msg1\n> 1 file changed, 1 insertion(+)\n> create mode 100644 f\n\nAt first I thought that it is a magic string for a reason. But there\ncould be an organic use-case here. Like someone who is merely\ncode-quoting the magic string. And also using code fences.\n\n Parse the magic mbox string\n\n Turns out that Git has something weird: a magic string to delimit emails\n or something. Look here:\n\n ```\n From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n ```\n\n It will look exactly like this except all those numbers in the middle.\n\n Detecting this is all we need to parse that 80-message file that Pedro\n sent us of our school project backup.\n\nThis will not apply because “Patch is empty”.\n\nBut if I change the commit message to use indentation for the\ncode block:\n\n Turns out that Git has something weird: a magic string to delimit emails\n or something. Look here:\n\n From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n\nIt does apply.\n\nAs for the `---`: it seems natural for people to use this as a section\nbreak. And I do end up needing that from time to time (maybe once a\nyear). For people who stick to ASCII (`-` times 5):\n\n Detecting this is all we need to parse that 80-message file that Pedro\n sent us of our school project backup.\n\n -----\n\n We should look into Pedro’s backup system. Why does he use email?\n\nOr `***`.\n\n> 3. Adding a 2nd commit, this time using the unified diff from the above\n> patch as commit message body(!).\n>\n> ~/test/foo$ echo b >>f; git add f\n> ~/test/foo$ git commit -m \"msg2\n>\n> diff --git a/f b/f\n> new file mode 100644\n> index 0000000..7898192\n> --- /dev/null\n> +++ b/f\n> @@ -0,0 +1 @@\n> +a\n> --\n> 2.45.2\"\n> [master 6bbe38c] msg2\n> 1 file changed, 1 insertion(+)\n> ~/test/foo$ git format-patch --root\n> 0001-msg1.patch\n> 0002-msg2.patch\n>\n>\n> 4. To no surprise, git itself of course knows the difference between\n> commit message and actual patch, as show e.g. by the following,\n> where the commit message is indented (by git):\n\nThis has happened out in the wild before.[1] And the advice was the same\nas what you’re doing here.\n\n🔗 1: https://lore.kernel.org/git/16297305.cDA1TJNmNo@earendil/#t\n\n>\n> $ git log --patch | cat\n> commit 6bbe38c33680239ac9767e0e5095f9f32ad41ade\n> Author: Christoph Anton Mitterer \n> Date: Mon Nov 4 20:00:20 2024 +0100\n>\n> msg2\n>\n> diff --git a/f b/f\n> new file mode 100644\n> index 0000000..7898192\n> --- /dev/null\n> +++ b/f\n> @@ -0,0 +1 @@\n> +a\n> --\n> 2.45.2\n>\n> diff --git a/f b/f\n> index 7898192..422c2b7 100644\n> --- a/f\n> +++ b/f\n> @@ -1 +1,2 @@\n> a\n> +b\n>\n> commit c08debcc502c78786ec71d50686ff0445a13b654\n> Author: Christoph Anton Mitterer \n> Date: Mon Nov 4 19:58:45 2024 +0100\n>\n> msg1\n>\n> From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n> --\n> ---\n>\n> diff --git a/f b/f\n> new file mode 100644\n> index 0000000..7898192\n> --- /dev/null\n> +++ b/f\n> @@ -0,0 +1 @@\n> +a\n>\n>\n> 5. Next I try whether git am can use the patches created above in a\n> fresh repo:\n>\n> ~/test/foo$ cd ..; git init bar; cd bar\n> Initialized empty Git repository in /home/calestyo/test/bar/.git/\n> ~/test/bar$ git am ../foo/0001-msg1.patch\n> Patch is empty.\n> hint: When you have resolved this problem, run \"git am --continue\".\n> hint: If you prefer to skip this patch, run \"git am --skip\" instead.\n> hint: To record the empty patch as an empty commit, run \"git am\n> --allow-empty\".\n> hint: To restore the original branch and stop patching, run \"git am\n> --abort\".\n> hint: Disable this message with \"git config advice.mergeConflict\n> false\"\n>\n> That already fails for the first patch, the reason probably being my\n> From 0000...\n> line in the commit message.\n>\n>\n> 6. So trying again with simply that From 000.. line removed\n>\n> ~/test/bar$ sed -i '/^From 00000/d' ../foo/0001-msg1.patch\n> ~/test/bar$ git am ../foo/0001-msg1.patch\n> fatal: previous rebase directory .git/rebase-apply still exists but\n> mbox given.\n>\n> and again on a freshly created repo:\n>\n> ~/test/bar$ cd ..; rm -rf bar; git init bar; cd bar\n> Initialized empty Git repository in /home/calestyo/test/bar/.git/\n> ~/test/bar$ git am ../foo/0001-msg1.patch\n> Applying: msg1\n> applying to an empty history\n>\n> Ah, now it works, so it was indeed the (unusual but still valid\n> commit message).\n>\n>\n> 7. Now that 0001-msg1.patch is applied, let's try the 2nd patch:\n>\n> ~/test/bar$ git am ../foo/0002-msg2.patch\n> Applying: msg2\n> error: f: already exists in index\n> Patch failed at 0001 msg2\n> hint: Use 'git am --show-current-patch=diff' to see the failed patch\n> hint: When you have resolved this problem, run \"git am --continue\".\n> hint: If you prefer to skip this patch, run \"git am --skip\" instead.\n> hint: To restore the original branch and stop patching, run \"git am --abort\".\n> hint: Disable this message with \"git config advice.mergeConflict false\"\n> ~/test/bar$\n>\n> And again, ... the reason most likely git not being able to get that\n> the \"first diff\" is not actually a diff but part of the commit message.\n\nIt did the right thing for me with this (last part) of the commit message:\n\n We should look into Pedro’s backup system. Why does he use email?\n\n diff --git a/a.txt b/a.txt\n index ce01362503..a32119c8aa 100644\n --- a/a.txt\n +++ b/a.txt\n @@ -1 +1,2 @@\n hello\n +goodbye\n\n***\n\nThe `---` part could show up in a commit message naturally. I guess the\nstatus quote remedy (?) is to get burned once with a failed application\nand not use that as a section break any more.\n\nIt seems like it would be nice if format-patch complained if it found\nregex `^---$` in the commit body. It is not at all a magic\n(obfuscated/unlikely) string. If it crashes on that you can fix it\nstraight away. Then you don’t have to wait for two email roundtrips and\nan git-am(1) application.\n\nThe magic string is unlikely but could happen. The solution is to use\nan indented block. Same for the diff. (Hopefully few have to\ncode-quote diffs)\n\nBut escaping things in this format? That has knock-on effects like\nescaping-escaping. Certainly with one-character escapes. I mean if you\nare trying to be resistant to all kinds of strings and characters, and\ncode blocks (with fences, not indented) can contain basically anything,\nyou could end up with likely collisions. Then the format gets harder to\nread.\n\nMaybe with yet more magic strings since (unlike single chars) they are\nunlikely to occur in the input:\n\n Detecting this is all we need to parse that 80-message file that Pedro\n sent us of our school project backup.\n\n (blasphemous bath tub)---\n\n We should look into Pedro’s backup system. Why does he use email?\n\nAnd if you really meant to write that literally:\n\n Detecting this is all we need to parse that 80-message file that Pedro\n sent us of our school project backup.\n\n (blasphemous bath tub)(blasphemous bath tub)---\n\n We should look into Pedro’s backup system. Why does he use email?\n\nBut then the problem is that people have to look at that. Just because\nthey wanted to use `---` for section break. Then they’re probably just\ngonna stop using it for section breaks. Back to square one (could have\njust forbidden it).\n\n> btw and shamelessly off-topic question:\n> Any chance that git format-patch / am will ever support keeping track\n> of the branch/merge history of generated / applied patches?\n> That would be really neat.\n\nWhat does this mean more concretely? :)\n\n> PS: Not subscribed, so please keep me CCed in case you want me to read\n> any possible replies :-)\n\nThat’s the list policy.\n"},{"id":"506580","messageId":"20241104235432.GB3017597@coredump.intra.peff.net","threadId":"62446","inReplyTo":"ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-04T23:54:32Z","receivedAt":"2024-11-04T23:54:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 04, 2024 at 08:24:14PM +0100, Christoph Anton Mitterer wrote:\n\n> 2. The format-patch for that looks already suspicious:\n> - The From line is not escaped (as some variants of mbox would do,\n> some properly some, causing corruption by the escaping with >\n> itself).\n\nAs you note, the mbox format is not well defined. :) The variant with\n\">\"-quoting of \"From\" lines is often called \"mboxrd\", and you can get it\nwith the \"--format=mboxrd\" option.\n\n> - What the format may think of as a separator after the commit\n> message (namely the ---) cannot be used as that either, as a ---\n> in the commit message is again not escaped.\n\nFor this, though, I don't think there is any solution. The receiving\nside of \"git am\" does not know of any unquoting mechanism. So even if\nyou wanted to quote it, you could not get a lossless transmission of the\ncommit message.\n\nThis does occasionally cause confusion. Especially if you include an\nunindented diff in your commit message, which similarly (and\nintentionally) triggers the \"---\" detection. But I think it's one of\nthose things that just doesn't come up often enough for anybody to have\ncared about trying to address.\n\n-Peff\n"},{"id":"506582","messageId":"xmqqttcmv8a6.fsf@gitster.g","threadId":"62446","inReplyTo":"ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-05T00:22:41Z","receivedAt":"2024-11-05T00:22:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christoph Anton Mitterer writes:\n\n> There seems unfortunately only little (written) definition of that\n> format, git-format-patch(1) merely says it's in UNIX mailbox format\n> (which itself is, AFAIK, not really formally defined).\n>\n>\n> Anyway, it seems to turn out, that no escaping is done for the commit\n> message in the patch format and that this can cause actual breakage\n> with valid commit messages.\n\nYes, so ...\n\n> Consider the following example:\n> ~/test/foo$ git commit -m \"msg1\n> \n> From 0000000000000000000000000000000061603705 Mon Sep 17 00:00:00 2001\n> --\n\n... this falls squarely into \"if it hurts, don't do it\" category.\n\nA commonly used trick, when you are working on Git and have to\ninclude such a line in the commit log message, e.g., when you\ndiscuss how the output produced by \"git log --format=email\" looks\nlike, is to indent such lines in the paragraph you talk about them,\ne.g.\n\n In a format-patch output file, the contents of each commit\n begins with\n\n\tFrom 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772 Mon Sep 17 00:00:00 2001\n\n where the string 8f8d... is replaced with the name of the commit\n object the patch was taken from.\n\n"},{"id":"506585","messageId":"83639e75d9d04208aa0dee345d9ef3536de105c9.camel@scientia.org","threadId":"62446","inReplyTo":"305dc9f7-4bdb-40c5-92f4-7438a9ecd482@app.fastmail.com","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Christoph Anton Mitterer","fromEmail":"calestyo@scientia.org","sentAt":"2024-11-05T01:26:00Z","receivedAt":"2024-11-05T01:26:04Z","isPatch":false,"sender":{"key":"calestyo@scientia.org","avatar":"https://gravatar.com/avatar/2dd3cd5191e4ec4a5e1e0a0861a64378636270f4007bab080a27b27a607bb267?d=mp&s=160"},"body":"On Mon, 2024-11-04 at 23:15 +0100, Kristoffer Haugsbakk wrote:\n> It seems to me (totally naïve) that you would do something like\n> \n> 1. Blank line terminates headers\n> 2. Then there might be some optional commit-headers which can\n> override\n>    things (`From`)\n> 3. Commit message\n> 4. `---`\n> 5. Look for a regex `^diff` line\n>    • Now the indentation will tell you when it ends\n> 6. `^Range-diff` and `^Interdiff` can also make an appearance in this\n>    section\n\nWell as you've seen by the follow-up, such a naive approach is not\nreally possible, as the commit message may also contain ---, unified\ndiffs, etc.\n\n\n\n> At first I thought that it is a magic string for a reason.\n\nI think such magic cookies can always only (safely) be used to detect a\ntype, but not as a separator, at least not when free text is allowed,\ntoo.\n\n\n> It did the right thing for me with this (last part) of the commit\n> message:\n> \n>     We should look into Pedro’s backup system.  Why does he use\n> email?\n> \n>         diff --git a/a.txt b/a.txt\n>         index ce01362503..a32119c8aa 100644\n>         --- a/a.txt\n>         +++ b/a.txt\n>         @@ -1 +1,2 @@\n>          hello\n>         +goodbye\n> \n> ***\n\nBut yours look indented? I mean then it's clear it works.\n\n\n> It seems like it would be nice if format-patch complained if it found\n> regex `^---$` in the commit body.\n\nActually already when committing... cause there it's taken as valid and\nthen it should also work with any following tools.\n\n\n> The magic string is unlikely but could happen.  The solution is to\n> use\n> an indented block.  Same for the diff.  (Hopefully few have to\n> code-quote diffs)\n\nAs written in the other mail, there is nothing real obvious for the\nuser that this wouldn’t be allowed, and in fact committing and such\nworks.\nThe simple problem here is the fuzzy format which cannot be parsed\nproperly.\n\n\n\n\n> But escaping things in this format?\n\nCoudln't one do something like this:\n\nIf the line consisting of three - is the line that ends the commit\nmessage, check during format-patch whether it's contained in that.\nIf not, generate the patch as now.\nIf so, use another magic timestamp and/or (since that might get lost\nwhen sending as mail, set some X-git-patch-format: header, there adding\nperhaps a flag like \"escaped\" and if that's set, any line that matches\nthe regexp:\n>*---\nget's another > prepended when escaping, and one removed when\nunescaping (well in the latter only lines that match >+---).\n* = 0..n\n+ = 1..n\n\nOr probably thinking about some more sophisticated solution or at least\na better character than > .\n\n\n> > btw and shamelessly off-topic question:\n> > Any chance that git format-patch / am will ever support keeping\n> > track\n> > of the branch/merge history of generated / applied patches?\n> > That would be really neat.\n> \n> What does this mean more concretely?  :)\n\nI guess I want what's been asked there:\nhttps://stackoverflow.com/questions/2285699/git-how-to-create-patches-for-a-merge\n\n\nIn short, imagine I have the following commit tree (just a simple\nexample with --no-ff):\n*\n|\\\n|*\n|/\n*\n\nAnd I make a series of git format-patch patches from that, it would be\nnice if git am *.patches, give back that structure (i.e. with the\nbranch and the merge).\n\n\nCheers,\nChris.\n"},{"id":"506588","messageId":"3a96888e76e4dd26a3b0a81a19cda8ec7de72662.camel@scientia.org","threadId":"62446","inReplyTo":"20241104235432.GB3017597@coredump.intra.peff.net","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Christoph Anton Mitterer","fromEmail":"calestyo@scientia.org","sentAt":"2024-11-05T01:03:21Z","receivedAt":"2024-11-05T01:42:53Z","isPatch":false,"sender":{"key":"calestyo@scientia.org","avatar":"https://gravatar.com/avatar/2dd3cd5191e4ec4a5e1e0a0861a64378636270f4007bab080a27b27a607bb267?d=mp&s=160"},"body":"On Mon, 2024-11-04 at 18:54 -0500, Jeff King wrote:\n> As you note, the mbox format is not well defined. :) The variant with\n> \">\"-quoting of \"From\" lines is often called \"mboxrd\", and you can get\n> it\n> with the \"--format=mboxrd\" option.\n\nBut as you've said below, here too, the receiving side most likely\ndoesn’t know that and then a wrong commit message would be applied\n(with no unescaping being performed or it would simply break again when\nthe magic From is used).\n\n\nCheers,\nChris.\n"},{"id":"506600","messageId":"b856a94969b7065e84dce60d35ae3ce72b5d0f91.camel@scientia.org","threadId":"62446","inReplyTo":"xmqqttcmv8a6.fsf@gitster.g","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Christoph Anton Mitterer","fromEmail":"calestyo@scientia.org","sentAt":"2024-11-05T01:01:16Z","receivedAt":"2024-11-05T04:17:54Z","isPatch":false,"sender":{"key":"calestyo@scientia.org","avatar":"https://gravatar.com/avatar/2dd3cd5191e4ec4a5e1e0a0861a64378636270f4007bab080a27b27a607bb267?d=mp&s=160"},"body":"Hey.\n\n\nOn Mon, 2024-11-04 at 16:22 -0800, Junio C Hamano wrote:\n> ... this falls squarely into \"if it hurts, don't do it\" category.\n\nI rather thought it would be the category \"if it's a bug, that is\ncompletely non-obvious and not really to expect, with not even a\nwarning in place,... it should better be fixed\" ;-)\n\nThere is not reason for any commit author to assume that these strings\nare not valid, or possibly cause later breakage.\n\nEven if so, it seems not really feasible to know any possible strings,\nespecially when people might just copy&paste output from others into\ntheir commit message.\n\nTo the least there should be an error when format-patch creates a one\nthat cannot be parsed afterwards.\nAnd a proper solution would employ some form of escaping.\n\n\n\nAnd if one's on the other side, and wants to write a parser, one has no\nreal chance of assuring that only \"valid\" input is used by users in a\ncommit message.\n\nOne can easily imagine hat this could accidentally (or intentionally)\nbe used apply undesired patches.\n\n\nCheers,\nChris.\n\n"},{"id":"506660","messageId":"43b401e0-df86-4849-8747-d5ab172becb6@app.fastmail.com","threadId":"62446","inReplyTo":"83639e75d9d04208aa0dee345d9ef3536de105c9.camel@scientia.org","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-05T14:32:21Z","receivedAt":"2024-11-05T14:32:52Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Tue, Nov 5, 2024, at 02:26, Christoph Anton Mitterer wrote:\n> On Mon, 2024-11-04 at 23:15 +0100, Kristoffer Haugsbakk wrote:\n>> It seems to me (totally naïve) that you would do something like\n>>\n>> 1. Blank line terminates headers\n>> 2. Then there might be some optional commit-headers which can\n>> override\n>>    things (`From`)\n>> 3. Commit message\n>> 4. `---`\n>> 5. Look for a regex `^diff` line\n>>    • Now the indentation will tell you when it ends\n>> 6. `^Range-diff` and `^Interdiff` can also make an appearance in this\n>>    section\n>\n> Well as you've seen by the follow-up, such a naive approach is not\n> really possible, as the commit message may also contain ---, unified\n> diffs, etc.\n\nIt’s possible in the sense that it would work just as well as\ngit-format-patch(1).\n\nYou could make it more robust with some backtracking, like finding the\nlast `^diff` and movig back. That’s OK in a file with only one patch\n(`.patch`) but harder to do for an mbox file.\n\n> […]\n>> It seems like it would be nice if format-patch complained if it found\n>> regex `^---$` in the commit body.\n>\n> Actually already when committing... cause there it's taken as valid and\n> then it should also work with any following tools.\n\nThat would inconvenience all users that never use format-patch. You\ncould provide an advice/hint about some configuration variable which\nturns off this new default. But that’s a lot of work to benefit\nformat-patch users.\n\nSuch an error would have to be a default because an opt-in would only be\ndiscovered (in practice) after you’ve been burned once. And then the\nopt-in error is of very little value. You’re realistically not going to\nforget and write commit messages with `^---$` after that.\n\n>> The magic string is unlikely but could happen.  The solution is to\n>> use\n>> an indented block.  Same for the diff.  (Hopefully few have to\n>> code-quote diffs)\n>\n> As written in the other mail, there is nothing real obvious for the\n> user that this wouldn’t be allowed, and in fact committing and such\n> works.\n> The simple problem here is the fuzzy format which cannot be parsed\n> properly.\n\nIt’s not non-obvious either. The simple format is apparent if you\nreview your patches before sending them out into the world. (I’m\nparanoid so I do that)\n\n>> But escaping things in this format?\n>\n> Coudln't one do something like this:\n>\n> If the line consisting of three - is the line that ends the commit\n> message, check during format-patch whether it's contained in that.\n> If not, generate the patch as now.\n> If so, use another magic timestamp and/or (since that might get lost\n> when sending as mail, set some X-git-patch-format: header, there adding\n> perhaps a flag like \"escaped\" and if that's set, any line that matches\n> the regexp:\n>>*---\n> get's another > prepended when escaping, and one removed when\n> unescaping (well in the latter only lines that match >+---).\n> * = 0..n\n> + = 1..n\n>\n> Or probably thinking about some more sophisticated solution or at least\n> a better character than > .\n\nThat’s interesting and a good idea to use an email header to signal the\nescaping.\n\nThinking just about `^---$`: an email header could be generated if\n`^---$` occurs in the commit message. Then it could suggest something\nnon-occurring instead. Simply `-----` or `***` or something.\n\nBut this might not just help with the commit message separator.\nApparently the status quo is that git-am(1) will try to apply the diff\nin a commit message. Even though it hasn’t seen `^---$` yet. But if it\nwas required to see that first then you could do something like:\n\n```\ndiff ...\n\n```\n\nAnd not have any issues.\n\nOf course you can’t change the magic mbox string. But I don’t see how\nyou can’t do some more expensive parsing and require that you see the\nend of the commit message before parsing this string as the\nmarker/delimiter.\n\nSome niggles: the commit message might just be the subject line and\nthere might be no diff (empty patch). Then looking for `^---$` will\ntake you too far. Maybe just look for the signature line.\n"},{"id":"506663","messageId":"20241105150540.GA3043294@coredump.intra.peff.net","threadId":"62446","inReplyTo":"3a96888e76e4dd26a3b0a81a19cda8ec7de72662.camel@scientia.org","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-05T15:05:40Z","receivedAt":"2024-11-05T15:05:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 05, 2024 at 02:03:21AM +0100, Christoph Anton Mitterer wrote:\n\n> On Mon, 2024-11-04 at 18:54 -0500, Jeff King wrote:\n> > As you note, the mbox format is not well defined. :) The variant with\n> > \">\"-quoting of \"From\" lines is often called \"mboxrd\", and you can get\n> > it\n> > with the \"--format=mboxrd\" option.\n> \n> But as you've said below, here too, the receiving side most likely\n> doesn’t know that and then a wrong commit message would be applied\n> (with no unescaping being performed or it would simply break again when\n> the magic From is used).\n\nI think there are two differences between \">From\" and quote \"---\":\n\n 1. There are already mbox parsers that understand and unquote \">From\".\n If you know you are feeding your patches to such a parser (e.g.,\n the one in mutt) then using \"mboxrd\" makes sense.\n\n 2. The consumer of the mbox format is much \"closer\" than the consumer\n of the \"---\" line.\n\n If you produce some patches with format-patch, you might feed them\n to your MUA or another local tool, and you know how that tool will\n unquote them. Once you email the patches, that \">From\" quoting is\n irrelevant, because the receiver will get individual emails without\n the quoting. They may themselves choose to store them in an mbox,\n of course, but the details are up to them.\n\n Whereas the \"---\" line is inside the email, and will be interpreted\n on the other end by a tool like \"git am\". So if we introduce\n quoting in format-patch and unquoting in git-am, then correctly\n unquoting will depend on the version that the other side is using.\n\nIMHO that is not necessarily a reason to avoid implementing quoting. If\nyou do accidentally put \"---\" in a commit message, the current outcome\nis for your commit message to get silently cut off by \"git am\". But in a\nworld where we quote it and the other side is too old to unquote, then\nthey may end up with an extra quoting character in the result. But that\nis probably a less bad outcome overall.\n\n There's also another flip-side mistake, which is somebody who doesn't\n quote sends to somebody who unquotes, and the result is similarly\n slightly corrupted. The mbox equivalent is that you meant to say\n \">From\" with the \">\", but the receiver turned it into just \"From\".\n\nSo I don't think it's a totally wrong direction to go. But like I said,\nit doesn't seem to happen often enough in practice that anybody has been\nmotivated to add the feature.\n\n-Peff\n"},{"id":"506664","messageId":"527da3336bc6cbc550b5cd271dc5689b32f400e1.camel@scientia.org","threadId":"62446","inReplyTo":"43b401e0-df86-4849-8747-d5ab172becb6@app.fastmail.com","subject":"Re: git format-patch escaping issues in the patch format","fromName":"Christoph Anton Mitterer","fromEmail":"calestyo@scientia.org","sentAt":"2024-11-05T15:02:16Z","receivedAt":"2024-11-05T15:07:50Z","isPatch":false,"sender":{"key":"calestyo@scientia.org","avatar":"https://gravatar.com/avatar/2dd3cd5191e4ec4a5e1e0a0861a64378636270f4007bab080a27b27a607bb267?d=mp&s=160"},"body":"On Tue, 2024-11-05 at 15:32 +0100, Kristoffer Haugsbakk wrote:\n> You could make it more robust with some backtracking, like finding\n> the\n> last `^diff` and movig back.  That’s OK in a file with only one patch\n> (`.patch`) but harder to do for an mbox file.\n\nI wonder whether that (parsing from the end) is really a proper\nsolution or whether it could still break.\n\nA patch can contain multiple diffs, like in:\n From b3b82e59ea52fc059dc23ecc1a3cc0810d297b10 Mon Sep 17 00:00:00 2001\n From: Christoph Anton Mitterer \n Date: Tue, 5 Nov 2024 15:40:13 +0100\n Subject: [PATCH] foo\n \n ---\n a | 1 +\n b | 1 +\n 2 files changed, 2 insertions(+)\n create mode 100644 a\n create mode 100644 b\n \n diff --git a/a b/a\n new file mode 100644\n index 0000000..ae5b3c5\n --- /dev/null\n +++ b/a\n @@ -0,0 +1 @@\n +Aaa\n diff --git a/b b/b\n new file mode 100644\n index 0000000..f761ec1\n --- /dev/null\n +++ b/b\n @@ -0,0 +1 @@\n +bbb\n -- \n 2.45.2\n\n\nBut the unified diff shouldn't be able to contain newlines or a line\nconsisting only of --- .\n\nSo would a proper description be:\nFrom Mon Sep 17 00:00:00 2001\n\n\n\n---\n\n\n\n--\n\n\n?\n\nSeems still pretty brittle.\n\n> \n\n> > Actually already when committing... cause there it's taken as valid\n> > and\n> > then it should also work with any following tools.\n> \n> That would inconvenience all users that never use format-patch.\n\nSure, the real solution would still be to do proper escaping.\n\n\n> It’s not non-obvious either.  The simple format is apparent if you\n> review your patches before sending them out into the world.  (I’m\n> paranoid so I do that)\n\nThe whole arguing \"the user must check what he writes in the commit\nmessage\" seems a bit to me as if users would need to think about not\nbeing allowed to use characters like < etc. when they write text which\nmight be stored as HTML, because that wouldn't provide a quoting\nmechanism which an editor could automatically employ.\n\n\n> That’s interesting and a good idea to use an email header to signal\n> the\n> escaping.\n\nMy first idea was using MIME, but I guess many people wouldn’t be all\ntoo delighted seeing patches with MIME and the commit message encoded\nas base64 or so ;-)\n\nSo any quoting should be still human readable (though git already does\nuse some IMO not so human readable encoding for Subject: lines).\n\nUsing something that is already standard would be of course nicer, but\nif it's not accepted, than better a simple schema that still works.\n\n\n> Thinking just about `^---$`: an email header could be generated if\n> `^---$` occurs in the commit message.  Then it could suggest\n> something\n> non-occurring instead.  Simply `-----` or `***` or something.\n\nShould work, but if one has strange enough commit messages, either the\nnew separator would get stranger and stranger, or, if there was just a\nhardcoded list of alternatives, one could run out of alternatives.\n\n\nIf a header would get introduced one should make it generic enough...\ne.g. to allow for different quoting schemas, or even to allow for\ncompletely other format information.\n\n\n> Some niggles: the commit message might just be the subject line and\n> there might be no diff (empty patch).  Then looking for `^---$` will\n> take you too far.  Maybe just look for the signature line.\n\nAnother way might be to simply store in a header just how many lines of\ncommit messages follow.\nBut I think that would have also some downsides.\n\n\nCheers,\nChris.\n"}]}