Hi Alexey.

Thanks for the patches. I'll look into them.

Some comments below.

On Sat, Jan 31, 2009 at 3:21 AM, Alexey Morozov <morozov...@ngs.ru> wrote:

> When I first switched to LDAP based auth I discovered that I can't login
> as 'admin' anymore. Short investigation showed that the problem is in
> exception handling. django.contrib.auth.authenticate() (as of 1.0.2)
> catches
> only TypeErrors from a backend authenticate() so if another error occures
> (IndexError in my case) it simply stops processing of an authentication
> chain
> (in case of LDAP in reviewboard this chain is 'LDAP, builtin'). Probably
> the
> similar appoach should be applied to all auth backeds too.

Good to know. We'll certainly want to fix this for all backends.

> While submitting these two patches I also stepped into few other issues :)
> First I couldn't type certain letters (e.g. 'a' and 'A') into Summary and
> Description fields when diffviewer is active (e.g. on
> http://reviews.review-board.org/r/729/diff/ ) Probably these keys are
> intercepted as hotkeys by diffviewer component. I use Firefox-3.0.4 on
> Linux.

Sounds like a regression. I was sure we had this fixed. I'll take a look.
Would you mind filing a bug on this for tracking purposes?

> Also I found that submitting a patch is somewhat hard and cumbersome . The
> problem is how a patch is parsed upon upload. Since reviewboard repo type
> is
> SVN the patch processing machinery assumes that the patch is made by svn
> (or
> svntool), so it has additional fields in patch header, particularly file
> revision. This obviously isn't always true, suppose the patch is made
> against
> an installed file or it was made in another SCM (git in my case). I had to
> look through reviewboard code to understand what it expects to see in the
> patch and modify the patches accordingly.
> Maybe it's worth to re-arrange the whole process. I would propose smth like
> this:
> when a submitter uploads a patch he or she is presented with an interface
> which allows to choose a place (either a branch/tag or a single revision)
> where the patch should be applied. Since reviewboard knows exactly how to
> exctract this information from the repository this shouldn't be too hard to
> implement. Another option is to upload a modified file (or files) instead
> of
> a patch. Probably this interface should have SCM-specific parts (or
> callbacks) (yes I dream of git commits-based reviews :))
> Unfortunately reviewboard is the first django app I deal with so I can't
> implement it myself, at least right now.

It's a hard problem to solve, using diffs without the right info, because
the info is very much needed and having the user input it is extremely
error-prone and time-consuming. Asking for revision info wouldn't even work
in all cases. Imagine if someone just did a "diff -ur" on some files for a
CVS repository and then tried to upload it. CVS has per-file revisions, so
asking for the right revision on each thing would be insanely annoying.

Uploading modified files doesn't give enough information for proper diffs.
We really need that revision information and it needs to be correct.

The web interface is alright if you're dealing with SVN diffs with an SVN
repository, or CVS diffs with a CVS repository, but we recommend people
instead use post-review for all submissions.

The advantage of post-review is that you don't even have to care about any
of this info anymore. You just run "post-review" and it does the right
thing. It knows how to generate CVS diffs, SVN, Git, git-svn, Mercurial,
Mercurial-svn, Clearcase, Perforce...

So I'd strongly recommend looking into using that. It'll make your life a
lot easier :) We're slowly working with people to improve post-review
support, and to integrate it into IDEs.


Christian Hammond - chip...@chipx86.com
VMware, Inc.

You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to 
For more options, visit this group at 

Reply via email to