patch granularity, extracting patches
Do we have or follow a standard on patch granularity? When I started, I picked up on the culture that we do small, logical commits -- as much as possible, each commit should be focused on a specific purpose. I've perceived some reviewers prefer to have all orthogonal changes (fix a random spelling error, remove a duplicate semicolon) extracted to other patches for clarity. Others don't seem to mind. I feel like I always want to ask, but I feel bad because it's a hassle. Also, when you are asked to extract something, do you have a trick to make it as easy as possible? Best wishes, Greg -- Greg Sheremeta, MBA Sr. Software Engineer Red Hat, Inc. gshereme@redhat.com
On Thu, Jun 29, 2017 at 6:16 PM, Greg Sheremeta <gshereme@redhat.com> wrote:
Do we have or follow a standard on patch granularity?
I don't think we have or should have any strict standard about this. Every reviewer is different and prefers different kind of patch granularity. For example I really dislike if someone break one logical work (like implement one small feature) into 10 smaller patches (all in the same area) to easy review. To me reviewing 10 smaller patches is much more work than review one bigger because I keep loosing the context and the bigger picture. In such cases I typically end up downloading the series locally and review in IDE. But than I loose the gerrit features like compare patchsets. Others, on the other hand, prefer to see tiny patches and that helps them to review. Ultimately, if you want to get a patch in, you need to know the culture and the people to know how to shape it. A rule of thumb: in VDSM break up the patches to as small as possible, in engine, break up into logical areas of expertise (e.g. DB, core, frontend).
When I started, I picked up on the culture that we do small, logical commits -- as much as possible, each commit should be focused on a specific purpose. I've perceived some reviewers prefer to have all orthogonal changes (fix a random spelling error, remove a duplicate semicolon) extracted to other patches for clarity.
From my perspective this is a complete waste of time. It does not bring any more clarity to the review nor to the git log. It may be aesthetically more appealing but brings much more work while babysitting the patches and does not bring anything of value. Also I feel it discourages people to fix issues like typo in comment when crossing it because they will have to create a separate patch, than either depend on it or have conflicts with it.
I personally prefer to follow the boy scout rule. On the other hand, some find it disturbing. That is fine, we are all humans, there is no one good option and this is not a technical question...
Others don't seem to mind. I feel like I always want to ask, but I feel bad because it's a hassle.
Contributing code is a way of communication with people. I don't think asking anything is a problem...
Also, when you are asked to extract something, do you have a trick to make it as easy as possible?
Best wishes, Greg
-- Greg Sheremeta, MBA Sr. Software Engineer Red Hat, Inc. gshereme@redhat.com
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
Hi, I think I have found a rule which applies to lot of CRs. If you submit set of small patches, you will be asked to squash them, and if you submit 'big bang' patch, you will be asked to split it. Sometimes, and that's the hardest situation, you are asked to do both :D I think you should lead with what is most effective for you to work with. And that ideally should be (for your own good) small commits with one (or some small number) logical change (rename this, add this) in local feature branch. That way you can easily remove some change if it's not needed in the end, you have patches ready for 'fine granularity reviewer', and if you need to provide big bang patch of 'bulk reviewer', you can create one in seconds. And potential 're-split' is easy too, you just squash them again differently. Squashing is easy, splitting is not. _warning_mine_opinion_: About patch size in general. It's like method / class names. If it's called 'perform' and have 100 LOC, it's most probably bad. Thus if your patch description does not list all the changes and/or its description is excessively long: "I did X and Y, then Z, while W, unless Q", then your patch is big, and kinda defies purpose of VCS. Next time you should write smaller one, and squash it to bulk patch only if reviewer insists. Also patch "rename methodA->methodB" takes no time to review(eyes are really good to see same patterns), with small probablility of error, and when we all use IDE for refactorings, there's no need to review it at all. Five renames and method extractions in one patch, garnished with added (even trivial) new functionality is significantly harder to review, and reviewer really needs to pay attention to every line change. M. On Thu, Jun 29, 2017 at 6:16 PM, Greg Sheremeta <gshereme@redhat.com> wrote:
Do we have or follow a standard on patch granularity? When I started, I picked up on the culture that we do small, logical commits -- as much as possible, each commit should be focused on a specific purpose. I've perceived some reviewers prefer to have all orthogonal changes (fix a random spelling error, remove a duplicate semicolon) extracted to other patches for clarity. Others don't seem to mind. I feel like I always want to ask, but I feel bad because it's a hassle.
Also, when you are asked to extract something, do you have a trick to make it as easy as possible?
Best wishes, Greg
-- Greg Sheremeta, MBA Sr. Software Engineer Red Hat, Inc. gshereme@redhat.com
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
On Thu, Jun 29, 2017 at 6:16 PM, Greg Sheremeta <gshereme@redhat.com> wrote:
Do we have or follow a standard on patch granularity?
As a matter of fact, we have. According to https://www.ovirt.org/develop/dev-process/devprocess/#sending-a-patch-for-wo... 1. Each commits contain a single logical change 2. Keep refactoring separate from bug fixes More details on above link.
When I started, I picked up on the culture that we do small, logical commits -- as much as possible, each commit should be focused on a specific purpose. I've perceived some reviewers prefer to have all orthogonal changes (fix a random spelling error, remove a duplicate semicolon) extracted to other patches for clarity. Others don't seem to mind. I feel like I always want to ask, but I feel bad because it's a hassle.
Also, when you are asked to extract something, do you have a trick to make it as easy as possible?
Best wishes, Greg
-- Greg Sheremeta, MBA Sr. Software Engineer Red Hat, Inc. gshereme@redhat.com
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- SANDRO BONAZZOLA ASSOCIATE MANAGER, SOFTWARE ENGINEERING, EMEA ENG VIRTUALIZATION R&D Red Hat EMEA <https://www.redhat.com/> <https://red.ht/sig> TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
participants (4)
-
Greg Sheremeta -
Martin Mucha -
Sandro Bonazzola -
Tomas Jelinek