Hi, there are a few cases this document doesn't cover (understandably as they're not so typical).
How to best make updates to a patch? ------------------------------------ While proposing changes and having the patch author apply them is almost always preferred, there are times when it is less trouble to make the changes to the code directly. Options include: - Make a patch on top of their patch, submit the raw diff as a paste. (it can be awkward to manually apply patches). - Create a temporary branch and reference that, the patch author needs to manually checkout and apply the patch. (it can be awkward to download temporary branches, we need to remember to delete them afterwards). - Commandeer the patch and update it, allowing time for the original author and others to review the changes. Note that these situations are times when I'd often prefer to collaborate on a branch to prepare a change for final review. However it's not always practical, especially if the developer doesn't have commit access. In this situation github/gitlab's model works has an advantage as I'm able to submit a pull-request to their repository. NOTE: Phabricator already has support for proposing changes to a single block of code, assuming this feature isn't practical (for larger changes in multiple areas/files for example). How to handle superficial updates to a patch? --------------------------------------------- +1 for having diff's match the final commit. If the proposed policies are followed to the letter, we end up in situations where extra review iterations would be required for running clang-format, stripping what-space and spelling mistakes. Or where patches are applied with known superficial issues that need to have separate clean up commits applied afterwards (adding more noisy commits). An argument can be made that having patch authors get these kinds of details right is something we should expect from them. Not something to resolve on their behalf. When the issues are trivial though (trailing white-space) it adds some burden on the patch reviewer to have to remember that some patch which was otherwise fine - needs an extra iteration. When to commandeer a patch? --------------------------- Occasionally there are older patches that can be reviewed and applied with only minor changes even if those changes are to resolve merge conflicts. A one-size-fits-all policy for the situation is to give the original author some *reasonable* amount of time to respond, only commandeering the patch and updating if they don't reply or are unable to update the patch. In the case of opinionated/functional changes this makes sense. In the case of updating the patch to resolve conflicts, this doesn't seem necessary or useful. On Tue, Jun 1, 2021 at 9:05 PM Sergey Sharybin via Bf-committers <bf-committers@blender.org> wrote: > > Hi, > > Just a quick note. The bf-admin team worked on several process related > documents to ensure a pleasant and efficient development process. > > Today we've updated wiki with the patch review process: > https://wiki.blender.org/wiki/Process/Patch_Review > > Feedback is welcome. > > Best regards, > - Sergey - > -------------------------------------------------------------------- > Sergey Sharybin - ser...@blender.org - www.blender.org > Principal Software Engineer, Blender > Buikslotermeerplein 161, 1025 ET Amsterdam, the Netherlands > _______________________________________________ > Bf-committers mailing list > Bf-committers@blender.org > https://lists.blender.org/mailman/listinfo/bf-committers -- - Campbell _______________________________________________ Bf-committers mailing list Bf-committers@blender.org https://lists.blender.org/mailman/listinfo/bf-committers
