Re: [HACKERS] commitfest management webapp

On Tue, May 26, 2009 at 10:15 PM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:
> On Tue, 26 May 2009, Robert Haas wrote:
>> I'm open to suggestions on how to improve this situation, though, because
>> it's definitely not ideal, and precludes things that reasonable people might
>> want to do, like "contact the guy who submitted this patch", "contact the
>> authors of all patches waiting for review", and similar.
>
> Since you're taking the message-id where the patch was submitted at as an
> input, couldn't you scrape this information out of the archives?  You
> probably want to do a bit of that regardless; having the program pull and
> display the author and subject line of the archived message is a good sanity
> check that you entered the message ID correctly.

I think something like this might work. I had a suggestion previously
of just checking that the message-IDs are even valid, which might be a
good place to start, and then I could try to figure out how to extend
it along these lines.

I'm not totally keen on pulling the subject lines. I know that's what
we've mostly been doing, but sometimes the subject line is something
like "patch to improve the way that foo does bar", rather than "make
bar use baz algorithm" or (even worse) "patch to add support for foo"
rather than "foo". Also, I think we may also want to assign each
patch a shortname that can be used as an argument to command-line
tools. I'd really like to be able to do something like this:

$ pgcf-patch foo

...and have foo.patch show up in $CWD. Even swankier would be to make
this integrate with git somehow.

>> I know that there are some of you reading this who may think that we
>> should convert to reviewboard or patchwork or some other system.  I can say
>> that personally I'm unimpressed by those suggestions because they will
>> almost certainly require process changes that this does not
>
> We used Reviewboard a fair amount here at Truviso for a while.  Lately a
> good chunk of that patch review has been happening more efficiently by
> passing pointers to private git branches around instead.  I think you're
> right to focus on just the review workflow and not the patch review itself,
> let people use whatever tools they're already comfortable with for that
> part.

Thanks and well said.

> I just spent a few minutes poking around your code and that quickly was able
> to see how things fit together, which is certainly not something I can say
> about Reviewboard etc.  The interface looks good and the code easy enough to
> improve.  The main concerns I'm left with after that review are with how to
> properly test the security of the code.  Some maturity there is one major
> thing that more packages in larger use have going for them vs. rolling your
> own in this sort of situation.

Well, the nice thing about it is that it's not a ton of code, so
visual inspection ought to buy you something. But I obviously can't
and don't promise that it is free of bugs, security-related or
otherwise. I was pretty dismayed when I realized that Template's "|
html" filter did not think apostrophes needed to be quoted, which they
obviously do if you're going to use them in a context like <a href='[%
foo | html %]'>. Now that's the one I caught - question is - what did
I miss?

...Robert