[openstack-dev] [fuel] Unrelated changes in patches

Igor Kalnitsky ikalnitsky at mirantis.com
Mon Apr 4 15:15:06 UTC 2016


Dmitry Guryanov wrote:
> It's often not so easy to decide, if you should include some unrelated
> changes to your patch, like fixing spaces, renaming variables or
> something else, which don't change logic.

I'd say it depends. If, for example, variable name is used inside one
function - it's ok to rename it within a patch. On the other hand, if
this variable is used across the code and it requires to change it in
few places - I'd prefer to do not do it within the patch. Any
unrelated change complicates review (if we're taking about thorough
review).

The things go worse when patch authors tries to implement two business
changes in one patch. In that case, it's really hard to distinguish
those both changes one from another in order to understand what's
going on.

So generally I'd prefer to see all unrelated changes in separate
patches. It's not necessary to create a bug for them, it's ok to
submit them with detailed commit message why this should be done.

Dmitry Guryanov wrote:
> On the one hand you see something's wrong with the code and you'd like
> to fix it, on the other hand reviewers can vote of -1 and you'll have
> to fix you patch and upload it again and this is very annoying.

You can fix it in first patch, and make *business* changes in the second one.

Dmitry Guryanov wrote:
> You can also create separate review for such changes, but it will
> require additional effort from you and reviewers.

As reviewer I can say: I'm spending more time trying to figure out
what's going on in patch that changes two (or even more) unrelated
things, than I'd spend if I review those changes in independent
patches.

On Mon, Apr 4, 2016 at 3:46 PM, Dmitry Guryanov <dguryanov at mirantis.com> wrote:
> Hello, colleagues!
>
> It's often not so easy to decide, if you should include some unrelated
> changes to your patch, like fixing spaces, renaming variables or something
> else, which don't change logic. On the one hand you see something's wrong
> with the code and you'd like to fix it, on the other hand reviewers can vote
> of -1 and you'll have to fix you patch and upload it again and this is very
> annoying. You can also create separate review for such changes, but it will
> require additional effort from you and reviewers.
>
> If you are a reviewer, and you've noted unrelated changes you may hesitate,
> if you should ask an author to remove them and upload new version of the
> patch or not. Also such extra changes may confuse you sometimes.
>
> So I suggest creating separate patches for unrelated changes if they add new
> chucks to patch. And I'd like to ask authors to clearly state in the subject
> of a commit message, that this patch just fixes formatting. And reviewers
> shouldn't check such patches too severely, so that they'll get into repo as
> soon as possible.
>
> What do you think?
>
>
> --
> Dmitry Guryanov
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



More information about the OpenStack-dev mailing list