On Feb 15, 2013, at 1:26 PM, Matthew Woehlke <mwoehlke.fl...@gmail.com> wrote:

> On 2013-02-15 15:29, Christian Hammond wrote:
>> Yes, this is "correct" in that this is how it's known to work. It's not 
>> ideal, though. There are many things with interdiffs that could be improved, 
>> some easy to fix, some harder. This might not be so bad, if someone wanted 
>> to take it on. (I won't be able to get to it any time soon.)
> On a related note, is there a public list of things to improve (other than 
> issues floating around in the tracker)?

There's no public list beyond the bug tracker and some long-term goals we've 
mentioned on reviewboard-dev (like the DVCS work we're going to do).

> Something related that occurred to me would be to compute the range of 
> modified lines for each revision and throw out any (inter)diff hunks that 
> don't appear in at least one (mainly benefits revisions with different merge 
> bases).

This is something I would love to have.

> Drifting even further off topic, have you ever given thought to using 
> patience diff in RB? (I've seen spots where it would have been an 
> improvement... might help with better moved-lines detection also. For that 
> matter, what would be REALLY cool would be to compute common lines across the 
> entire patch, to get cross-file line move detection :-).)

We haven't looked into patience diff. It might be worth looking into, but I 
know my TODO list is too long to consider rewriting the diff generator with a 
new algorithm :)

We want to be able to extend move detection across files. There are plans for a 
set of enhancements to the diff viewer to add that, improve scalability, and a 
couple other things on our wish list, but it would likely be a for-pay add-on. 
It'll be a bit of work.

The reason it's not something we'll just be able to add today is that our diff 
generation is done on a per-file basis, one-by-one. You see this when loading 
the diff viewer. We need to lazy-generate because otherwise, large diffs would 
cause load times so long that they'd time out. We'd need to offload the diff 
scanning and generation to a background service, which requires more 
infrastructure than we'd like to require for a standard Review Board install.

> On another related note, moved-lines detection might benefit from doing 
> similarity comparison on adjacent lines, to find lines that were moved with 
> changes.

Moved line detection right now focuses only on lines that were actually moved 
and not those that changed. Adding some similarity checks would be interesting. 
Prone to error, but worth looking into when we can. We have to be careful right 
now to not increase diff generation times too much, and move detection's 
already pretty expensive. Backgrounding this task would give us more ability to 
do a more detailed analysis.

Some low-hanging fruit that I'd like to see, though, would be to also consider 
adjacent blank lines as part of a moved region when the blank lines are 
surrounded by moves.

> (There's lots of stuff - like this - that I'd actually love to work on if I 
> could justify doing it 'on the clock'…)

I know how you feel :) Review Board so far has been a spare-time effort. 
Looking to change that in the near future.


Christian Hammond - chip...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com

Want to help the Review Board project? Donate today at 
Happy user? Let us know at http://www.reviewboard.org/users/
To unsubscribe from this group, send email to 
For more options, visit this group at 
You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Reply via email to