Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Patch applies fake diffs from commit messages

samizdat.dev

Post reply on HN
Print view View original

11–20 of 43 posts

Re: Patch applies fake diffs from commit messages

#11

Post by thaumasiotes »

Groxx wrote: post #3

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.

Re: Patch applies fake diffs from commit messages

#12

Post by phoe-krk »

yjftsjthsd-h wrote: post #4
LiamPowell wrote: post #2

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…

> 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. Patch files are readable by humans. Replacing them with XML or JSON would fix this problem, but at the expense of removing a core feature.

If, by "readable by humans", you mean "it would reliably fool humans as well", I'd say it's an ambiguity bug regardless of whether it's "a core feature" or not. A patch format, human-readable or not, should clearly indicate which part is the commit message and which part is an actual diff; it's not the case here.

Re: Patch applies fake diffs from commit messages

#13

Post by thesz »

kevin_thibedeau wrote: post #9
LiamPowell wrote: post #2

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…

Patch: 1985 SGML: 1986

XML: 1996

Re: Patch applies fake diffs from commit messages

#14

Post by Avamander »

LiamPowell wrote: post #2

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…

This is where I kind-of like the idea of PowerShell, it's just that I dislike almost all other aspects of it and around it.

Re: Patch applies fake diffs from commit messages

#15

Post by nyrikki »

LiamPowell wrote: post #2

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…

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.

[0] https://pubs.opengroup.org/onlinepubs/9799919799/utilities/p... [1] https://github.com/git/git/blob/94f057755b7941b321fd11fec1b2...

Re: Patch applies fake diffs from commit messages

#16

Post by yjftsjthsd-h »

phoe-krk wrote: post #12
yjftsjthsd-h wrote: post #4

Earlier quoted context omitted.

> 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. Patch files are readable by humans. Replacing them with XML or JSON would fix this problem, but at the expense of removing a core feature.

If, by "readable by humans", you mean "it would reliably fool humans as well", I'd say it's an ambiguity bug regardless of whether it's "a core feature" or not. A patch format, human-readable or not, should clearly indicate which part is the commit message and which part is an actual diff; it's not the case here.

Alright, allow me to disambiguate in your preferred format.

     Patch is perfect. Ambiguity is good. There are no better formats for conveying patches.   Patch files are readable by humans. Being readble by humans is useful. XML is painful for humans to read and write. JSON is painful for humans to read and write. JSON or XML would actually fix this problem in the format.   The patch format could be improved. Formats should be unambiguous. Separating sections is good.

Re: Patch applies fake diffs from commit messages

#17

Post by quuxplusone »

thaumasiotes wrote: post #11
Groxx wrote: post #3

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 messag…

> fake email headers

That's the output you get from `git format-patch --stdout -1 dd28283`. The idea is that it's suitable for emailing to a mailing list for review (hence the subject line beginning with [PATCH], and so on).

If you ask for colorized output with `git format-patch --color=always --stdout -1 dd28283` you'll see that `git format-patch` itself knows which bits are the commit message and which bits are the diff. (Well, of course it does, I guess.)

I suspect that if you sent a patch like this to the mailing list, they'd get mad at you. So `git format-patch` is working OK for its intended use-case. Arguably it's GitHub causing the problem here by "misusing" `git format-patch` as a way to deliver patches that are (these days) expected to be machine-readable — something you can just curl and pipe into `patch`. `git format-patch` doesn't do that.

That said, yeah, it's amusing that (as TFA says)

    git format-patch -1 HEAD --stdout > 0001 ;
    git checkout HEAD~ ;
    git am 0001
isn't a clean round-trip. `git am` applies the fake diff.

> 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.

Well put. :)

Re: Patch applies fake diffs from commit messages

#18

Post by jolmg »

> 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.

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.

Re: Patch applies fake diffs from commit messages

#19

Post by Cpoll »

yjftsjthsd-h wrote: post #16
phoe-krk wrote: post #12

Earlier quoted context omitted.

If, by "readable by humans", you mean "it would reliably fool humans as well", I'd say it's an ambiguity bug regardless of whether it's "a core feature" or not. A patch format, human-readable or not, should clearly indicate which part is the commit message and which part is an actual diff; it's not the case here.

Alright, allow me to disambiguate in your preferred format. Patch is perfect. Ambiguity is good. There are no better formats for conveying patches. Patch files are readable by humans. Being readble by humans is useful. XML is painful for humans to read and write. JSON is painful for humans to read and write. JSON or XML would actually fix this problem in the format. The patch format could be improved. Formats should…

That's really not that bad, especially with indentation and color coding. You're kind of cheating by putting it into HN, which is terrible for code.

> XML is painful for humans to read and write.

Speaking of claims no-one made; no-one's talking about writing patch files by hand.

Re: Patch applies fake diffs from commit messages

#20

Post by bombcar »

Avamander wrote: post #14
LiamPowell wrote: post #2

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…

This is where I kind-of like the idea of PowerShell, it's just that I dislike almost all other aspects of it and around it.

Same - psh has one good idea and it’s this. The next evolution of shells needs to include it.
Post reply on HN