Re: curious inter-diff bug?
On 2013-02-16 02:43, Christian Hammond wrote: On Feb 15, 2013, at 1:26 PM, Matthew Woehlke mwoehlke.fl...@gmail.com wrote: 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 :) In my experience with git, patience diff generates much more reasonable diffs in many cases. I would strongly encourage you to at least do a web search and read one or more articles on the topic. I wonder if you aren't already doing part of the work (finding common lines) for line move detection... (or if the way patience does line matching might be more efficient that what line move detection currently does...) We want to be able to extend move detection across files. [...] 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. For moved-line detection, do you at least scan the diff itself and not the patched files? (Since unmodified lines obviously will not have moved, this should be reasonably efficient except on reasonable sized diffs.) For that matter, if performance is a problem, it may be a good idea to turn off more expensive features for very large diffs. -- Matthew -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
On Saturday, February 16, 2013 1:13:02 PM UTC+5:30, Christian Hammond wrote: On Feb 15, 2013, at 1:26 PM, Matthew Woehlke mwoehlk...@gmail.comjavascript: 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). It would help a lot if the long-term goals and wanted-features-list (like a roadmap) were put in a wiki on code.google.com. For folks who want to contribute, it might be a good list to work on. Thanks Raja -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
On Sat, Feb 16, 2013 at 12:43 AM, Raja rajas...@gmail.com wrote: On Saturday, February 16, 2013 1:13:02 PM UTC+5:30, Christian Hammond wrote: On Feb 15, 2013, at 1:26 PM, Matthew Woehlke mwoehlk...@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). It would help a lot if the long-term goals and wanted-features-list (like a roadmap) were put in a wiki on code.google.com. For folks who want to contribute, it might be a good list to work on. I agree. Though we've decided against continuing with code.google.com for a wiki. So here's a roadmap I just put on hackpad.com: https://hackpad.com/Roadmap-MAZA9992jv5 Subject to change, but should give you an idea of the general direction we're going, and some of the bigger tasks. Christian -- 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 http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
curious inter-diff bug?
Let's say I upload a patch that changes two files: foo.cpp and bar.cpp, and publish it. Now let's say I upload a new version of the patch that only changes bar.cpp. When I ask RB to show the diff between the two request versions, I see changes to foo.cpp as if I was looking at revision 1. Is this correct/expected? Known issue? I would have expected to see a reverse of the changes to foo.cpp from revision 1. (This is with RB 1.7.5. Also, the merge base between revisions 1 and 2 differs, which may be part of the trouble.) -- Matthew -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
Yes, this is a known issue (filed as bug 1486). Thanks, -David On Fri, Feb 15, 2013 at 8:54 AM, Matthew Woehlke mwoehlke.fl...@gmail.comwrote: Let's say I upload a patch that changes two files: foo.cpp and bar.cpp, and publish it. Now let's say I upload a new version of the patch that only changes bar.cpp. When I ask RB to show the diff between the two request versions, I see changes to foo.cpp as if I was looking at revision 1. Is this correct/expected? Known issue? I would have expected to see a reverse of the changes to foo.cpp from revision 1. (This is with RB 1.7.5. Also, the merge base between revisions 1 and 2 differs, which may be part of the trouble.) -- Matthew -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/**donate/ http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/**users/http://www.reviewboard.org/users/ -~--~~~~--**~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscribe@** googlegroups.com reviewboard%2bunsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/** group/reviewboard?hl=en http://groups.google.com/group/reviewboard?hl=en --- 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+unsubscribe@**googlegroups.comreviewboard%2bunsubscr...@googlegroups.com . For more options, visit https://groups.google.com/**groups/opt_outhttps://groups.google.com/groups/opt_out . -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
Hi, 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.) Christian On Feb 15, 2013, at 8:54, Matthew Woehlke mwoehlke.fl...@gmail.com wrote: Let's say I upload a patch that changes two files: foo.cpp and bar.cpp, and publish it. Now let's say I upload a new version of the patch that only changes bar.cpp. When I ask RB to show the diff between the two request versions, I see changes to foo.cpp as if I was looking at revision 1. Is this correct/expected? Known issue? I would have expected to see a reverse of the changes to foo.cpp from revision 1. (This is with RB 1.7.5. Also, the merge base between revisions 1 and 2 differs, which may be part of the trouble.) -- Matthew -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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. -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
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)? 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). 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 :-).) On another related note, moved-lines detection might benefit from doing similarity comparison on adjacent lines, to find lines that were moved with changes. (There's lots of stuff - like this - that I'd actually love to work on if I could justify doing it 'on the clock'...) -- Matthew -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
Re: curious inter-diff bug?
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 -- 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 http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- 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.
