On 03/25/2013 05:04 PM, Christian Hammond wrote:
> Hi Stephen,
> Whitespace for indentation has never been shown for most files,
> regardless of that setting. That's just due to how we've always
> generated diffs.
> During diff generation, indentation changes are ignored. The intent was
> to prevent a block of indentation changes from showing up as a bunch of
> modified lines or, worse, deleted/added lines, distracting from the
> content of those reviews.

Sure, I understand that, but I would have figured that's what the "Hide
whitespace changes" button would have been for. As it stands right now,
those two buttons don't appear to do anything at all.

> All other whitespace changes are then let through (trailing whitespace,
> whitespace within the line), and lines containing just that are assigned
> a class. The "Show/Hide Extra Whitespace" toggle then just changes the
> visibility of any chunks containing these classes. It doesn't regenerate
> the diff or anything. The goal of this feature is to make code review
> easier when the file contains lots of whitespace cleanup, but not to
> show indentation.
> So, they're noticing this for the first time. It has never been any
> different, out of the box.

Well, this instance has only been up for about a week, so "for the first
time" happened pretty quickly.

> This can be disabled in an installation, though. You can include *.py
> files in "Show all whitespace for:" list in the Diff Viewer Settings in
> the Admin UI, and we'll stop ignoring leading whitespace on lines.

I couldn't get this to work for

I added "*.py, openlmi-doc-class2*" to the ignore list in the Diff
Viewer settings (and restarted the server), but it still does not
display leading whitespace changes on line 124 of that diff.

> Can this all be better? Yes. What I'd love is to have the diff
> generation be much smarter and say "This line hasn't changed except for
> the indentation," and then mark that up differently. There's a lot of
> work that would be needed, though. Something to put on the increasingly
> large TODO list.

The diff generation seems to be able to identify the specific characters
that have changed within a line other than whitespace, so I'm not sure
why we can't just use that same logic. But I'll admit to having no clear
view of the internal details here.

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