Re: Patch review

Gregory Stark wrote:
> "Stefan Kaltenbrunner" <stefan(at)kaltenbrunner(dot)cc> writes:
>
>> Tom Lane wrote:
>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>> Not sure if this is platform-specific, but I keep being annoyed by not being
>>>> able to actually *view* the patches in the queue. I have to download them,
>>>> and then open them separately. I can't just view them in the browser,
>>>> because they're all named ".bin" and come out as mime type
>>>> "application/octet-stream".
>>> Yeah, it happens that way for me too. The other huge, huge problem with
>>> it is the lack of stability of URLs for the items in the list, which
>>> makes it difficult to identify which item you're talking about.
>> yeah that is fairly annoying :-(
>
> Strangely enough I have the opposite complaint. Sometimes the patch is
> displayed inline (seems to be pretty unpredictable) and that makes it hard to
> download and apply. If you download the page you have to un-html-escape it and
> if you copy-paste the contents the whitespace is messed up.
>
> It would be nice if the patches list were fed into reviewboard or viewcvs or
> something like it. Even if we only used it for viewing patches it would at
> least make that nice.

hmm that could be made to work but why not submit them to reviewboard in
the first place and let it send an mail to the list ?

>
> I couldn't see how to make it work with reviewboard at first glance. It seems
> to assume you're starting form a working tree and wants to generate the diff
> and send it in itself. If you generate it yourself then you have to tell it
> precisely what it's a diff against. It doesn't seem to expect to be used as a
> general purpose patch viewer.

yeah - I think that dave was looking into checking if something like
that would be possible.

>
>>> Personally I'd be happier with an editable wiki page consisting of links
>>> to the original messages in the mail list archives, plus free-format
>>> annotations (such as status). This should be trivial to set up and
>>> reasonably flexible. With experience we might find we need something
>>> fancier, but let's not overdesign our solution at the start.
>> I will see if I can come up with a proposal on the developer wiki for a list
>> that looks like that tomorrow.
>
> How powerful are these wiki things? Can you write a script which automatically
> inserts a new line in a table in the wiki whenever an email to the patches
> list containing a patch comes in?

yes

>
> Can you add buttons on the wiki itself to do common actions like removing the
> item from the table or joining it up with another item? That way we would only
> have to actually edit the wiki content if we have something substantive to add
> such as review comments.

not sure on that one

>
>> _IF_ we are looking for an (over)designed solution
>
> I'm against overdesigned solutions. If we want more than a wiki I think it
> would be reviewboard. I think something which fills a specific hole like
> reviewboard is much more likely to work in the long term than an overarching
> solution whose problem description is as vague as a "tracker". As soon as our
> process changes the "tracker" will be obsolete whereas a problem-specific
> solution like reviewboard will always be useful as long as there are patches
> to review.

well the trackerdemo installation could do something very similiar to
what you are describing (I only considered that process for -bugs up to
know but it could be adapted to patches I guess).

* change the bugreporting for to one that posts to the tracker and have
it generate the (bug)id
* subscribe the tracker to the list (say pgsql-bugs)
* have people discuss stuff on either the list or via the GUI of the
tracker - the tracker will be able automatically track the mails and add
them as comments to the "bug" or not
* have that tracker generate a short summary report based on "open"
issues on either the wiki (plugins are available for that) or use one of
the integrated reporting features

Has anybody actually looked into how suitable reviewboard is for
reviewing large patches ? it seems to have some limitations based on the
observation that patches are only commented on but are not expected to
be changed/modified by the reviewer (in fact I think the reviewer cannot
even upload a modified version of the patch but I could be wrong there).

Stefan