erichkeane added a comment. In D148835#4288148 <https://reviews.llvm.org/D148835#4288148>, @cjdb wrote:
> In D148835#4286924 <https://reviews.llvm.org/D148835#4286924>, @aaron.ballman > wrote: > >> In D148835#4286905 <https://reviews.llvm.org/D148835#4286905>, @erichkeane >> wrote: >> >>> In D148835#4284871 <https://reviews.llvm.org/D148835#4284871>, @cjdb wrote: >>> >>>> I've had some very good input about why this probably shouldn't go ahead: >>>> git history erasure :') >>> >>> While that is a common criticism, if we're going to do this at all, we >>> should do it in a single patch, as we can use --ignore-rev: >>> https://akrabat.com/ignoring-revisions-with-git-blame/ >>> >>> This is a pretty common thing to do, and I don't think it should prevent us >>> from doing this, which I think is the 'greater good' here. The alternative >>> is to continue having a bunch of unrelated patches piece-meal 'erase' git >>> history as people accidentally fix these. >> >> We already ignore blame revisions like this today: >> https://github.com/llvm/llvm-project/blob/main/.git-blame-ignore-revs > > Oh cool, I had no idea this was possible :) > >> However, I'd ask that we hold off on this change unless we have some tooling >> in place that rejects new additions of trailing whitespace, otherwise we're >> going to end up in this exact same situation again in another week or two. >> When CI starts failing on patches that insert *new* trailing whitespace, >> then I'm all for this change because we can fix it once and hopefully keep >> it fixed. > > Since Clang doesn't have pre-commit CI, how can we meaningfully progress here? Phab at least has Pre-commit CI, i would expect that to be sufficient? It would at least cut down accidential whitespace by orders of magnitude. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148835/new/ https://reviews.llvm.org/D148835 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
