From: Christoph Anton Mitterer <calestyo@scientia.org>
Date: Tue, 05 Nov 2024 15:02:16 GMT
Subject: Re: git format-patch escaping issues in the patch format
Message-ID: <527da3336bc6cbc550b5cd271dc5689b32f400e1.camel@scientia.org>
In-Reply-To: <43b401e0-df86-4849-8747-d5ab172becb6@app.fastmail.com>

On Tue, 2024-11-05 at 15:32 +0100, Kristoffer Haugsbakk wrote:
> You could make it more robust with some backtracking, like finding
> the
> last `^diff` and movig back.  That’s OK in a file with only one patch
> (`.patch`) but harder to do for an mbox file.

I wonder whether that (parsing from the end) is really a proper
solution or whether it could still break.

A patch can contain multiple diffs, like in:
   From b3b82e59ea52fc059dc23ecc1a3cc0810d297b10 Mon Sep 17 00:00:00 2001
   From: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
   Date: Tue, 5 Nov 2024 15:40:13 +0100
   Subject: [PATCH] foo
   
   ---
    a | 1 +
    b | 1 +
    2 files changed, 2 insertions(+)
    create mode 100644 a
    create mode 100644 b
   
   diff --git a/a b/a
   new file mode 100644
   index 0000000..ae5b3c5
   --- /dev/null
   +++ b/a
   @@ -0,0 +1 @@
   +Aaa
   diff --git a/b b/b
   new file mode 100644
   index 0000000..f761ec1
   --- /dev/null
   +++ b/b
   @@ -0,0 +1 @@
   +bbb
   -- 
   2.45.2


But the unified diff shouldn't be able to contain newlines or a line
consisting only of --- .

So would a proper description be:
From <ID> Mon Sep 17 00:00:00 2001
<mail style headers, no empty lines>
<exactly one empty line to separate headers from commit message>
<any arbitrary sequence of zero or more lines>
---
<a number of lines (not sure whether it could be zero) all starting with a space>
<exactly one empty line>
<n unified diffs, no empty lines, no lines consisting solely of --- >
--
<version>

?

Seems still pretty brittle.

> 

> > Actually already when committing... cause there it's taken as valid
> > and
> > then it should also work with any following tools.
> 
> That would inconvenience all users that never use format-patch.

Sure, the real solution would still be to do proper escaping.


> It’s not non-obvious either.  The simple format is apparent if you
> review your patches before sending them out into the world.  (I’m
> paranoid so I do that)

The whole arguing "the user must check what he writes in the commit
message" seems a bit to me as if users would need to think about not
being allowed to use characters like < etc. when they write text which
might be stored as HTML, because that wouldn't provide a quoting
mechanism which an editor could automatically employ.


> That’s interesting and a good idea to use an email header to signal
> the
> escaping.

My first idea was using MIME, but I guess many people wouldn’t be all
too delighted seeing patches with MIME and the commit message encoded
as base64 or so ;-)

So any quoting should be still human readable (though git already does
use some IMO not so human readable encoding for Subject: lines).

Using something that is already standard would be of course nicer, but
if it's not accepted, than better a simple schema that still works.


> Thinking just about `^---$`: an email header could be generated if
> `^---$` occurs in the commit message.  Then it could suggest
> something
> non-occurring instead.  Simply `-----` or `***` or something.

Should work, but if one has strange enough commit messages, either the
new separator would get stranger and stranger, or, if there was just a
hardcoded list of alternatives, one could run out of alternatives.


If a header would get introduced one should make it generic enough...
e.g. to allow for different quoting schemas, or even to allow for
completely other format information.


> Some niggles: the commit message might just be the subject line and
> there might be no diff (empty patch).  Then looking for `^---$` will
> take you too far.  Maybe just look for the signature line.

Another way might be to simply store in a header just how many lines of
commit messages follow.
But I think that would have also some downsides.


Cheers,
Chris.