[Bf-committers] Simple steps to get an harmonious collaboration

[Campbell Barton]

On Fri, Jun 4, 2021 at 7:51 PM Sybren A. Stüvel via Bf-committers
<bf-committers at blender.org> wrote:
>
> On Wed, 2 Jun 2021 at 07:56, Campbell Barton via Bf-committers <
> bf-committers at blender.org> wrote:
>
> > 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.
> >
>
> I don't really see a problem here. Somebody needs to do that work anyway.
> This is typically done by the patch author if they have commit access, and
> a reviewer otherwise. As someone with too many unread emails from
> still-to-be-reviewed patches, I would prefer to not have to do these
> cleanups myself when I land a patch. Often such formatting & whitespace
> issues can be solved permanently by configuring the patch author's IDE to
> do this for them, after which all subsequent patches will be fine in this
> respect. To me that looks like a better solution than to accept
> badly-formatted patches. I know I'm polarizing the situation here, but I
> also feel that if one extra review iteration is so troublesome, we could
> also look at the source of that trouble and see if we can improve things.

I tend to agree with your position - that having people set up their
editors properly for Blender development is the way to go.
This works when we're already in the middle of short patch review iterations.

Even so, there are cases where I have accepted a patch, someone else
is set as a reviewer too. I get back to the patch when some weeks have
passed and the patch was accepted. Download and apply the patch
locally, double checking the issue the patch resolves still works as
expected, then I notice some trivial oversight.
At this point the overhead of adding an extra iteration, having to
remember to check back for their update, re-downloading and applying
the patch on a commit that I'm ready to push - doesn't seem like a
good use of time.

It's worth noting even developers with commit access sometimes make
these kinds of oversights too
(rather this be a separate discussion, just noting this is not *just*
an issue for beginner developers).

> For example, is `arc diff` too hard to use, and should we maybe look at
> some other tool to make reviewing easier? Or should we have an automatic
> cleanup on patches, so that they adhere to the coding standard?

+1 (at least to investigate).
If diverging from the patch authors local branch is a problem, some
automatic alert that their patch has some trivial issue would avoid us
having to do that manually.

> > 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.
> >
>
> You don't have to remember when you write it in the review comments.

I mean that I need to remember to check on the patch again in the future.
(side-note, the number of emails I get from phabricator a day on
differentials alone makes it not so useful to prioritize patch review)


> Sybren
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> https://lists.blender.org/mailman/listinfo/bf-committers



--
- Campbell