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
