On 5 November 2015 at 15:59, Torsten Zühlsdorff <mailingli...@toco-domains.de> wrote: > Hello, > >>> +1. FWIW, I'm willing to review some patches for this CommitFest, but >>> if the committers have to do first-round review as well as >>> committer-review of every patch in the CommitFest, this is going to be >>> long, ugly, and painful. We need to have a substantial pool of >>> non-committers involved in the review process so that at least some of >>> the work gets spread out. >> >> >> As a non-committer, let me offer my thoughts. >> >> First, I would ask that every patch include a commit id that the patch >> was generated against. > > + 1
Including a git ref in the commitfest entry helps a lot with that. So does using "git format-patch" when generating patches, now that everyone seems to have pretty much stopped caring about context diffs. >> Second, it would be convenient if there were a make target that would >> set up a test environment. Yes, a "make check NOSTOP=1" would be handy. That said, it's not hard to "make temp-install" then "PGPORT=whatever make installcheck" > From my point of view it is very hard to review a patch without having much > time. That's the eternal problem with patch review. I know the feeling. > My C knowledge is very very basic. I read many patches just to get > better with this, but as you (not) noticed, there is rarely feedback from > me. Reading C is useful for learning, but learned massively faster when I started writing extensions, etc. Then my reading tended to be more directed too, and with better retention. > Often it is unclear what to test. The threads about the features are very > long and mostly very technical. While interesting for a good C-programmer > this is not helpful for an end user. When there is a patch i am missing: > - a short description how to set up an test installation (just a link to the > wiki would be very helpful) Wiki links for patches would be a huge plus. The CF app "Links" section can be used for that, and should be more than it is. Peter Geoghegan did this with the insert ... on commit update patch, to useful effect. I try to include enough documentation directly in the patch - either in the commit message(s) from git format-patch, or in in-patch README text etc - for this not to be necessary. But for bigger patches it's hard to avoid, and I'd love it if more people did it. > Often terms, configuration options or syntax changes between the patches. > This is needed and very good - but currently requires me to read the > complete thread, which has many many information i do not understand because > of the missing inside. Especially when threads reference things that were discussed months or years prior! > I believe that we need to lower the barrier for testing. While I agree, I'd also like to note that formulaic testing is often of limited utility. Good testing still requires a significant investment of time and effort to understand the changes made by a patch, which areas need focused attention, think about corner cases, etc. "make check passes" doesn't really tell anyone that much. > I could even imagine to set up an open for everyone test-instance of HEAD > where users are allowed to test like the wanted. Than the barrier is reduced > to "connect to PostgreSQL and execute SQL". Gee, that'd be fun to host ;) More seriously, it's not HEAD that's of that much interest, it's HEAD + [some patch or set of patches]. There are systems that can pull in patchsets, build a project, and run it. But for something like PostgreSQL it'd be pretty hard to offer wide public access, given the trivial potential for abuse. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
