Re: [openstack-dev] Multiple patches in one review

[Mark McLoughlin]

On Mon, 2014-03-24 at 10:49 -0400, Russell Bryant wrote:
> Gerrit support for a patch series could certainly be better.

There has long been talking about gerrit getting "topic review"
functionality, whereby you could e.g. approve a whole series of patches
from a "topic view".

See:

  https://code.google.com/p/gerrit/issues/detail?id=51
  https://groups.google.com/d/msg/repo-discuss/5oRra_tLKMA/rxwU7pPAQE8J

My understanding is there's a fork of gerrit out there with this
functionality that some projects are using successfully.

Mark.



___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[Carl Baldwin]

+1 to all Ben said.

There are reasons to split things up in to a logical progression of
changes but each change must stand alone and must pass tests.

Carl

On Mon, Mar 24, 2014 at 10:03 AM, Ben Nemec  wrote:
> I should point out that Jenkins can't apply the next patch in sequence
> just to get tests passing.  What happens if the next patch never merges
> or has to be reverted?  Each commit needs to be able to pass tests using
> only the previous commits in the sequence.  If it relies on a subsequent
> commit then either the commits need to be reordered or, if there's a
> circular dependency, maybe those commits aren't logically separate and
> should just be squashed together.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[Ben Nemec]

On 2014-03-24 09:31, John Dennis wrote:
> When a change is complex good practice is to break the change into a
> series of smaller individual patches that show the individual
> incremental steps needed to get to the final goal. When partitioned into
> small steps each change is easier to review and hopefully illustrates
> the progression.
> 
> In most cases such a series of patches are interdependent and order
> dependent, jenkins cannot run tests on any patch unless the previous
> patch has been applied.
> 
> I was under the impression gerrit review supported multiple commits. In
> fact you can submit multiple commits with a single "git review" command.
> 
> But from that point forward it appears as if each commit is handled
> independently rather than being an ordered list of commits that are
> grouped together sharing a single review where their relationship is
> explicit. Also the jenkins tests either needs to apply all the commits
> in sequence and run the test or it needs to run the test after applying
> the next commit in the sequence.

I should point out that Jenkins can't apply the next patch in sequence
just to get tests passing.  What happens if the next patch never merges
or has to be reverted?  Each commit needs to be able to pass tests using
only the previous commits in the sequence.  If it relies on a subsequent
commit then either the commits need to be reordered or, if there's a
circular dependency, maybe those commits aren't logically separate and
should just be squashed together.

> 
> Can someone provide some explanation on how to handle this situation?
> 
> Or perhaps I'm just not understanding how the tools work when multiple
> commits are submitted.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[Russell Bryant]

On 03/24/2014 11:08 AM, John Griffith wrote:
> 
> On Mon, Mar 24, 2014 at 8:31 AM, John Dennis  > wrote:
> 
> When a change is complex good practice is to break the change into a
> series of smaller individual patches that show the individual
> incremental steps needed to get to the final goal. When partitioned into
> small steps each change is easier to review and hopefully illustrates
> the progression.
> 
> Definitely agree, however I've noticed people aren't necessarily very
> *good* about breaking these into logical pieces sometimes.  In other
> words it becomes random changes throughout multiple patches; in most
> cases it seems to be after-thoughts or just what the submitter managed
> to work on at the time.
> 
> Personally I'd love to see these be a bit more well thought out and
> organized for my own sake as a reviewer.  While we're at it (I realize
> this isn't the case you're talking about) I also would REALLY like to
> not see 5 individual patches all dependent on each other and all just
> changing one or two lines (I was seeing this quite a bit this cycle, and
> the only thing I can think of is perhaps it's developers getting some
> sort of points for number of commits).
> 

Some related good docs on splitting up changes:

https://wiki.openstack.org/wiki/GitCommitMessages

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[John Griffith]

On Mon, Mar 24, 2014 at 8:31 AM, John Dennis  wrote:

> When a change is complex good practice is to break the change into a
> series of smaller individual patches that show the individual
> incremental steps needed to get to the final goal. When partitioned into
> small steps each change is easier to review and hopefully illustrates
> the progression.
>
Definitely agree, however I've noticed people aren't necessarily very
*good* about breaking these into logical pieces sometimes.  In other words
it becomes random changes throughout multiple patches; in most cases it
seems to be after-thoughts or just what the submitter managed to work on at
the time.

Personally I'd love to see these be a bit more well thought out and
organized for my own sake as a reviewer.  While we're at it (I realize this
isn't the case you're talking about) I also would REALLY like to not see 5
individual patches all dependent on each other and all just changing one or
two lines (I was seeing this quite a bit this cycle, and the only thing I
can think of is perhaps it's developers getting some sort of points for
number of commits).

>
> In most cases such a series of patches are interdependent and order
> dependent, jenkins cannot run tests on any patch unless the previous
> patch has been applied.
>
> I was under the impression gerrit review supported multiple commits. In
> fact you can submit multiple commits with a single "git review" command.
>
> But from that point forward it appears as if each commit is handled
> independently rather than being an ordered list of commits that are
> grouped together sharing a single review where their relationship is
> explicit. Also the jenkins tests either needs to apply all the commits
> in sequence and run the test or it needs to run the test after applying
> the next commit in the sequence.
>
> Can someone provide some explanation on how to handle this situation?
>


> Or perhaps I'm just not understanding how the tools work when multiple
> commits are submitted.
>
> --
> John
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[Russell Bryant]

On 03/24/2014 10:31 AM, John Dennis wrote:
> When a change is complex good practice is to break the change into a
> series of smaller individual patches that show the individual
> incremental steps needed to get to the final goal. When partitioned into
> small steps each change is easier to review and hopefully illustrates
> the progression.
> 
> In most cases such a series of patches are interdependent and order
> dependent, jenkins cannot run tests on any patch unless the previous
> patch has been applied.
> 
> I was under the impression gerrit review supported multiple commits. In
> fact you can submit multiple commits with a single "git review" command.
> 
> But from that point forward it appears as if each commit is handled
> independently rather than being an ordered list of commits that are
> grouped together sharing a single review where their relationship is
> explicit. Also the jenkins tests either needs to apply all the commits
> in sequence and run the test or it needs to run the test after applying
> the next commit in the sequence.
> 
> Can someone provide some explanation on how to handle this situation?
> 
> Or perhaps I'm just not understanding how the tools work when multiple
> commits are submitted.

Gerrit support for a patch series could certainly be better.

When you push a series of dependent changes, each commit has its own
review, but the dependencies between them are still tracked.

As an example, take a look at this series of patches:

https://review.openstack.org/#/q/status:merged+project:openstack/nova+branch:master+topic:bp/admin-event-callback-api,n,z

Take a look at this patch from the middle:

https://review.openstack.org/#/c/74576/

There is a "Dependencies" section above the list of patch versions.
That's where you see the patches linked together.  Our other tools that
do testing and merging of changes respect these dependencies.  Changes
will only be tested with the other changes they depend on.  They will
also only be merged once all changes they depend on have been merged.

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] Multiple patches in one review

[Julien Danjou]

On Mon, Mar 24 2014, John Dennis wrote:

> But from that point forward it appears as if each commit is handled
> independently rather than being an ordered list of commits that are
> grouped together sharing a single review where their relationship is
> explicit. Also the jenkins tests either needs to apply all the commits
> in sequence and run the test or it needs to run the test after applying
> the next commit in the sequence.
>
> Can someone provide some explanation on how to handle this situation?
>
> Or perhaps I'm just not understanding how the tools work when multiple
> commits are submitted.

If a patch depends on another one, Jenkins is run with all the needed
patches applied.

-- 
Julien Danjou
;; Free Software hacker
;; http://julien.danjou.info


signature.asc
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] Multiple patches in one review

[John Dennis]

When a change is complex good practice is to break the change into a
series of smaller individual patches that show the individual
incremental steps needed to get to the final goal. When partitioned into
small steps each change is easier to review and hopefully illustrates
the progression.

In most cases such a series of patches are interdependent and order
dependent, jenkins cannot run tests on any patch unless the previous
patch has been applied.

I was under the impression gerrit review supported multiple commits. In
fact you can submit multiple commits with a single "git review" command.

But from that point forward it appears as if each commit is handled
independently rather than being an ordered list of commits that are
grouped together sharing a single review where their relationship is
explicit. Also the jenkins tests either needs to apply all the commits
in sequence and run the test or it needs to run the test after applying
the next commit in the sequence.

Can someone provide some explanation on how to handle this situation?

Or perhaps I'm just not understanding how the tools work when multiple
commits are submitted.

-- 
John

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev