PSA: Did you know that it’s **unsafe** to put code diffs into your commit messages?
Like https://github.com/i3/i3/pull/6564 for example
Such diffs will be applied by patch(1) (also git-am(1)) as part of the code change!
This is how a sleep(1) made it into i3 4.25-2 in Debian unstable.
@RandamuMaki @zekjur This was not caused by GitHub. The GitHub PR merged without issue (because it's not patch-based), it was when the Debian maintainer exported it as a patch and applied it that the problem appeared. patch is the problem, and git-am inherits the problem from patch.
edit: Just to verify (I definitely don't want to unfairly not blame Microsoft; I love blaming Microsoft), I tested it out on the command line with just git tools, and got the same result. If GitHub is generating bad patches that need escaping, so does git format-patch:
Michael Stapelberg 🐧🐹😺
Maki 🔻 🌹
taylor> git checkout e285b3c47b2fc7a4cdaba9a427d2958410e64620
HEAD is now at e285b3c4 Fix i3bar workspace buttons for primary screen (#6564)
> grep sleep i3bar/src/workspaces.c
> git format-patch e285b3c47b2fc7a4cdaba9a427d2958410e64620~..e285b3c47b2fc7a4cdaba9a427d2958410e64620
0001-Fix-i3bar-workspace-buttons-for-primary-screen-6564.patch
> git checkout e285b3c47b2fc7a4cdaba9a427d2958410e64620~
Previous HEAD position was e285b3c4 Fix i3bar workspace buttons for primary screen (#6564)
HEAD is now at 3a6f17d7 t/289-ipc-shutdown-event: remove AnyEvent timers
> git am 0001-Fix-i3bar-workspace-buttons-for-primary-screen-6564.patch
Applying: Fix i3bar workspace buttons for primary screen (#6564)
> grep sleep i3bar/src/workspaces.c
sleep(1);
>
This is an inherent problem with in-band signaling. The patch format is simply poorly structured for anything containing non-patch data, unless the non-patch data is specially and deterministically formatted to avoid collision. As-is, git-format-patch and GitHub's patch output can't do anything about it without changing the commit message (which is probably a better solution, given that the current patch breaks both the patch application and the commit message).
git format-patch and git am to use MIME multipart/mixed and deterministically and unambiguously separate the patch from the commit message. They're already generating an mbox format as it is.An amusing thing is that there is undoubtedly a whole segment of the readership here that has already remembered the days when the multitude of "mbox" mailbox formats caused bodies to be interpreted as headers in much the same way as patch is interpreting human-readable messages as patching instructions.
And that has remembered Bernstein on the subject of parsing and quoting, too.
@RandamuMaki @zekjur
#patch #DanielJBernstein #security #git
JdeBP