This has come up multiple times before [1], and more generally it's come up hundreds of times with Unix style tools in general. It's always been a stupid idea for every tool to have its own barely documented file format.
This wouldn't be an issue if patches were XML or JSON with a well defined schema, but everything must be a boutique undocumented format in the world of Unix tools.
Maybe the worst part about this is that it can entirely come from a patch being exported by git and then imported straight back in to git. If you can't even handle your own undocumented format then what hope do other tools have that want to work with it?
While patch[0] has problems, the issue here is not that it is undocumented.
Git recently added this doc on roundtripping, and the problem is with git.
Any line that is of the form:
* three-dashes and end-of-line, or
* a line that begins with "diff -", or
* a line that begins with "Index: "
is taken as the beginning of a patch, and the commit log message is terminated before the first occurrence of such a line.
The patch isn't even the complicated forms with RCS, ClearCase, Perforce, or SCCS support, it is just doing what the pre-POSIX spec says.
The argument is if git should do input sanitation etc...
But `patch -p1` is doing exactly what was documented, even in the original Larry Wall usenet post of the program.
Seems like this would probably be solved if github returned a patch file formatted like `git show` provides, specifically with the commit message indented? I do see that `git format-patch` doesn't do this indentation though.
In any case, agreed that it's not a great "feature" to use in-band signaling of when patch data starts, with no escaping. Confusion and misbehavior is pretty much guaranteed.
> Seems like this would probably be solved if github returned a patch file formatted like `git show` provides, specifically with the commit message indented? I do see that `git format-patch` doesn't do this indentation though.
This would be "solved" if the patch file only included the patch. That's pretty straightforward. The file github provides includes fake email headers for no particular reason. The commit message appears to be part of the subject header. The subject header is never terminated, so arguably applying this patch shouldn't do anything. (Because the actual patch data is also part of the email headers.) The other headers aren't terminated either, so actually there is no subject header. This shouldn't really matter, because the patch file isn't email, but it does seem to want to pretend to be.
The usual question to ask here would be "why are you applying patch files from an untrusted source?". If patch(1) was stricter about the format of its input files... applying patches from an untrusted source would still be a good way to get owned. If you think I can get you to patch inappropriate files by writing a fake diff into my commit message... wait until you see what I can do by writing those same changes into the real diff.
git-am(1) (apply patches) delimits the commit message from the patch/diff by looking for (1) a line `---` or (2) a line that starts with `diff -` or (3) a line that starts with `Index:SP` (SP is space). Only the first rule is necessary for patches generated git-format-patch(1). But git-am(1) is for applying patches, and you are free to bring patches from some other system. That’s why, I suppose, there are multiple options.
This means that it will try to apply any unindented diffs in the commit message. But you’re fine if you indent the diff. (Newschool code fencers will have a worse time here.)
I imagine that this worked fine for changes that were authored by one person and submitted by another person via email, or by their friend, or by someone trying to resurrect a previous attempt at getting something upstreamed. Someone is likely to notice that examples diffs are getting applied. But it won’t work well at all if you are some software distributor who is using patch files to apply modifications to packages.
Recall that git-am(1) will not apply indented diffs. Well have a look at my GNU patch 2.7.6:
If the entire diff is indented by a consistent amount, if lines end in
CRLF, or if a diff is encapsulated one or more times by prepending "- "
to lines starting with "-" as specified by Internet RFC 934, this is
taken into account.
Some may say that patch(1) should work like a more straightforward importer. But I’ve been itching to point out something else.
> It matters (to me) because `wget`/`curl` plus `patch` is not some exotic lab setup.
If the point is to be able to do `curl https://...deadbeef.patch | patch -p1`, you can just change the extension provided to Github from `.patch` to `.diff`. That way, it just includes the hunks. E.g.
But what problem does this actually introduce? If you are applying a patch you must already trust the source anyways and this isn’t harder to spot than a rogue file anywhere else in the patch as it looks the same.
Field confusion. If you manually reviewed the diff in GitHub but did not pay attention to the commit message (which GitHub already collapses when long or may start doing any day they want), you are screwed.
Imagine the diff only has
if (someIntParamThatShouldHaveBeenUInt < 0)
throw new ArgumentOutOfRangeException();
I like that the site offers your choice of two fonts, but they're both ugly and hard to read. Mono is too narrow, and OpenDyslexic is cartoonish and o v e r k e r n e d.
Not sure why somebody thought it was a good idea. There probably was a use case at some point where it seemed smart to allow for things like this. Maybe it’s for CI, but I really can’t think of why.
But I really doubt this could be a bug. I mean why would patch even need to read the commit message if it wasn’t to scan it for diff?
14 comments
[ 2.6 ms ] story [ 35.4 ms ] threadThis wouldn't be an issue if patches were XML or JSON with a well defined schema, but everything must be a boutique undocumented format in the world of Unix tools.
Maybe the worst part about this is that it can entirely come from a patch being exported by git and then imported straight back in to git. If you can't even handle your own undocumented format then what hope do other tools have that want to work with it?
[1]: https://mas.to/@zekjur/116022397626943871
Git recently added this doc on roundtripping, and the problem is with git.
The patch isn't even the complicated forms with RCS, ClearCase, Perforce, or SCCS support, it is just doing what the pre-POSIX spec says.The argument is if git should do input sanitation etc...
But `patch -p1` is doing exactly what was documented, even in the original Larry Wall usenet post of the program.
[0] https://pubs.opengroup.org/onlinepubs/9799919799/utilities/p... [1] https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2...
In any case, agreed that it's not a great "feature" to use in-band signaling of when patch data starts, with no escaping. Confusion and misbehavior is pretty much guaranteed.
This would be "solved" if the patch file only included the patch. That's pretty straightforward. The file github provides includes fake email headers for no particular reason. The commit message appears to be part of the subject header. The subject header is never terminated, so arguably applying this patch shouldn't do anything. (Because the actual patch data is also part of the email headers.) The other headers aren't terminated either, so actually there is no subject header. This shouldn't really matter, because the patch file isn't email, but it does seem to want to pretend to be.
The usual question to ask here would be "why are you applying patch files from an untrusted source?". If patch(1) was stricter about the format of its input files... applying patches from an untrusted source would still be a good way to get owned. If you think I can get you to patch inappropriate files by writing a fake diff into my commit message... wait until you see what I can do by writing those same changes into the real diff.
This means that it will try to apply any unindented diffs in the commit message. But you’re fine if you indent the diff. (Newschool code fencers will have a worse time here.)
I imagine that this worked fine for changes that were authored by one person and submitted by another person via email, or by their friend, or by someone trying to resurrect a previous attempt at getting something upstreamed. Someone is likely to notice that examples diffs are getting applied. But it won’t work well at all if you are some software distributor who is using patch files to apply modifications to packages.
Recall that git-am(1) will not apply indented diffs. Well have a look at my GNU patch 2.7.6:
Some may say that patch(1) should work like a more straightforward importer. But I’ve been itching to point out something else. Is it surprising if patch(1) is a bit DWIM?If the point is to be able to do `curl https://...deadbeef.patch | patch -p1`, you can just change the extension provided to Github from `.patch` to `.diff`. That way, it just includes the hunks. E.g.
https://github.com/torvalds/linux/commit/dca922e019dd758b4c1...
I don't see it as a problem with the email format, because I can't imagine someone just patching from an email without looking at the email first.
Imagine the diff only has
Would you care who wrote it?Not sure why somebody thought it was a good idea. There probably was a use case at some point where it seemed smart to allow for things like this. Maybe it’s for CI, but I really can’t think of why.
But I really doubt this could be a bug. I mean why would patch even need to read the commit message if it wasn’t to scan it for diff?
Andreas Gruenbacher is active currently. Or git can change to differentiate the message prefix better