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_FAQI 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]https://github.com/macdice/cfbot/blob/master/cfbot_commitfest_rpc.py 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