Thread: Context diffs
Our developer FAQ mentions context diffs in several places for their
improved readability:
https://wiki.postgresql.org/wiki/Developer_FAQ
I also know git can product context diff as _output_. However, there
are several negatives to context vs unified diffs in our workflow:
* "git apply" and "git am" can't process context diffs (they throw an
error once a context-like section of the diff is hit; simple
adding/removing lines in a block works)
* the commit-fest doesn't recognized context diff attachments as
patches:
https://commitfest.postgresql.org/31/2912/
* cfbot can't process file renames/add/delete from context diffs
I am not 100% sure on the above items, but that is what I have seen in
my testing. Should we post only unified diffs in certain cases, even
with the readability issues? (I know some people prefer the readability
of unified diffs.) Should we document this?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Bruce Momjian <bruce@momjian.us> writes: > Our developer FAQ mentions context diffs in several places for their > improved readability: > https://wiki.postgresql.org/wiki/Developer_FAQ I think that's kind of out of date; AFAIK the majority of hackers think -u format is more readable. (For myself, I've gotten better at reading -u format, but I still find it illegible for complex changes. Still, I try to always post patches in -u format to defer to the majority.) It is true that some of the tools we use don't work right with anything except "git diff" output or close equivalents. regards, tom lane
On Mon, Jan 4, 2021 at 11:07 AM Bruce Momjian <bruce@momjian.us> wrote: > * "git apply" and "git am" can't process context diffs (they throw an > error once a context-like section of the diff is hit; simple > adding/removing lines in a block works) I think that this is the big issue for me. I really find it convenient to use "git format-patch" to produce patches for complicated long lived projects -- having commit messages and explicit ordering of multiple patches is really helpful. I've come to prefer unified, perhaps as a result of using "git format-patch" so much. -- Peter Geoghegan
On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian <bruce@momjian.us> wrote: > * "git apply" and "git am" can't process context diffs (they throw an > error once a context-like section of the diff is hit; simple > adding/removing lines in a block works) > > * the commit-fest doesn't recognized context diff attachments as > patches: > > https://commitfest.postgresql.org/31/2912/ > > * cfbot can't process file renames/add/delete from context diffs For the record, cfbot just uses plain old GNU patch, because that seems to accept nearly everything that anyone posts here (after a step that tries to unpack tarballs etc). Several people have suggested I change it to use git apply instead (IIRC it works better for patches containing binary files such as cryptographic keys?), but then it wouldn't accept ye olde context diffs.
On Tue, Jan 05, 2021 at 11:21:07AM +1300, Thomas Munro wrote: > On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian <bruce@momjian.us> wrote: > > * "git apply" and "git am" can't process context diffs (they throw an > > error once a context-like section of the diff is hit; simple > > adding/removing lines in a block works) > > > > * the commit-fest doesn't recognized context diff attachments as > > patches: > > > > https://commitfest.postgresql.org/31/2912/ > > > > * cfbot can't process file renames/add/delete from context diffs > > For the record, cfbot just uses plain old GNU patch, because that > seems to accept nearly everything that anyone posts here (after a step > that tries to unpack tarballs etc). Several people have suggested I > change it to use git apply instead (IIRC it works better for patches > containing binary files such as cryptographic keys?) It does work better for binary files, though there's little benefit in storing binary cryptographic keys as opposed to ASCII ones. Unfortunately for the cfbot, "git apply" forces the equivalent of "patch -F0", so it rejects patches needlessly. If you do change the cfbot, I recommend having it start with "git apply -3" (able to succeed when plain "git apply" fails), then fallback to "patch" when "git apply -3" fails.
On 1/4/21 5:21 PM, Thomas Munro wrote: > On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian <bruce@momjian.us> wrote: >> * "git apply" and "git am" can't process context diffs (they throw an >> error once a context-like section of the diff is hit; simple >> adding/removing lines in a block works) >> >> * the commit-fest doesn't recognized context diff attachments as >> patches: >> >> https://commitfest.postgresql.org/31/2912/ >> >> * cfbot can't process file renames/add/delete from context diffs > For the record, cfbot just uses plain old GNU patch, because that > seems to accept nearly everything that anyone posts here (after a step > that tries to unpack tarballs etc). Several people have suggested I > change it to use git apply instead (IIRC it works better for patches > containing binary files such as cryptographic keys?), but then it > wouldn't accept ye olde context diffs. > > I would try 'git apply' and fall back on 'patch' if it fails. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Mon, Jan 4, 2021 at 02:42:16PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Our developer FAQ mentions context diffs in several places for their > > improved readability: > > https://wiki.postgresql.org/wiki/Developer_FAQ > > I think that's kind of out of date; AFAIK the majority of hackers > think -u format is more readable. (For myself, I've gotten better > at reading -u format, but I still find it illegible for complex > changes. Still, I try to always post patches in -u format to > defer to the majority.) OK, I removed the mention of "context" diffs from the developer FAQ. I think I will produce unified diffs going forward, for the reasons mentioned. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Tue, Jan 5, 2021 at 11:21:07AM +1300, Thomas Munro wrote: > On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian <bruce@momjian.us> wrote: > > * "git apply" and "git am" can't process context diffs (they throw an > > error once a context-like section of the diff is hit; simple > > adding/removing lines in a block works) > > > > * the commit-fest doesn't recognized context diff attachments as > > patches: > > > > https://commitfest.postgresql.org/31/2912/ > > > > * cfbot can't process file renames/add/delete from context diffs > > For the record, cfbot just uses plain old GNU patch, because that > seems to accept nearly everything that anyone posts here (after a step > that tries to unpack tarballs etc). Several people have suggested I > change it to use git apply instead (IIRC it works better for patches > containing binary files such as cryptographic keys?), but then it > wouldn't accept ye olde context diffs. Does Windows also use 'patch'? I think I saw Windows behave differently for file additions. Does the commit-fest app and cfbot both have the same criteria for recognizing attachments as patches? I don't think they do. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Wed, Jan 6, 2021 at 6:58 AM Bruce Momjian <bruce@momjian.us> wrote: > On Tue, Jan 5, 2021 at 11:21:07AM +1300, Thomas Munro wrote: > > For the record, cfbot just uses plain old GNU patch, because that > > seems to accept nearly everything that anyone posts here (after a step > > that tries to unpack tarballs etc). Several people have suggested I > > change it to use git apply instead (IIRC it works better for patches > > containing binary files such as cryptographic keys?), but then it > > wouldn't accept ye olde context diffs. > > Does Windows also use 'patch'? I think I saw Windows behave differently > for file additions. Does the commit-fest app and cfbot both have the > same criteria for recognizing attachments as patches? I don't think > they do. If you're asking if cfbot uses 'patch' on Windows, then no, the work of finding and applying patches is done on a Un*x box (currently a FreeBSD box of mine, maybe soon a Debian box in postgresql.org orbit). Once that's done, it's pushed to a branch along with a CI control file, which causes the CI system(s) to wake up and process it. No, it doesn't use the CF app's patch recognition logic. It scrapes the CF app's main page periodically to find out about registered threads, and whenever it sees the "Latest mail" time change it reads the thread via the archive URL to find the latest message that has an attachment with a suffix matching \\.(diff|diff\\.gz|patch|patch\\.gz|tar\\.gz|tgz|tar\\.bz2). It would indeed be nice to integrate better, probably while switching from the current web scraping regexfest[1] to a proper JSON HTTP endpoint. [1] https://github.com/macdice/cfbot/blob/master/cfbot_commitfest_rpc.py
On 1/5/21 12:58 PM, Bruce Momjian wrote: > On Tue, Jan 5, 2021 at 11:21:07AM +1300, Thomas Munro wrote: >> On Tue, Jan 5, 2021 at 8:07 AM Bruce Momjian <bruce@momjian.us> wrote: >>> * "git apply" and "git am" can't process context diffs (they throw an >>> error once a context-like section of the diff is hit; simple >>> adding/removing lines in a block works) >>> >>> * the commit-fest doesn't recognized context diff attachments as >>> patches: >>> >>> https://commitfest.postgresql.org/31/2912/ >>> >>> * cfbot can't process file renames/add/delete from context diffs >> For the record, cfbot just uses plain old GNU patch, because that >> seems to accept nearly everything that anyone posts here (after a step >> that tries to unpack tarballs etc). Several people have suggested I >> change it to use git apply instead (IIRC it works better for patches >> containing binary files such as cryptographic keys?), but then it >> wouldn't accept ye olde context diffs. > Does Windows also use 'patch'? I think I saw Windows behave differently > for file additions. Does the commit-fest app and cfbot both have the > same criteria for recognizing attachments as patches? I don't think > they do. > Patch is available for windows both as a standard program under Msys and as a native tool via "choco install patch". But unless you're developing on Windows you would not need to bother about that at all. And if you are developing on Windows, just produce your patches using "git diff" or "git format-patch" just like on Unix. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Jan 6, 2021 at 10:21:22AM +1300, Thomas Munro wrote: > On Wed, Jan 6, 2021 at 6:58 AM Bruce Momjian <bruce@momjian.us> wrote: > > On Tue, Jan 5, 2021 at 11:21:07AM +1300, Thomas Munro wrote: > > > For the record, cfbot just uses plain old GNU patch, because that > > > seems to accept nearly everything that anyone posts here (after a step > > > that tries to unpack tarballs etc). Several people have suggested I > > > change it to use git apply instead (IIRC it works better for patches > > > containing binary files such as cryptographic keys?), but then it > > > wouldn't accept ye olde context diffs. > > > > Does Windows also use 'patch'? I think I saw Windows behave differently > > for file additions. Does the commit-fest app and cfbot both have the > > same criteria for recognizing attachments as patches? I don't think > > they do. > > If you're asking if cfbot uses 'patch' on Windows, then no, the work > of finding and applying patches is done on a Un*x box (currently a > FreeBSD box of mine, maybe soon a Debian box in postgresql.org orbit). > Once that's done, it's pushed to a branch along with a CI control > file, which causes the CI system(s) to wake up and process it. > > No, it doesn't use the CF app's patch recognition logic. It scrapes > the CF app's main page periodically to find out about registered > threads, and whenever it sees the "Latest mail" time change it reads > the thread via the archive URL to find the latest message that has an > attachment with a suffix matching > \\.(diff|diff\\.gz|patch|patch\\.gz|tar\\.gz|tgz|tar\\.bz2). It would > indeed be nice to integrate better, probably while switching from the > current web scraping regexfest[1] to a proper JSON HTTP endpoint. Offlist, thanks for the summary. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee