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