From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 09 Feb 2026 15:58:51 GMT
Subject: Re: git-am applies commit message diffs
Message-ID: <aYoEO0CcVt2Qjgnb@pks.im>
In-Reply-To: <20260206090358.GA2761602@coredump.intra.peff.net>

On Fri, Feb 06, 2026 at 04:03:58AM -0500, Jeff King wrote:
> On Fri, Feb 06, 2026 at 09:18:50AM +0100, Matthias Beyer wrote:
> 
> > That said, I am no expert in either C or the git codebase at all, but
> > from what I saw from reading the git-am codebase, it looks like it tries
> > to find the patch by looking for three dashes on a line with a linebreak
> > behind ("---\n").
> 
> Yes, that is how the split is made.
> 
> > From what I read, it looks for that from the first line.
> > What I would think of here is looking for that "patchbreak" from the
> > _end_ of the email rather than from the top, that would have prevented
> > this issue, right?
> 
> The patch itself may legitimately contain "---" on a line by itself (it
> would indicate that the line "--" was removed from a file). That would
> confuse your parser, including in a way that we end up only applying
> part of the diff (everything before that fake "---" becomes commit
> message, and everything after becomes cover-letter material up to the
> next "diff" line).
> 
> I suspect it also creates corner cases with cover-letter material
> (between the "---" and the diff itself) that itself contains any "---"
> marker.
> 
> I don't think there is a way to unambiguously parse the single-stream
> output that format-patch produces. This is a reasonably well-known
> gotcha (at least around here). E.g., some earlier discussions:
> 
>   2024: https://lore.kernel.org/git/ca13705ae4817ffba16f97530637411b59c9eb19.camel@scientia.org/
>   2022: https://lore.kernel.org/git/d0b577825124ac684ab304d3a1395f3d2d0708e8.1662333027.git.matheus.bernardino@usp.br/
>   2015: https://lore.kernel.org/git/CAFOYHZC6Qd9wkoWPcTJDxAs9u=FGpHQTkjE-guhwkya0DRVA6g@mail.gmail.com/
> 
> There are probably more, but it's actually a tricky thing to search for
> in the archive, so I stopped digging. ;)

Maybe we can't parse it unambiguously. But what we _can_ detect is that
a patch is ambiguous in the first place, right? So maybe we could extend
git-am(1) to bail by default with a hint that tells the user that:

  - They ought to double-check the patch.

  - They can override the check with "--accept-ambiguous-patch".

It at least notifies the user that something potentially-fishy is going
on, even though it still shifts the burden onto the person that applies
the patch. But I guess that cannot ever be avoided anyway, at least in
the general case.

Patrick