On Wed, Sep 5, 2012 at 9:43 AM, Matthieu Monrocq <[email protected]> wrote: > > > On Tue, Sep 4, 2012 at 8:36 PM, David Blaikie <[email protected]> wrote: >> >> On Tue, Sep 4, 2012 at 10:18 AM, John McCall <[email protected]> wrote: >> > On Sep 4, 2012, at 9:42 AM, Chandler Carruth wrote: >> > >> > On Fri, Aug 31, 2012 at 2:45 PM, Joao Matos <[email protected]> >> > wrote: >> >> >> >> Author: triton >> >> Date: Fri Aug 31 13:45:21 2012 >> >> New Revision: 163013 >> >> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=163013&view=rev >> >> Log: >> >> Improved MSVC __interface support by adding first class support for it, >> >> instead of aliasing to "struct" which had some incorrect behaviour. >> >> Patch by >> >> David Robins. >> > >> > >> > For the record, I do not think this should have been committed. David >> > mailed >> > the patch and you reviewed it, but both of you are in the >> > 'commit-after-approval' group as far as I'm aware. This is clearly not >> > an >> > obvious patch, it as a huge extension to the Clang AST. >> > >> > John McCall reviewed previous versions of this patch and suggested the >> > changes that led to the current form. Why didn't you wait until he >> > approved >> > it? Was there some email that didn't make it to the list approving the >> > patch? (I know that email was getting dropped with earlier phases of the >> > review for that patch...) >> > >> > I don't think we can just revert this though because so many patches >> > have >> > gone in behind it. >> > >> > I can't even post-commit review this because several files have had >> > *all* >> > lines changed due to your client thrashing line endings!!! This is >> > really >> > terrible. I know we already discussed this some, but let me re-iterate: >> > you >> > must not submit patches with thrashed line endings like this. >> > >> > >> > A whitespace-insensitive diff will work, although of course that's not >> > the >> > default for most tools, and it's definitely going to complicate git >> > blames >> > forever on these files. >> >> (pedantry that seems to warrant being pointed out every time this comes >> up: >> git blame -w >> (& not entirely portable: svn blame -x -w)) >> >> Though that still leaves our viewvc providing less-than-ideal blame. >> > > Isn't there an option in the viewvc configuration to address that ? I would > be surprised if the command line offered it and the viewer could not, but it > may not be implemented :x
Seems there is a server-side config to just make all diffs whitespace ignorant (hr_ignore_white) & also someone provided a patch/filed a bug on viewvc to make it a user-configurable option from the web UI: http://viewvc.tigris.org/issues/show_bug.cgi?id=492 > > -- Matthieu > >> >> > >> > John, thoughts on how to handle this? How can you effectively review it? >> > >> > >> > I can offer to back out the entire sequence of patches and essentially >> > return us to before this patch went in, and then perhaps we can get >> > meaningful diffs that you can review? >> > >> > >> > Ugh. Unfortunately, that will just make naive tools say that *you* made >> > all >> > the changes (only due to svn's interference — IIRC pure git can preserve >> > the >> > authorship / intent history, but...), which is actively worse. I think >> > we >> > just live with it. >> > >> > Joao, I'm sure it's frustrating to not have things reviewed as quickly >> > as >> > you'd like, but committing without approval is not a way to achieve >> > this. >> > You pushed the envelope a bit before; if you continue this pattern, we >> > may >> > have to revoke your commit privileges, which obviously we'd prefer not >> > to >> > do. >> > >> > I'll try to review this quickly. >> > >> > John. >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
