Let's remove clang-tidy changes altogether. >>>> # Fix T82520: error building freestyle with Python3.8 >>>> # Cleanup: use preprocessor version check for PyTypeObject declaration
They were results of clang-tidy changes -> builds breaking -> fixing them -> clang-tidy builds breaking -> fixing them. Adding them felt logical at the time, not so much now. Several other commits are removed in D9986. Ankit > On 31-Dec-2020, at 21:59, Brecht Van Lommel via Bf-committers > <bf-committers@blender.org> wrote: > > On wiki.blender.org, click on Code Review in the sidebar. > > On Tue, Dec 29, 2020, 02:22 amdbcg via Bf-committers < > bf-committers@blender.org> wrote: > >> I'm new to this. Where is the procedure for code review? >> >> On Mon, Dec 28, 2020 at 6:06 PM Ray Molenkamp via Bf-committers < >> bf-committers@blender.org> wrote: >> >>> I'd prefer if we excluded small commits completely, but willing to >>> settle on clarifying that the "smaller" commits still ought to be >>> the result of automated tools, If there is no clear indication a >>> cleanup was automated the commit should not be added. ie a commit >>> "Cleanup: clang-tidy some-check" could still very much be a manual >>> cleanup of the warns exposed by clang-tidy and suspect to unintentional >>> functional changes. >>> >>> --Ray >>> >>> On 2020-12-28 5:01 p.m., Brecht Van Lommel via Bf-committers wrote: >>>> Hi Ankit, >>>> >>>> Please go through code review for all commits, unless you are a module >>>> owner or admin that is the policy. >>>> >>>> Note that there are guidelines in the file itself. We could document it >>> in >>>> the wiki too, but I'm not sure it's needed. Perhaps the wording can be >>>> clarified? Suggestions are welcome. The text is as follows: >>>> >>>> # Changes that belong here: >>>> # - Massive comment, doxy-sections, or spelling corrections. >>>> # - Clang-format, PEP8 or other automated changes which are *strictly* >>> "no >>>> functional change". >>>> # - Several smaller commits should be added to this list at once, >> because >>>> adding >>>> # one extra commit (to edit this file) after every small cleanup is >>> noisy. >>>> >>>> Note that only massive or automated changes are mentioned as belonging >>>> here, nothing else. And commits like these have functional changes: >>>> >>>> # Fix T82520: error building freestyle with Python3.8 >>>> # Build-system: Force C linkage for all DNA type headers >>>> # Cleanup: use preprocessor version check for PyTypeObject declaration >>>> >>>> So as far as I can tell this is just not following the documented >>>> guidelines. >>>> >>>> Thanks, >>>> Brecht. >>>> >>>> >>>> >>>> On Mon, Dec 28, 2020 at 8:48 PM Ankit via Bf-committers < >>>> bf-committers@blender.org> wrote: >>>> >>>>> Hello >>>>> I'm getting used to it. >>>>> I'll remove several commits soon, now that I have received the >>>>> feedback on the last commit, and will use stricter conditions in the >>>>> future. >>>>> >>>>> My premise >>>>> - for being lenient was that it saves other people from committing to >>>>> the file which keeps the noise in git log low. >>>>> - for renames was that a refactoring tool was used >>>>> with as little human intervention as possible. >>>>> - for clang-tidy being the auto-fixes feature that clang-tidy gives. >>>>> >>>>>> All, >>>>> That's a new way of raising concerns on commits. >>>>> >>>>>> but I really >>>>>> would still like to see smaller and clearly manual cleanups >>>>>> in git blame. >>>>> If I see "cleanup" in the title, the onus is on the committer to make >>>>> sure that it really is a cleanup. If that promise is kept, I don't see >>>>> why a cleanup commit interests you. >>>>> >>>>>> Changes in this file don't even seem to go through code review. >>>>> I thought it keeps the overhead of keeping a utility file low. >>>>> >>>>> Ankit >>>>> >>>>> >>>>>> On 29-Dec-2020, at 00:40, Ray Molenkamp via Bf-committers < >>>>> bf-committers@blender.org> wrote: >>>>>> All, >>>>>> >>>>>> What's going in with this file? there's 50+ commits in there >>>>>> and I disagree with virtually every single hash I sampled >>>>>> from it. >>>>>> >>>>>> If we want to hide large changes made by automated tools like >>>>>> the big clang-format [1] change, yeah awesome, but I really >>>>>> would still like to see smaller and clearly manual cleanups >>>>>> in git blame. >>>>>> >>>>>> Changes in this file don't even seem to go through code review. >>>>>> In the original review [2] both Brecht and Campbell mention >>>>>> that it be "OK for larger changes" but that is not what appears to >>>>>> be happening. >>>>>> >>>>>> This is not how this is supposed to work, is it? >>>>>> >>>>>> --Ray >>>>>> >>>>>> [1] >>>>> >>> https://developer.blender.org/rBe12c08e8d170b7ca40f204a5b0423c23a9fbc2c1 >>>>>> [2] https://developer.blender.org/D9234 >>>>>> _______________________________________________ >>>>>> Bf-committers mailing list >>>>>> Bf-committers@blender.org >>>>>> https://lists.blender.org/mailman/listinfo/bf-committers >>>>> _______________________________________________ >>>>> Bf-committers mailing list >>>>> Bf-committers@blender.org >>>>> https://lists.blender.org/mailman/listinfo/bf-committers >>>>> >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> Bf-committers@blender.org >>>> https://lists.blender.org/mailman/listinfo/bf-committers >>> _______________________________________________ >>> Bf-committers mailing list >>> Bf-committers@blender.org >>> https://lists.blender.org/mailman/listinfo/bf-committers >>> >> _______________________________________________ >> Bf-committers mailing list >> Bf-committers@blender.org >> https://lists.blender.org/mailman/listinfo/bf-committers >> > _______________________________________________ > Bf-committers mailing list > Bf-committers@blender.org > https://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list Bf-committers@blender.org https://lists.blender.org/mailman/listinfo/bf-committers