[ovirt-devel] patch granularity, extracting patches

[Martin Mucha]

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 at 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 at redhat.com
>
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170630/80c38c20/attachment-0001.html>