cjdb added a comment. In D148835#4288151 <https://reviews.llvm.org/D148835#4288151>, @erichkeane wrote:
> 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. Right, but Phab's precommit CI doesn't seem to be a blocker for merging, and often has false negatives, so I expect stuff to slip through :/ In D148835#4288248 <https://reviews.llvm.org/D148835#4288248>, @sbc100 wrote: > In D148835#4288151 <https://reviews.llvm.org/D148835#4288151>, @erichkeane > wrote: > >> 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. > > Yes, isn't this already covered by the existing use of `clang-format` on > upload to phabricator (`arc diff`) ? At least for me it always gives me > warnings if I try to upload anything that doesn't match clang-format. There are folks who have historically uploaded using the web UI specifically to ignore clang-format's suggestions, and `arc diff --nolint` will skip the formatting stage. 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
