Gerrit Topic Review - Take two?

[Chris Harris]

Some of you may be aware that Deniz Turkoglu and I have been working on getting topic review support upstream. We have been trying to revive the work developed by Codethink and have rebased/rewritten several patches in the sequence:

https://gerrit-review.googlesource.com/#/q/owner:%22Pete+Fotheringham%22+status:open,n,z

We at Kitware have been using this functionality for some time now on various projects.

However, it seems that the general consensus is that this topic is too invasive/large and is unlikely to ever to be upstreamed. Martin Fick suggested rethinking the support in a way that will enable more of the existing change functionality to be reused rather than creating a new reviewable entity.

So with this in mind, my colleague Brad King and I have come up with a possible alternative (see below). We would very much appreciate feedback/comments/thoughts. We would like to try to determine if the community feels this is a viable solution before investing any time in implementation.

Many Thanks, 

Chris Harris

Definitions/Background

  - A “Topic” allows a group of one or more Changes to be reviewed together.

  - Each Topic has an external “Topic Name” for reference by humans and during pushes.

  - Each Topic has an internal “Topic-Id” representing the topic as a whole.

  - Only one Topic with a given Topic Name may be open at any one time.

      - Each open Topic Name will be mapped to its internal Topic-Id.

      - Once merged/abandoned the name can be reused and will get a new id.

  - A “Topic Rev” is one revision of a Topic, defined by its head commit and merge base(s), and conceptually equivalent to a Patch Set in a normal Change.

  - Topics can overlap i.e. a Change can appear in multiple open topics.

  - A Topic that contains an out-of-date Patch Set of a Change cannot be Submitted.

Approach

In order to reuse existing infrastructure for review, Gerrit will construct a Change to represent a Topic. For example, when a user pushes a topic branch as

 git push gerrit HEAD:refs/for/master/$topic_name

Gerrit will lookup/generate its Topic-Id and construct a commit equivalent to

 git commit-tree $topic_head^{tree} -p $topic_base

where $topic_head is the head commit of the topic and $topic_base is the merge base (or bases) against master.  The committer will be the configured “Code Review” user and the author will be the uploader.  The commit message will store meta data about the topic e.g.

 Topic

 (blank line)

 Topic-Id: $topic_id

 Topic-Rev: $topic_rev

 Topic-Head: $topic_head

 Topic-Name: $topic_name

where $topic_id is the Topic-Id and $topic_rev is the Topic Rev number.  Each push to a new or active Topic will construct a new Topic Rev and add a reference to it as

 refs/topics/$topic_id/1

 refs/topics/$topic_id/2

 refs/topics/$topic_id/3

 … 

The constructed “topic changes” in most senses will be treated as changes like any other, they would appear in dashboard etc. However, the rendering of the change page for a Topic will be a bit different:

  - The raw meta data commit message will not be rendered.  Instead a commit message will be generated that looks a lot like what might be generated when the topic is merged e.g. a list of commits in the topic as “%h %s” lines.

  - Each revision of the topic will be shown in a “Topic Rev $N” block similar to a normal “Patch Set $N” block.

  - Each “Topic Rev” block will list the changes associated with the Topic as links to the individual Patch Sets on their corresponding Change pages.

  - Each “Topic Rev” block will also show the normal file-wise diff computed from the artificial topic commit to show the net change.  It will be reviewable with line-wise comments like any other Patch Set.

The “topic change” can be review and merged at which point Gerrit would then also label the changes associated with it as merged.

[Chris Harris]

On Thu, Dec 13, 2012 at 7:01 PM, Sergio Ahumada <s...@sansano.inf.utfsm.cl> wrote:

Definitions/Background

   - A “Topic” allows a group of one or more Changes to be reviewed
together.
   - Each Topic has an external “Topic Name” for reference by humans and
during pushes.
   - Each Topic has an internal “Topic-Id” representing the topic as a
whole.
   - Only one Topic with a given Topic Name may be open at any one time.
       - Each open Topic Name will be mapped to its internal Topic-Id.
       - Once merged/abandoned the name can be reused and will get a new id.
   - A “Topic Rev” is one revision of a Topic, defined by its head
commit and merge base(s), and conceptually equivalent to a Patch Set in
a normal Change.

should a "Topic Rev" have always the same amount of changes ? can you add a new change in "Topic Rev 2" ? can you remove a change in "Topic Rev 3" ?

The number of changes in a Topic Rev could change. If a new copy of the topic branch is pushed without a particular or a change added these would then disappear/appear.  

 

   - Topics can overlap i.e. a Change can appear in multiple open topics.

could you please elaborate a bit on how this should work ?

if a change X that appears in both topic A and topic B gets merged with topic A, does it disappear from topic B ? or did I miss something ?

Unless you pushed a new version of the topic branch with a new merge base the change X would still appear in topic B but it would be marked as merged. If you rebases on master and then pushed a new version of the topics they would be removed  as merge base would have changed.

 

will a change belonging to a topic be rendered differently ? how will a change belonging to more than one topic be rendered ?

 

The plan would be to add links to the topics that a change belongs to. So the topic field in the change view might get replaced with a list of topics. 

 

   - A Topic that contains an out-of-date Patch Set of a Change cannot
be Submitted.

is this because of a change can belong to more than one topic ?

 

Correct, your can only merge head commit of a change.

 

 

The constructed “topic changes” in most senses will be treated as
changes like any other, they would appear in dashboard etc. However, the
rendering of the change page for a Topic will be a bit different:
   - The raw meta data commit message will not be rendered.  Instead a
commit message will be generated that looks a lot like what might be
generated when the topic is merged e.g. a list of commits in the topic
as “%h %s” lines.
   - Each revision of the topic will be shown in a “Topic Rev $N” block
similar to a normal “Patch Set $N” block.
   - Each “Topic Rev” block will list the changes associated with the
Topic as links to the individual Patch Sets on their corresponding
Change pages.
   - Each “Topic Rev” block will also show the normal file-wise diff
computed from the artificial topic commit to show the net change.  It
will be reviewable with line-wise comments like any other Patch Set.

will you render it similar to https://codereview.qt-project.org/#/t/56/ ?

 

That is actually the topic support developed by Codethink that we are trying to get upstreamed, I didn't know Qt where still using it :-)  Yes, the rendering would  be similar, but there would also be a file-wise diff for the artificial topic commit.

The “topic change” can be review and merged at which point Gerrit would
then also label the changes associated with it as merged.

Why would you like to review the topic ? Isn't enough that all the changes are reviewed to merge the topic ?

 

The unit of review here is the topic, individual changes can be review but its review of topic that would indicate the readiness to merge.

 

Cheers,
--
Sergio Ahumada
s...@sansano.inf.utfsm.cl

[Martin Fick]

Chris,

I appreciate your efforts to rethink things.

Your proposal below discusses technical details in light of
your vision of topics, but these pieces only seem to make
sense in the context of a larger solution. But we don't
share the big vision to this larger solution, so discussing
these details still seems premature. We don't share the big
vision because we don't understand what is wrong with the
current way that things are done in Gerrit today. We need
help, we are set in our ways. Make us feel your pain, share
a problem with us that we might agree is painful.

What we want is to see how today's Gerrit workflow is missing
pieces. We want to see small problems and small solutions
(not small pieces to a bigger solution). An example small
problem might be: "It is hard to navigate amongst changes
for a related topic", and then, a discussion about how can we
improve navigation amongst changes could ensue?

Edwin asked you earlier why using Gerrit's already existing
branching support does not work? It seems like a reasonable
question to me. If that approach does not work well for you,
could you describe why not? Could you pick one small thing
about such a workflow that doesn't work well and that could
use improvement?

-Martin



On Thursday, December 13, 2012 03:53:22 pm Chris Harris
wrote:

[Pursehouse, David]

Martin, is this not quite similar to the "superchange" feature that we (you, I, Gustaf, etc) discussed during the Hackathon?

If that's the case, it's probably something we want to look into a bit more.

One concern I have about the proposal below is that it changes the behavior of the existing topic name feature. The way topics are used now on gerrit-review, and in the workflow at my company (and most likely others), not all changes on the same topic are so tightly related that they must be reviewed and merged at the same time. A good example of this is the changes on the "docs" topic [1] - the changes currently open on that are not really related beyond the fact that they are documentation updates.

That concern might be addressed by the "change label" feature [2] though...


[1] https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit+branch:master+topic:docs,n,z
[2] https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit+branch:master+topic:edit-labels-in-change-screen,n,z

> ...

>
> The constructed "topic changes" in most senses will be treated as
> changes like any other, they would appear in dashboard etc. However,
> the rendering of the change page for a Topic will be a bit different:
> - The raw meta data commit message will not be rendered. Instead a
> commit message will be generated that looks a lot like what might be
> generated when the topic is merged e.g. a list of commits in the topic
> as "%h %s" lines.
> - Each revision of the topic will be shown in a "Topic Rev $N" block
> similar to a normal "Patch Set $N" block.
> - Each "Topic Rev" block will list the changes associated with the
> Topic as links to the individual Patch Sets on their corresponding
> Change pages. - Each "Topic Rev" block will also show the normal
> file-wise diff computed from the artificial topic commit to show the
> net change. It will be reviewable with line-wise comments like any
> other Patch Set.
>
> The "topic change" can be review and merged at which point Gerrit
> would then also label the changes associated with it as merged.

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Martin Fick]

On Thursday, December 13, 2012 06:39:39 pm Pursehouse, David
wrote:

> Martin, is this not quite similar to the "superchange"
> feature that we (you, I, Gustaf, etc) discussed during
> the Hackathon?

It shares some similarities, mainly that it is about dealing
with more than one change at a time. However, I suspect that
the similarities stop there,

-Martin

[Sergio Ahumada]

>
> Definitions/Background
>
> - A �Topic� allows a group of one or more Changes to be reviewed
> together.
> - Each Topic has an external �Topic Name� for reference by humans and
> during pushes.
> - Each Topic has an internal �Topic-Id� representing the topic as a

> whole.
> - Only one Topic with a given Topic Name may be open at any one time.
> - Each open Topic Name will be mapped to its internal Topic-Id.
> - Once merged/abandoned the name can be reused and will get a new id.

> - A �Topic Rev� is one revision of a Topic, defined by its head

> commit and merge base(s), and conceptually equivalent to a Patch Set in
> a normal Change.

should a "Topic Rev" have always the same amount of changes ? can you
add a new change in "Topic Rev 2" ? can you remove a change in "Topic
Rev 3" ?

> - Topics can overlap i.e. a Change can appear in multiple open topics.

could you please elaborate a bit on how this should work ?

if a change X that appears in both topic A and topic B gets merged with
topic A, does it disappear from topic B ? or did I miss something ?

will a change belonging to a topic be rendered differently ? how will a
change belonging to more than one topic be rendered ?

> - A Topic that contains an out-of-date Patch Set of a Change cannot
> be Submitted.

is this because of a change can belong to more than one topic ?

>

> The constructed �topic changes� in most senses will be treated as

> changes like any other, they would appear in dashboard etc. However, the
> rendering of the change page for a Topic will be a bit different:
> - The raw meta data commit message will not be rendered. Instead a
> commit message will be generated that looks a lot like what might be
> generated when the topic is merged e.g. a list of commits in the topic

> as �%h %s� lines.
> - Each revision of the topic will be shown in a �Topic Rev $N� block
> similar to a normal �Patch Set $N� block.
> - Each �Topic Rev� block will list the changes associated with the

> Topic as links to the individual Patch Sets on their corresponding
> Change pages.

> - Each �Topic Rev� block will also show the normal file-wise diff

> computed from the artificial topic commit to show the net change. It
> will be reviewable with line-wise comments like any other Patch Set.

will you render it similar to https://codereview.qt-project.org/#/t/56/ ?

> The �topic change� can be review and merged at which point Gerrit would

> then also label the changes associated with it as merged.

Why would you like to review the topic ? Isn't enough that all the
changes are reviewed to merge the topic ?

[Chris Harris]

On Thu, Dec 13, 2012 at 8:21 PM, Martin Fick <mf...@codeaurora.org> wrote:
> Chris,
>
> I appreciate your efforts to rethink things.
>
> Your proposal below discusses technical details in light of
> your vision of topics, but these pieces only seem to make
> sense in the context of a larger solution. But we don't
> share the big vision to this larger solution, so discussing
> these details still seems premature. We don't share the big
> vision because we don't understand what is wrong with the
> current way that things are done in Gerrit today. We need
> help, we are set in our ways. Make us feel your pain, share
> a problem with us that we might agree is painful.
>
> What we want is to see how today's Gerrit workflow is missing
> pieces. We want to see small problems and small solutions
> (not small pieces to a bigger solution). An example small
> problem might be: "It is hard to navigate amongst changes
> for a related topic", and then, a discussion about how can we
> improve navigation amongst changes could ensue?

I guess I didn't understand some in the Gerrit community didn't share
the greater view of added topic review support to Gerrit. I will do my
best to explain our motivation. However, I would also encourage others
that support the introduction of topic reviews to pitch in, I know we
are not alone and have heard from several groups that they feel this
is a deficiency of Gerrit and a reason not to adopt it. The fact that
a big project like Qt paid for the topic support to be added is
illustration enough in my opinion, it seems they are in a similar
position to us being suck on a fork:

https://codereview.qt-project.org

Git by its nature enables a branchy workflow that is used in many
project, including most of the projects at Kitware. A developer
creates a topic branch to work on a particular fix or feature. This
branch represents a logical change but may contain several commits,
here is an example of a topic on our Gerrit site:

http://review.source.kitware.com/#/t/1810/

When the developer is happy with their branch they push it for review.
As the developer has grouped the changes into a logical unit it makes
sense that the branch we reviewed as a whole. Its telling a story, I
have worked on this feature and these are the steps I took to get
there. Its also saying these changes should be merged together as
they logically build on one another. All this is lost if the topic has
to be squash into one commit for review or appears as multiple changes
on the review site.

We also run our CI on topics rather than individual changes for two
reason; one the cost of compiling and running tests on a big project
such as VTK is too great, we don't have the hardware and two its
possible that a particular change in a topic branch breaks a test as a
result of a refactor for example and is fixed in the next change,
although we do encourage developers to make sure commit standalone.
When a topic have been review and is merged into master, the history
still shows the logical grouping of changes which is useful to get the
context as to why a particular change was made.

We are not saying this is the only workflow, but it is a popular one
that Gerrit should support more fully. This would be a per project
setting so groups could decide if they wanted to use topic review for
a particular project.

If this is something people are still not interested in is there away
that this sort of functionality could be encapsulated in a plugin? I'm
not that familiar with the API so am not sure what it exposes.

> Edwin asked you earlier why using Gerrit's already existing
> branching support does not work? It seems like a reasonable
> question to me. If that approach does not work well for you,
> could you describe why not? Could you pick one small thing
> about such a workflow that doesn't work well and that could
> use improvement?
>

I am not familiar with the approach that Edwin detailed, but have
heard to from others that it don't address all the issues. I hope
someone with more knowledge of this can weigh in. It sounds like there
still isn't a strong grouping of changes in a topic. The current topic
field is really just a label in my opinion. Plus it complicate the
workflow as each change has to merged separately and then the merge
commit has to reviewed. Also how to you make updates to a change if
there are already merged into a branch, do you have to create a new
change?

[Chris Harris]

On Thu, Dec 13, 2012 at 8:39 PM, Pursehouse, David
<David.Pu...@sonymobile.com> wrote:
> Martin, is this not quite similar to the "superchange" feature that we (you, I, Gustaf, etc) discussed during the Hackathon?
>
> If that's the case, it's probably something we want to look into a bit more.
>
> One concern I have about the proposal below is that it changes the behavior of the existing topic name feature. The way topics are used now on gerrit-review, and in the workflow at my company (and most likely others), not all changes on the same topic are so tightly related that they must be reviewed and merged at the same time. A good example of this is the changes on the "docs" topic [1] - the changes currently open on that are not really related beyond the fact that they are documentation updates.

In my opinion the topic name is really more of a label or tag. Topic
review support would be enable on per project basis so I think with
adequate documentation this behavior could be changed if topic support
was enabled on a project.

[Johan Björk]

While I don't use gerrit at my dayjob anymore, I can pitch in that this is (was?) one of the most requested changes to our gerrit instance.

+1 for real topic support.

/Johan

[Martin Fick]

On Friday, December 14, 2012 07:23:01 am Chris Harris wrote:
> On Thu, Dec 13, 2012 at 8:21 PM, Martin Fick
<mf...@codeaurora.org> wrote:
> > Chris,

> I guess I didn't understand some in the Gerrit community
> didn't share the greater view of added topic review
> support to Gerrit.

It might be worth reading all the old posts on the mailing
list related to this to better understand some of our
frustrations with the approach taken (code dumps, dev on
github, external wikis...), there has been no community
involvement, not a very open approach.

> When the developer is happy with their branch they push it
> for review. As the developer has grouped the changes into
> a logical unit it makes sense that the branch we reviewed
> as a whole. Its telling a story, I have worked on this
> feature and these are the steps I took to get there. Its
> also saying these changes should be merged together as
> they logically build on one another.

Sounds like a git branch to me.


>...

>When a topic have been review and is
> merged into master, the history still shows the logical
> grouping of changes which is useful to get the context as
> to why a particular change was made.

Do you mean the git history? That would still be there if
with a merge from another branch. Gerrit history would
likely be there also since they would all have been destined
for the same branch, although if the destination branch is
reused that might get blurred.


...

> > Edwin asked you earlier why using Gerrit's already
> > existing branching support does not work? It seems
> > like a reasonable question to me. If that approach
> > does not work well for you, could you describe why not?
> > Could you pick one small thing about such a workflow
> > that doesn't work well and that could use improvement?
>
> I am not familiar with the approach that Edwin detailed,
> but have heard to from others that it don't address all
> the issues.

You are probably right, it probably doesn't. But that is
what we would like to hear, and that is what we would like to
focus on. How can those issues be described in small pieces,
and how can they be solved in small pieces?

> I hope someone with more knowledge of this
> can weigh in. It sounds like there still isn't a strong
> grouping of changes in a topic.

Agreed, which is a feature. If you want a strong grouping,
that is a branch.

> The current topic field is really just a label in my
opinion.

Agreed, and if we had real label support I would suggest we
eliminate it.

> Plus it complicate the workflow as each change has to merged
separately

I guess that is a matter of perspective, I would call that a
feature. :) However, since I am a fan of organizations being
able to define workflows how they see fit, I am fine with being
able to define this workflow, but I would like to discuss that
more...

One concern I have if I were subjected to it: I can't help of
thinking of the situation where I may have thought that I
wanted to use your version of a topic branch, uploaded 5
changes: A B C D E for review expecting them to be a unit,
only to figure out half way through the review process that
the review is bogging down on D & E. At this point I might
realize that I would like to get away with merging A B C to
have a partial feature right now, what can I do? Do I have
to push A B C as separate changes elsewhere now and loose all
my review history? Has this never happened while using your
approach?

So I wonder, is the desirable feature really to be able to
restrict partial merges, or to be able to merge them all at
once? This seems like 2 different features, and if people
want them, why force them to be tied together?

Could you suggest simple solutions to these 2 problems?
Solutions that could be used beyond topics, that could be
used for any grouping of changes?

> and then the merge commit has to reviewed.

How so, it could be pushed directly no? If that isn't easy
enough, please suggest a simpler solution...

-Martin

[Chris Harris]

On Fri, Dec 14, 2012 at 12:06 PM, Martin Fick <mf...@codeaurora.org> wrote:
> On Friday, December 14, 2012 07:23:01 am Chris Harris wrote:
>> On Thu, Dec 13, 2012 at 8:21 PM, Martin Fick
> <mf...@codeaurora.org> wrote:
>> > Chris,
>> I guess I didn't understand some in the Gerrit community
>> didn't share the greater view of added topic review
>> support to Gerrit.
>
> It might be worth reading all the old posts on the mailing
> list related to this to better understand some of our
> frustrations with the approach taken (code dumps, dev on
> github, external wikis...), there has been no community
> involvement, not a very open approach.

I am sorry to hear about the frustrations of the past I can really
change that. I am just trying to move forward with a feature that more
than a few people feel might be useful.

Is there anyone on the list that is using this approach? I would
interesting in hearing more about this.

>
>
>> I hope someone with more knowledge of this
>> can weigh in. It sounds like there still isn't a strong
>> grouping of changes in a topic.
>
> Agreed, which is a feature. If you want a strong grouping,
> that is a branch.

Yes, this is what we are looking for.

>
>
>> The current topic field is really just a label in my
> opinion.
>
> Agreed, and if we had real label support I would suggest we
> eliminate it.

+1

>
>
>
>> Plus it complicate the workflow as each change has to merged
> separately
>
> I guess that is a matter of perspective, I would call that a
> feature. :) However, since I am a fan of organizations being
> able to define workflows how they see fit, I am fine with being
> able to define this workflow, but I would like to discuss that
> more...
>
> One concern I have if I were subjected to it: I can't help of
> thinking of the situation where I may have thought that I
> wanted to use your version of a topic branch, uploaded 5
> changes: A B C D E for review expecting them to be a unit,
> only to figure out half way through the review process that
> the review is bogging down on D & E. At this point I might
> realize that I would like to get away with merging A B C to
> have a partial feature right now, what can I do? Do I have
> to push A B C as separate changes elsewhere now and loose all
> my review history? Has this never happened while using your
> approach?

The approach we would take here would to push a new version of the
topic with D & E removed, thus the topic could then be merged. The
review history of this topic would show this ( and in the current
version D & E would get abandon )\

>
> So I wonder, is the desirable feature really to be able to
> restrict partial merges, or to be able to merge them all at
> once? This seems like 2 different features, and if people
> want them, why force them to be tied together?

I think the desirable feature here is the ability to review and merge
them as one unit.

>
> Could you suggest simple solutions to these 2 problems?
> Solutions that could be used beyond topics, that could be
> used for any grouping of changes?
>
>
>> and then the merge commit has to reviewed.
>
> How so, it could be pushed directly no? If that isn't easy
> enough, please suggest a simpler solution...

I guess I don't understand the workflow. Edwin mentioned review and
merging each change and then having to review the merge commit? I
guess my point here is it hard enough to get our user to participate
in code review then all they have to do is type git gerrit-push :-)

>
> -Martin

[Marcus D. Hanwell]

On Fri, Dec 14, 2012 at 2:07 PM, Chris Harris <chris....@kitware.com> wrote:
> On Fri, Dec 14, 2012 at 12:06 PM, Martin Fick <mf...@codeaurora.org> wrote:
>> On Friday, December 14, 2012 07:23:01 am Chris Harris wrote:
>>> On Thu, Dec 13, 2012 at 8:21 PM, Martin Fick
>> <mf...@codeaurora.org> wrote:
>>> > Chris,
>>> I guess I didn't understand some in the Gerrit community
>>> didn't share the greater view of added topic review
>>> support to Gerrit.
>>
>> It might be worth reading all the old posts on the mailing
>> list related to this to better understand some of our
>> frustrations with the approach taken (code dumps, dev on
>> github, external wikis...), there has been no community
>> involvement, not a very open approach.
>
> I am sorry to hear about the frustrations of the past I can really
> change that. I am just trying to move forward with a feature that more
> than a few people feel might be useful.
>

We shared that pain, and really did not like how the feature was
developed. I came to the Git Together in 2011 so that we could discuss
how we might add real topic support to Gerrit (and if it would be a
welcome change). I left feeling that it would be, but Shawn made it
quite clear that we would likely have to own it as the original
development effort had ceased.

At that meeting I got a feeling that this was a widely requested
feature, with many people asking me about it during and after the
meeting. We showed several sites that are in production, but
unfortunately due to the large changes in Gerrit they are now
effectively forks of Gerrit. See the Qt Project and our Kitware
instance of Gerrit for topic forks that have been running in
production for well over a year now.

http://review.source.kitware.com/#/q/entity:topic%20status:open,n,z
https://codereview.qt-project.org/#q,status:open,n,z

We are both maintaining significant patch sets, and we would like to
work with the community to get this merged in. It is surprising to see
so much resistance to the basic approach. Is strong topic support the
general Gerrit community is unwilling to entertain at this point? See

http://git-scm.com/book/en/Git-Branching-Branching-Workflows

This isn't a workflow we are creating independently, but a widely used
workflow that we would like to enable in Gerrit. A large number of our
projects have requested this, and quite a few have been using it. It
shares some commonality with Github's pull request, along with those
on Gitorious and Bitbucket.

>>
>>> When the developer is happy with their branch they push it
>>> for review. As the developer has grouped the changes into
>>> a logical unit it makes sense that the branch we reviewed
>>> as a whole. Its telling a story, I have worked on this
>>> feature and these are the steps I took to get there. Its
>>> also saying these changes should be merged together as
>>> they logically build on one another.
>>
>> Sounds like a git branch to me.
>>

It is a git branch, our vision of topic review is entirely centered
around git branches and the branching workflows used by a large number
of projects employing git. The existing topic support is labels, which
we don't object to but they do not fulfill our needs (or those of
other projects).

The way it is implemented topic review is optional for projects that
want it, we maintain a mixture of projects using and not using topic
reviews on a single Gerrit instance. Chris has time to work on this,
and break it into smaller pieces (I even have a small amount of time,
but nothing significant) and we as an organization would like to
continue to use and develop this feature.

Thanks,

Marcus

[Martin Fick]

On Friday, December 14, 2012 12:22:34 pm you wrote:
...

> At that meeting I got a feeling that this was a widely
> requested feature, with many people asking me about it
> during and after the meeting. We showed several sites
> that are in production, but unfortunately due to the
> large changes in Gerrit they are now effectively forks of
> Gerrit. See the Qt Project and our Kitware instance of
> Gerrit for topic forks that have been running in
> production for well over a year now.
>
> http://review.source.kitware.com/#/q/entity:topic%20status
> :open,n,z
> https://codereview.qt-project.org/#q,status:open,n,z
>
> We are both maintaining significant patch sets, and we
> would like to work with the community to get this merged
> in. It is surprising to see so much resistance to the
> basic approach.

That seems unfair. Our resistance has been no secret from
some of us in the community, it began before this feature was
developed in isolation. Again please search this list, or
even the IRC logs... But to be clear (because I feel that
you are miss-representing things), the resistance is not to
having improvements in Gerrit, it is to the approach you
took, and is still being taken,

> Is strong topic support the general
> Gerrit community is unwilling to entertain at this point?

Again a bit unfair, "Strong" is highly subjective. We all
want improvements; you are portraying us as rejecting your
ideas because we don't want a massive code dump developed
outside of the community, which drastically changes the
Gerrit data structures, screens, creates new DB tables...
And this has happened several times. Yet, you are clearly
ignoring any of the feedback that we are giving other than to
simply point out that the feature is great and that it is
used and desired by "many". I am not disputing this. And I
welcome the knowledge you have gained, I wish you would share
it and come work with us. I wish you would explain to us how
things can be improved without saying: "here: we made it
great! Why don't you want this?"

I am struggling here, I am trying to work with you, but I
don't feel that this is being reciprocated,


-Martin

[Marcus D. Hanwell]

Didn't mean to be unfair, we did not develop the code in that fashion
- another party did. What we discussed back in 2011, and what we are
attempting to do here is rewrite portions of this functionality and
get them merged in smaller pieces. Redeveloping large parts of the
functionality, and asking for review as they are rewritten. It is not
ideal, and I would have really preferred the original functionality
wasn't written the way it was.

>
>> Is strong topic support the general
>> Gerrit community is unwilling to entertain at this point?
>
> Again a bit unfair, "Strong" is highly subjective. We all
> want improvements; you are portraying us as rejecting your
> ideas because we don't want a massive code dump developed
> outside of the community, which drastically changes the
> Gerrit data structures, screens, creates new DB tables...
> And this has happened several times. Yet, you are clearly
> ignoring any of the feedback that we are giving other than to
> simply point out that the feature is great and that it is
> used and desired by "many". I am not disputing this. And I
> welcome the knowledge you have gained, I wish you would share
> it and come work with us. I wish you would explain to us how
> things can be improved without saying: "here: we made it
> great! Why don't you want this?"

Poor choice of words, I was referring to the "strong grouping" you
mentioned and that was my intent. I agree "Strong" is far too vague,
and will try to be more careful with my wording. I think you are being
unfair referring to this massive code dump, someone else tried to do
that, Gerrit has changed so much in the meantime that much of the
functionality needs to be pulled through by hand. We are attempting to
create smaller, reviewable chunks, wait for their review, and then
create more on top of what has been accepted working towards merging
in a sizable new feature.

>
> I am struggling here, I am trying to work with you, but I
> don't feel that this is being reciprocated,
>

We are bending over backwards to work with you and the community, and
are also struggling to see the right way to get something like this
merged. I have followed the list through the initial merge requests,
none of which were made by myself or Chris. We worked with them to try
and help them format their topic branch, and can break the changes up
into smaller patches working towards the goal of adding this
functionality if needed. We encouraged them to upload the original
topic so that we were free to then rebase/rewrite them (satisfying the
CLA on Shawn's advice, but not expecting them to be merged in that
form). To be clear, we had no part in writing the original feature,
but have since fixed up bugs and improved the feature set.

I have not once said, "here: we made it great! Why don't you want
this?" I have offered example instances to show how things are
currently implemented, I did not intend to give you the impression we
thought it was done but use it as a starting point. We are trying to
understand if data structure changes, new screens and database tables
are entirely off the table. If not how best to engage in a
conversation about what changes we can make that are likely to be
merged.

I am trying to share our experience with you, and see how/if this
functionality can be merged. You may have to guide me further on what
level you wish to discuss this - high level workflow, i.e. why are
topic branches useful and what do we feel we gain, medium level of
what new interface, database tables, data structures, or how best to
proceed in rewriting the pieces of functionality and requesting review
of those patches.

Cheers,

Marcus

[Saša Živkov]

Let me try to describe how I would handle this scenario in the stock Gerrit:

1. assuming the master branch already exists in that project in Gerrit I would

create the 3 additional branches (in Gerrit): dumbidea, iss91, iss91v2

They would probably not all be created at the same time but at the point

in time when work has to start on that (topic) branch.

2. Developers working on the dumbidea topic would push to refs/for/dumbidea,

review changes and merge them in the dumbidea branch.

The same applies for the two other topic branches.

3. When the dumbidea is finished and looks great, a developer merges

it locally with the master branch and pushes the merge commit to the master

branch in Gerrit. It could be pushed for code review or bypassing code review.

4. when iss91v2 topic branch is good, a developer merges it locally with

master and pushes the merge commit to the master branch in Gerrit.

The result looks exactly as on the second commit graph in that article.

Can you identify and explain issues with the approach I described and then

explain how having "strong topic support" would help?

Saša

 

[Martin Fick]

On Friday, December 14, 2012 12:07:29 pm Chris Harris wrote:
> On Fri, Dec 14, 2012 at 12:06 PM, Martin Fick
<mf...@codeaurora.org> wrote:
> > On Friday, December 14, 2012 07:23:01 am Chris Harris
wrote:
> >> On Thu, Dec 13, 2012 at 8:21 PM, Martin Fick
> >
> > <mf...@codeaurora.org> wrote:
> >> > Chris,
> >>

> >> > Edwin asked you earlier why using Gerrit's already
> >> > existing branching support does not work? It seems
> >> > like a reasonable question to me. If that approach
> >> > does not work well for you, could you describe why
> >> > not?
> >> >
> >> > Could you pick one small thing about such a workflow
> >> >
> >> > that doesn't work well and that could use
> >> > improvement?
> >>
> >> I am not familiar with the approach that Edwin
> >> detailed, but have heard to from others that it don't
> >> address all the issues.
> >
> > You are probably right, it probably doesn't. But that
> > is what we would like to hear, and that is what we
> > would like to focus on. How can those issues be
> > described in small pieces, and how can they be solved
> > in small pieces?
>
> Is there anyone on the list that is using this approach? I
> would interesting in hearing more about this.

It is the normal git workflow.

> >> I hope someone with more knowledge of this
> >> can weigh in. It sounds like there still isn't a strong
> >> grouping of changes in a topic.
> >
> > Agreed, which is a feature. If you want a strong
> > grouping, that is a branch.
>
> Yes, this is what we are looking for.

Cool. So what is the next missing feature for branches that
you want?

Hmm, then what happens to D E? Do I have to push them as
separate changes and loose their history?

Also, how can someone indicate to you that A B C were ready
to be merged, go ahead and do this without D E?

> > So I wonder, is the desirable feature really to be able
> > to restrict partial merges, or to be able to merge them
> > all at once? This seems like 2 different features, and
> > if people want them, why force them to be tied
> > together?
>
> I think the desirable feature here is the ability to
> review and merge them as one unit.

Please clarify what you mean by review? Do you mean approve?
We have worked hard to make approvals flexible in Gerrit, but
I am sure it can be further improved. It sounds like you
want the ability to define approval rules on a per branch
level. You can likely do that with prolog rules, if you
can't, do you have any suggestions for improving this?

Here again, you see the inability to give approvals to
changes as a feature, I see it as a limit. If I don't like
change E, why can't I indicate this on change E with a -1.
Is this something your workflow is really trying to prevent?
Or is a side effect of the fact that you want to be able to
approve things in one place?

What if I want to be able to approve things in one place, but
I want to be able to reject things on the individual changes?
That seems like a more flexible approach. I suspect you might
be able to get there with some clever prolog which takes
rejections into account but only requires +2 on the final
merge commit?

> > How so, it could be pushed directly no? If that isn't
> > easy enough, please suggest a simpler solution...
>
> I guess I don't understand the workflow. Edwin mentioned
> review and merging each change and then having to review
> the merge commit? I guess my point here is it hard enough
> to get our user to participate in code review then all
> they have to do is type git gerrit-push :-)

Is your point that creating a merge commit is perhaps too
complicated? I probably agree in some cases. Do you have
any suggestions of how that can be improved? Perhaps all you
need is for a fancy way for Gerrit to create that merge
commit change automatically? This merge commit seems like it
is the more natural approval placeholder for the entire
branch then creating a whole new topic object?

-Martin

[Jonathan Nieder]

Saša Živkov wrote:

> Let me try to describe how I would handle this scenario in the stock Gerrit:
>
> 1. assuming the master branch already exists in that project in Gerrit I would
> create the 3 additional branches (in Gerrit): dumbidea, iss91, iss91v2
> They would probably not all be created at the same time but at the point
> in time when work has to start on that (topic) branch.
>
> 2. Developers working on the dumbidea topic would push to refs/for/dumbidea,
> review changes and merge them in the dumbidea branch.
> The same applies for the two other topic branches.
>
> 3. When the dumbidea is finished and looks great, a developer merges
> it locally with the master branch and pushes the merge commit to the master
> branch in Gerrit. It could be pushed for code review or bypassing code
> review.

I suspect a goal in Chris's design is to allow developers to work on
dumbidea coordinating some way outside of Gerrit and then to review
the merge as a single unit when the work is finished. Is that doable?

Hope that helps,
Jonathan

[Saša Živkov]

My understanding was that "topic support" meant coordinating in Gerrit

not outside of Gerrit. 

the merge as a single unit when the work is finished.  Is that doable?

Once the work in the dumbidea branch is done there are at least the following

possibilities to merge that work in the master branch:

1. merge the tips of dumbidea and master and push that merge to master (in Gerrit)

bypassing code review

2. the same as 1 but push the merge commit for code review. Approving the merge

commit means approving the complete work in the dumbidea.

I think option 2 is what you asked for?

 

Hope that helps,
Jonathan

[Martin Fick]

On Friday, December 14, 2012 03:57:30 pm Jonathan Nieder
wrote:

> Saša Živkov wrote:
> I suspect a goal in Chris's design is to allow developers
> to work on dumbidea coordinating some way outside of
> Gerrit and then to review the merge as a single unit when
> the work is finished. Is that doable?

By "review", do you mean view as a single diff? We have
talked before about making this possible for merge commits.
I believe there is a change up for review by Brad Larson to
do that,

-Martin

[Jonathan Nieder]

Martin Fick wrote:
> On Friday, December 14, 2012 03:57:30 pm Jonathan Nieder
> wrote:
>> Saša Živkov wrote:

>> I suspect a goal in Chris's design is to allow developers
>> to work on dumbidea coordinating some way outside of
>> Gerrit and then to review the merge as a single unit when
>> the work is finished. Is that doable?
>
> By "review", do you mean view as a single diff? We have
> talked before about making this possible for merge commits.

Yes, exactly. View as a first-parent diff, comment line-by-line, +1
if it looks good, etc.

Regards,
Jonathan

[Roland Schulz]

Hi,

two minor features which topics have and branches don't, and haven't seen mentioned yet:

A topic provides a list of all required changes. Currently the Dependencies view shows only the immediate parent and child. Adding the option to expand the dependency view to show all changes between the merge base (the closest parent which have been merged) and the tip would give this nice feature.

One minor issue with branches compared to topics is that they don't automatically get closed when merged. It might be useful to define a type of short lived topic branch, which can be created on the fly by a user by pushing to refs/for/somebranch/topic (this should already work with the current permission setup) and which (its ref) gets automatically deleted when merged. Otherwise the list of branches would become huge if a branch is used for any small topic.

We have
talked before about making this possible for merge commits.
I believe there is a change up for review by Brad Larson to
do that,

It would be nice, if this could be extended to all commits. For non merge commits it would be nice to be able to see the diff relative to the merge base (as defined above for the dependencies). 

An example of how to write a prologue rule which depends on the subsequent merge (e.g. from the topic branch into the main branch) would be nice.

Roland

--
ORNL/UT Center for Molecular Biophysics cmb.ornl.gov
865-241-1537, ORNL PO BOX 2008 MS6309

[Martin Fick]

Chris Harris <chris....@kitware.com> wrote:
>> Edwin asked you earlier why using Gerrit's already existing
>> branching support does not work? It seems like a reasonable
>> question to me. If that approach does not work well for you,
>> could you describe why not? Could you pick one small thing
>> about such a workflow that doesn't work well and that could
>> use improvement?
>>
>
>I am not familiar with the approach that Edwin detailed, but have
>heard to from others that it don't address all the issues. I hope
>someone with more knowledge of this can weigh in. It sounds like there
>still isn't a strong grouping of changes in a topic. The current topic
>field is really just a label in my opinion. Plus it complicate the
>workflow as each change has to merged separately and then the merge
>commit has to reviewed. Also how to you make updates to a change if
>there are already merged into a branch, do you have to create a new
>change?

I would like to suggest another line of thinking which might also be of use. I cannot help but wonder if adding Gerrit dependencies would help achieve some of the things which you desire. If I push changes A B C D E for review and want them to submitted as a unit, am I not describing a dependency?

Let's suppose for one moment that Gerrrit supported dependencies which transcended git dependencies, a highly requested Gerrrit feature, in various forms, which you will not likely find much opposition to. Now let us suppose that there were an easy way to define the dependency above: that A B C D E be submitted together. Naturally along with being able to define such dependencies, Gerrit would likely need to be able to intelligently manage their submission also. If you don't want the extra overhead of merging changes to a separate branch first, simply upload them together, tie them together with a dependency, and submit them all together.

Now, if you want to review them together, as Roland suggested, how about making the change screen display these dependencies in a nice table, would such a screen not begin to resemble your topic screen a bit?

-Martin


Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum

[Saša Živkov]

On Sat, Dec 15, 2012 at 1:14 AM, Roland Schulz <rol...@utk.edu> wrote:

Hi,

two minor features which topics have and branches don't, and haven't seen mentioned yet:

A topic provides a list of all required changes. Currently the Dependencies view shows only the immediate parent and child. Adding the option to expand the dependency view to show all changes between the merge base (the closest parent which have been merged) and the tip would give this nice feature.

It would be easy to show all commits on a (topic) branch that are not contained

in another (master) branch.

One minor issue with branches compared to topics is that they don't automatically get closed when merged. It might be useful to define a type of short lived topic branch, which can be created on the fly by a user by pushing to refs/for/somebranch/topic (this should already work with the current permission setup) and which (its ref) gets automatically deleted when merged. Otherwise the list of branches would become huge if a branch is used for any small topic.

Agreed. It looks reasonable to define such a branch type.

Looks like you possibly identified two feature requests that could make

usage of branches as topics more comfortable.

Another feature I could imagine would be a possibility to merge a topic branch

into another (normally master) branch from Gerrit UI. This would save the

effort of creating a merge commit locally when the topic and master merge

cleanly.

 

We have
talked before about making this possible for merge commits.
I believe there is a change up for review by Brad Larson to
do that,

It would be nice, if this could be extended to all commits. For non merge commits it would be nice to be able to see the diff relative to the merge base (as defined above for the dependencies). 

An example of how to write a prologue rule which depends on the subsequent merge (e.g. from the topic branch into the main branch) would be nice.

Roland

--
ORNL/UT Center for Molecular Biophysics cmb.ornl.gov
865-241-1537, ORNL PO BOX 2008 MS6309

--

[Swindells, Thomas]

For my teams the workflow we take is:

1.       Develop new features on a feature branch.

2.       Each commit on the feature branch is reviewed by the feature team.

3.       Prepare a squashed merge of the feature branch into master and push for review

4.       Prepare a real merge of the tip of the feature branch into master

5.       Perform code review by the wider team, when it fails code review go back to 2 for the issues to be fixed.

6.       Submit the real merge of tip and abandoned the squash merge.

 

The process isn’t ideal, particularly as master can often be changing as other features are being merged, poms updated with new release versions etc.

 

The number one biggest issue we have is that gerrit doesn’t support reviewing merge commits. The latest versions make it slightly better but still aren’t ideal. This is why we have to make the merge commit. Merging in Brad’s change (https://gerrit-review.googlesource.com/#/c/33960/4) would help resolve this part of the issue.

 

The next issue is that creating a topic merge is additional hassle (and potentially error prone). Having the ability in the UI to create/update the merge would simplify this. This probably needs to cater for two different workflows:

-          a short lived feature branch which will only be merged into master once. In this case if a new patch is pushed to the branch and then the prepare merge commit button is pressed for a second time it should reuse the existing merge commit, adding it as a new patchset.

-          A long lived branch which may be merged into master multiple times. In this case the user may need the option of creating a new change rather than reusing an existing one.

 

Sometime review comments (or build failures) are over trivial issues such as a missing semi-colon or comment. Having gerrit allow simple online file editing would allow these types of things to be resolved quicker. If the change-set is still outstanding then it should be added as a new patchset, if the change has already been merged then a new commit should be created for it.

 

Merges, like rebases, have a high chance of having conflicts. At the moment the rebase button flatly refused to rebase if it detects any conflicts. Once the above support is provided, it would be possible for a new patchset to be created regardless, with conflicts highlighted and the option to compare against both base branches. The patch-set can be prevented from being merged (or even build by Jenkins) until someone has gone through and edited the change to resolve/verify the conflicts as necessary.

 

To me these are the main changes that would make working with topics (and just generally) much easier and reduce a load of pain, though implementing them probably isn’t anywhere near as simple as I’ve made out above! Getting Brad’s change merged would at least be a giant leap in the right direction.

 

Thanks for the hard work,

 

Thomas

 

From: repo-d...@googlegroups.com [mailto:repo-d...@googlegroups.com] On Behalf Of Saša Živkov
Sent: 16 December 2012 21:06
To: Roland Schulz
Cc: Repo and Gerrit Discussion
Subject: Re: Gerrit Topic Review - Take two?

 

 

On Sat, Dec 15, 2012 at 1:14 AM, Roland Schulz <rol...@utk.edu> wrote:

---

**************************************************************************************
This message is confidential and intended only for the addressee. If you have received this message in error, please immediately notify the postm...@nds.com and delete it from your system as well as any copies. The content of e-mails as well as traffic data may be monitored by NDS for employment and security purposes. To protect the environment please do not print this e-mail unless necessary.

NDS Limited. Registered Office: One London Road, Staines, Middlesex, TW18 4EX, United Kingdom. A company registered in England and Wales. Registered no. 3080780. VAT no. GB 603 8808 40-00
**************************************************************************************

[Chris Harris]

Let me try to describe how I would handle this scenario in the stock Gerrit:

1. assuming the master branch already exists in that project in Gerrit I would

create the 3 additional branches (in Gerrit): dumbidea, iss91, iss91v2

They would probably not all be created at the same time but at the point

in time when work has to start on that (topic) branch.

2. Developers working on the dumbidea topic would push to refs/for/dumbidea,

review changes and merge them in the dumbidea branch.

The same applies for the two other topic branches.

3. When the dumbidea is finished and looks great, a developer merges

it locally with the master branch and pushes the merge commit to the master

branch in Gerrit. It could be pushed for code review or bypassing code review.

4. when iss91v2 topic branch is good, a developer merges it locally with

master and pushes the merge commit to the master branch in Gerrit.

The result looks exactly as on the second commit graph in that article.

Can you identify and explain issues with the approach I described and then

explain how having "strong topic support" would help?

Saša

Sasa, 

Thanks for outlining your current workflow. There are a couple of issue with this approach from our point of view.   

- This workflow seems a little more complicated that what we currently have, having to create branches in Gerrit before pushing etc. Educating our user who have a wide range of git/gerrit skills would not be easy. We are really trying to keep the process to a minimum, its seems to me that a multi step process like this would really hinder progress. What we currently have is; user creates a branch, push the branch to Gerrit, the branch gets review, the branch gets merged ... 

- We would not want our user to be able directly push to bypass code review in step 3 so another review would be required, yet another step.

- There is no point to attach our CI to unless we ran it on every change that simply isn't possible.

- How are these Gerrit branches cleaned up? Is this another step the user has to perform when the branch has been merged into master?

Have said this I can see two possible features that would improve this workflow:

- Allow the branch in Gerrit to automatically be created when the user does their first push on a topic.

- Allow the branch to be merged into master from the UI, so the user doesn't have to push the merge for review ... 

 

[Chris Harris]

D E will still be in Gerrit, their history will not be lost. They could pushed as separate changes or added to a new topic branch.

 

Also, how can someone indicate to you that A B C were ready
to be merged, go ahead and do this without D E?

A B C are changes they can mark them as review and verified, D and E and can be -2. Also comments can be added to a topic so this could be used to communicate this as well.

 

> > So I wonder, is the desirable feature really to be able
> > to restrict partial merges, or to be able to merge them
> > all at once?  This seems like 2 different features, and
> > if people want them, why force them to be tied
> > together?
>
> I think the desirable feature here is the ability to
> review and merge them as one unit.

Please clarify what you mean by review?

When we say review of topic we mean the same process that a change goes through but applied to a group of changes. So +2, -2, comments etc.

 

 Do you mean approve?

The same as for a change but applied to a group of changes.

 

We have worked hard to make approvals flexible in Gerrit, but
I am sure it can be further improved.  It sounds like you
want the ability to define approval rules on a per branch
level.  

Correct, the topic branch. But if you are defining approval rules on a topic branch, it needs some representation in UI so you can attach approvals, comments etc to it?

 

Here again, you see the inability to give approvals to
changes as a feature, I see it as a limit.  If I don't like
change E, why can't I indicate this on change E with a -1.

You can E is normal change, it just part of a topic. You can -1 it etc. However, you can't submit it on its own as it part of a topic. The final approval and submission is at the topic level.

 

What if I want to be able to approve things in one place, but
I want to be able to reject things on the individual changes?

 

You can, as I said above. The final approval is at a topic level but you can reject a change with a -1. As the moment this isn't enforced (i.e. is up to a user to to +2 a topic that has changes with a -1 etc. ) but could be with some prolog rules.

 

> > How so, it could be pushed directly no?  If that isn't
> > easy enough, please suggest a simpler solution...
>
> I guess I don't understand the workflow. Edwin mentioned
> review and merging each change and then having to review
> the merge commit? I guess my point here is it hard enough
> to get our user to participate in code review then all
> they have to do is type git gerrit-push :-)

Is your point that creating a merge commit is perhaps too
complicated?

Yes, I think having to create the merge commit is too complicated. 

 

 I probably agree in some cases.  Do you have
any suggestions of how that can be improved?  Perhaps all you
need is for a fancy way for Gerrit to create that merge
commit change automatically?

The ability to merge a branch into master in the UI might improve this and I mention in my reply to Sasa. May be this button would only appear if all changes targeted at the branch has been merged ...

 

[Chris Harris]

I suspect a goal in Chris's design is to allow developers to work on
dumbidea coordinating some way outside of Gerrit and then to review
the merge as a single unit when the work is finished. 

All the coordinating is done through Gerrit. If user want to work on a topic branch together, they can do this through Gerrit.

[Chris Harris]

I would like to suggest another line of thinking which might also be of use.  I cannot help but wonder if adding Gerrit dependencies would help achieve some of the things which you desire.  If I push changes A B C D E for review and want them to submitted as a unit, am I not describing a dependency?

Let's suppose for one moment that Gerrrit supported dependencies which transcended git dependencies, a highly requested Gerrrit feature, in various forms, which you will not likely find much opposition to.  Now let us suppose that there were an easy way to define the dependency above: that A B C D E be submitted together.   Naturally along with being able to define such dependencies, Gerrit would likely need to be able to intelligently manage their submission also.  If you don't want the extra overhead of merging changes to a separate branch first, simply upload them together, tie them together with a dependency, and submit them all together.

Now, if you want to review them together, as Roland suggested, how about making the change screen display these dependencies in a nice table, would such a screen not begin to resemble your topic screen a bit?

It would start to look like the topic screen. However, where would you been able to attach comments associated with group of changes? I also don't thing this would solve our CI issue? Or may be the create of a dependency could be used to trigger it? 

[Chris Harris]

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

- Ability to review individual commits, and the topic in its entirety.

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips.

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits.

- All changes in a topic branch should be able to be merged in a single step.

If we change achieve this with only minor changes to Gerrit then great :-)

Thanks,

Chris

[Nasser Grainawi]

On Dec 17, 2012, at 9:55 AM, Chris Harris wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Aren't you just defining your CI system to be greedy here? What does that actually have to do with topics? I might have missed it earlier, but I'm under the assumption that topics stay within a single projects. If so, you could just use the native git dependencies and teach your CI system to only care about the furthest children.

I can't go into details, but we have a CI system that handles dependencies, does verification in batches of changes, and has a (per-project) atomic submit on success. We don't have topic or any other custom dependency stuff built into Gerrit to enable that.

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

- Ability to review individual commits, and the topic in its entirety.

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips.

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits.

- All changes in a topic branch should be able to be merged in a single step.

If we change achieve this with only minor changes to Gerrit then great :-)

Thanks,

Chris

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,

hosted by The Linux Foundation

[Martin Fick]

[Martin Fick]

On Monday, December 17, 2012 08:29:22 am Chris Harris wrote:

Well, I said resemble. I was hoping that you could fill in
the blanks a bit here. I am proposing approaches using
generic ideas that would solve many problems in Gerrit. If
in the end it does not fit 100%, I hope that you can propose a
small improvement that will fill the last 10~20%?

> I also don't thing this would solve our CI issue?

Could you explain? If changes are dependent, simply download
the last change in the series and test that? What
specifically would you be missing for CI?

> Or may be the create of a dependency
> could be used to trigger it?

Why would a normal upload not be good enough to trigger it?

-Martin

[Chris Harris]

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Aren't you just defining your CI system to be greedy here? What does that actually have to do with topics? I might have missed it earlier, but I'm under the assumption that topics stay within a single projects. If so, you could just use the native git dependencies and teach your CI system to only care about the furthest children.

So you are using the refUpdated hook here and then git to workout the furthest children?

[Chris Harris]

> I also don't thing this would solve our CI issue?

Could you explain?  If changes are dependent, simply download
the last change in the series and test that?  What
specifically would you be missing for CI?

How do you know which is the latest change is series? Are you suggesting using the ref-updated hook? Otherwise, if you are trying to trigger of the patchset-created hook you don't know if you are the first or last change is a topic? 

 

 

> Or may be the create of a dependency
> could be used to trigger it?

Why would a normal upload not be good enough to trigger it?

A normal upload as in ref-updated?

 

-Martin

[Nasser Grainawi]

On Dec 17, 2012, at 10:39 AM, Chris Harris wrote:

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Aren't you just defining your CI system to be greedy here? What does that actually have to do with topics? I might have missed it earlier, but I'm under the assumption that topics stay within a single projects. If so, you could just use the native git dependencies and teach your CI system to only care about the furthest children.

So you are using the refUpdated hook here and then git to workout the furthest children?

We're not using any hooks for our CI system. We're doing a poll instead of a trigger. I think you could get to the same place triggering off a hook instead of a poll though.

Gerrit should be flexible enough to support many different CI systems and modes of operation. There are certainly parts of Gerrit that could be improved to make it easier to use with CI systems, but I don't think there's anything fundamentally lacking to support your use cases for CI.

 

I can't go into details, but we have a CI system that handles dependencies, does verification in batches of changes, and has a (per-project) atomic submit on success. We don't have topic or any other custom dependency stuff built into Gerrit to enable that.

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Brad King]

On 12/13/2012 08:21 PM, Martin Fick wrote:
> Your proposal below discusses technical details in light of
> your vision of topics, but these pieces only seem to make
> sense in the context of a larger solution. But we don't
> share the big vision to this larger solution

The motivation for topic review support is to provide a first-class
presentation of the review process for a whole patch series. When a
developer proposes a patch series for a project the resulting
discussion has two components:

1. Status of each individual change. Is the commit message okay? Is
the implementation correct? Etc.

2. Status of the series as a whole. Is it complete or should more
changes be made? Is it focused or should it be split up into multiple
series for separate review? Etc.

For example, a developer may use "git format-patch --cover-letter" and
post the series to a mailing list. Reviewers may respond to an
individual patch to comment on the implementation (#1), or to the
cover letter to comment on the whole series (#2). Then either the
maintainer accepts the series or the developer posts a revised series
and the process repeats.

Gerrit currently facilitates discussion #1 on Change review pages, but
has no first-class interface for discussion #2. Topic support should
be dedicated to the latter. Pushing, reviewer selection, review
comments, notifications, approval status, and merging should all be
available at this level (but not all exclusive to it).

The goal of a topic-level review is to decide whether a proposed patch
series is ready to be merged. Reviewers making the decision should
know exactly what patch sets are proposed for inclusion in the merge.
A topic review interface might present this as a proposed "topic
revision" that links to review interfaces for all its patch sets.

A topic revision can be either approved or rejected as a whole. If
approved, the topic revision can be merged. If rejected, the
developer can push a new revision of the topic making the requested
corrections (adding/removing commits, rebasing, splitting, etc.) and
the process repeats. (If the reviewers requested that the topic be
split the developer may push a new revision and push a new topic.)

This approach allows review of both individual changes and entire
topics at their natural granularity. It leaves no ambiguity in the
completeness of a patch series proposal or approval. It allows the
generated merge commit messages to include meta-data about the whole
series such as the topic name.

-Brad

[Saša Živkov]

On Mon, Dec 17, 2012 at 3:38 PM, Chris Harris <chris....@kitware.com> wrote:

Let me try to describe how I would handle this scenario in the stock Gerrit:

1. assuming the master branch already exists in that project in Gerrit I would

create the 3 additional branches (in Gerrit): dumbidea, iss91, iss91v2

They would probably not all be created at the same time but at the point

in time when work has to start on that (topic) branch.

2. Developers working on the dumbidea topic would push to refs/for/dumbidea,

review changes and merge them in the dumbidea branch.

The same applies for the two other topic branches.

3. When the dumbidea is finished and looks great, a developer merges

it locally with the master branch and pushes the merge commit to the master

branch in Gerrit. It could be pushed for code review or bypassing code review.

4. when iss91v2 topic branch is good, a developer merges it locally with

master and pushes the merge commit to the master branch in Gerrit.

The result looks exactly as on the second commit graph in that article.

Can you identify and explain issues with the approach I described and then

explain how having "strong topic support" would help?

Saša

Sasa, 

Thanks for outlining your current workflow. There are a couple of issue with this approach from our point of view.   

- This workflow seems a little more complicated that what we currently have, having to create branches in Gerrit before pushing etc. Educating our user who have a wide range of git/gerrit skills would not be easy.

Hmm... sounds strange: if your developers already have a wide range of git/gerrit skills why then

it would be hard to educate them? :-)

 

We are really trying to keep the process to a minimum, its seems to me that a multi step process like this would really hinder progress. What we currently have is; user creates a branch, push the branch to Gerrit, the branch gets review, the branch gets merged ... 

- We would not want our user to be able directly push to bypass code review in step 3 so another review would be required, yet another step.

- There is no point to attach our CI to unless we ran it on every change that simply isn't possible.

- How are these Gerrit branches cleaned up? Is this another step the user has to perform when the branch has been merged into master?

Currently one has to delete them from the UI. I guess pushing an empty ref would also delete a branch in Gerrit:

$ git push origin :topic-brach

 

Have said this I can see two possible features that would improve this workflow:

- Allow the branch in Gerrit to automatically be created when the user does their first push on a topic.

- Allow the branch to be merged into master from the UI, so the user doesn't have to push the merge for review ... 

This might be convenient as long as the topic merges cleanly.

However, when a topic branch cannot be merged cleanly one must resolve conflicts locally

and push yet another commit. This cannot be avoided. 

[Roland Schulz]

On Mon, Dec 17, 2012 at 10:29 AM, Chris Harris <chris....@kitware.com> wrote:

I would like to suggest another line of thinking which might also be of use.  I cannot help but wonder if adding Gerrit dependencies would help achieve some of the things which you desire.  If I push changes A B C D E for review and want them to submitted as a unit, am I not describing a dependency?

Let's suppose for one moment that Gerrrit supported dependencies which transcended git dependencies, a highly requested Gerrrit feature, in various forms, which you will not likely find much opposition to.  Now let us suppose that there were an easy way to define the dependency above: that A B C D E be submitted together.   Naturally along with being able to define such dependencies, Gerrit would likely need to be able to intelligently manage their submission also.  If you don't want the extra overhead of merging changes to a separate branch first, simply upload them together, tie them together with a dependency, and submit them all together.

Now, if you want to review them together, as Roland suggested, how about making the change screen display these dependencies in a nice table, would such a screen not begin to resemble your topic screen a bit?

It would start to look like the topic screen. However, where would you been able to attach comments associated with group of changes?

As comments to the merge commit. To reduce one step the merge commit could be automatically generated by gerrit. This probably should be controlled by a setting to allow to configure whether one wants to have this automatic generated merge commit for a) all commits b) all (current tag only) topics c) only uploads consisting of two or more changes d) none (current behavior). 

How do you handle tracking of identify of topics? I see you don't have a topic ID (as equivalent to a change-id). So how does your version of gerrit knows that it is suppose to update an existing topic instead of creating a new one? Is this based solely on the name? Is this why the topic name has to be unique at one time? If so the same logic could be used for updating of auto-created merge commits but only if used with (current type) topics not with any kind of change. 

 

I also don't thing this would solve our CI issue? Or may be the create of a dependency could be used to trigger it? 

Another option would be to only trigger for the auto-created merge commits if you would follow a policy that all changes are uploaded with (auto-created) merge commit .

Roland

 

 

 -Martin


Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Chris Harris]

Thanks for outlining your current workflow. There are a couple of issue with this approach from our point of view.   

- This workflow seems a little more complicated that what we currently have, having to create branches in Gerrit before pushing etc. Educating our user who have a wide range of git/gerrit skills would not be easy.

Hmm... sounds strange: if your developers already have a wide range of git/gerrit skills why then

it would be hard to educate them? :-)

When I say wide range, I mean from total beginners to masters, we need to cater for lowest skill level .... Sorry for the confusion :-)

 

[Roland Schulz]

On Mon, Dec 17, 2012 at 11:55 AM, Chris Harris <chris....@kitware.com> wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

Disclaimer: I'm only a user of Gerrit and have looked at the code only very limited. But from my very limited understanding:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

Should be possible if there is a setting to auto create merges.

 

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

Should be possible by extending the existing search query to show only the auto-created merges.

 

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

That could be done by showing all changes the merge is depending on.

 

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Instead the same should be achievable by a filter.

 

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

See above.

 

- Ability to review individual commits, and the topic in its entirety.

Would automatically be possible (entirety would go into merge commit).

 

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips.

Should be possible to modify the prolog submit rules accordingly.

 

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits. 

- All changes in a topic branch should be able to be merged in a single step.\

Should both be no problem with the above.

If we change achieve this with only minor changes to Gerrit then great :-)

 

It seems to me that the combination of:

- Review of Merge commits (Brad Larson's change)

- Auto created merge commits at time of upload (and updated for modified changes)

- Verification which is only triggered for the auto-created merge commits

- Submit rules which require verification only for the tip/merge commit (would automatically prohibit merging of part of a "topic")

- Extended dependency view which shows all changes between the common ancestor and the merge

- Filter rules to view only topic merges

could enable all objectives without creating a "topic" as a new first class type.

Roland 

[Saša Živkov]

On Mon, Dec 17, 2012 at 5:55 PM, Chris Harris <chris....@kitware.com> wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

We often push a, so called, patch series which is a chain of two or

more changes each depending on the previous one. And this is done

with a single push to Gerrit.

The only thing that is missing is creation of the target branch if it does not exist.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

if topics would be implemented as branches then it would mean showing

those branches which represent the topics.

 

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

This comes naturally if topics are implemented as a Git branches. Or?

 

- Ability to review individual commits, and the topic in its entirety. 

 

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips. 

I understand that you want to merge only the whole topic branch.

But this shouldn't in any way limit how do you prepare that topic branch.

And this is the key reason why I believe that topics should be implemented

as branches. I want to use the same workflow for working on a topic branch

as for working on any other branch. If topics are done this way then all the

improvements we add to Gerrit for the normal workflow can also be used

for working with topic branches. For example, since some time Gerrit supports

drafts changes. So, I want also to be able to push a draft change to a topic

branch, look at it from the UI and then publish it. If topics use their own workflow,

which is different from the default workflow then. this wouldn't be easily possible.

Another point that I have in mind is that, if we use branches for topics, then

it is very easy to have not only topics but also sub-topics and sub-sub-topics, etc...

 

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits.

This would be convenient but creation of own merge commits cannot be avoided

when conflicts have to be resolved.

 

- All changes in a topic branch should be able to be merged in a single step.

If you merge the tip of the topic branch then, semantically, all history of that branch

is merged... this is how Git works.

 

If we change achieve this with only minor changes to Gerrit then great :-)

Thanks,

Chris

[Chris Harris]

On Mon, Dec 17, 2012 at 5:11 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Mon, Dec 17, 2012 at 5:55 PM, Chris Harris <chris....@kitware.com> wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

We often push a, so called, patch series which is a chain of two or

more changes each depending on the previous one. And this is done

with a single push to Gerrit.

The only thing that is missing is creation of the target branch if it does not exist.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

if topics would be implemented as branches then it would mean showing

those branches which represent the topics.

The problem with this is if a user pushes a topic branch for review, the branch created in Gerrit wouldn't would represent the topic until all the changes had been merged into it, so there is still not way to to show a summary of the topics? Although I guess a UI could be created that attach changes to the target branch to represent the topic.

 

 

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

This comes naturally if topics are implemented as a Git branches. Or?

This suffers from the same problem as above? The branch doesn't represent the topic until all the changes have been merged?

 

- Ability to review individual commits, and the topic in its entirety. 

 

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips. 

I understand that you want to merge only the whole topic branch.

But this shouldn't in any way limit how do you prepare that topic branch.

And this is the key reason why I believe that topics should be implemented

as branches. I want to use the same workflow for working on a topic branch

as for working on any other branch. If topics are done this way then all the

improvements we add to Gerrit for the normal workflow can also be used

for working with topic branches. For example, since some time Gerrit supports

drafts changes. So, I want also to be able to push a draft change to a topic

branch, look at it from the UI and then publish it. If topics use their own workflow,

which is different from the default workflow then. this wouldn't be easily possible.

Another point that I have in mind is that, if we use branches for topics, then

it is very easy to have not only topics but also sub-topics and sub-sub-topics, etc...

 

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits.

This would be convenient but creation of own merge commits cannot be avoided

when conflicts have to be resolved.

Sure, we suffer from this with our current implementation, we yet users to rebase so the merge commit is still created by Gerrit.

[Saša Živkov]

On Tue, Dec 18, 2012 at 2:23 PM, Chris Harris <chris....@kitware.com> wrote:

On Mon, Dec 17, 2012 at 5:11 PM, Saša Živkov <ziv...@gmail.com> wrote:

On Mon, Dec 17, 2012 at 5:55 PM, Chris Harris <chris....@kitware.com> wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support:

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

We often push a, so called, patch series which is a chain of two or

more changes each depending on the previous one. And this is done

with a single push to Gerrit.

The only thing that is missing is creation of the target branch if it does not exist.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

if topics would be implemented as branches then it would mean showing

those branches which represent the topics.

The problem with this is if a user pushes a topic branch for review, the branch created in Gerrit wouldn't would represent the topic until all the changes had been merged into it,

The (topic) branch in Gerrit would represent a topic which is a work in progress.

Why should we assume that a topic must be complete from its creation?

Working on a topic branch shouldn't really be different than working on any other branch.

All future improvements in Gerrit should also be usable with topic branches.

 

so there is still not way to to show a summary of the topics?

There should be a way to show a summary of a topic. Listing all commits

on a topic branch is as simple as:

$ git log topic-branch..target-branch

and presenting this result somehow in the UI.

It is not about rebase vs merge but about the fact that someone must look at conflicts and resolve them.

Btw, since topic branch is shared, rebasing that branch wouldn't be a good practice.

[Monty Taylor]

On Monday, December 17, 2012 5:01:16 PM UTC, Nasser Grainawi wrote:

On Dec 17, 2012, at 9:55 AM, Chris Harris wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support

Thanks. And also thanks for your work on this. On behalf of the OpenStack project, let me cast my vote for getting this upstreamed. It's the single most complained about and missing feature. (ok, that's not true - single page diff might be tied)

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

Yes.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

Yes

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

Yes

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Aren't you just defining your CI system to be greedy here? What does that actually have to do with topics? I might have missed it earlier, but I'm under the assumption that topics stay within a single projects. If so, you could just use the native git dependencies and teach your CI system to only care about the furthest children.

I can't go into details, but we have a CI system that handles dependencies, does verification in batches of changes, and has a (per-project) atomic submit on success. We don't have topic or any other custom dependency stuff built into Gerrit to enable that. 

We have that too - and I'm happy to go in to details on it. We use a tool called zuul  http://ci.openstack.org/zuul/ which does the above. It also tests changes between related sets of projects. It's great. I'm more than happy to expand on it as much as possible.

It does not help us with our need for topic branches. Testing related changes is just engineering. Collaborating amongst different people to produce a coherent final product in a manner that's still reviewable by a different set of people is where topics are sorely needed.

However, if there are topics, then I want topic updated events so that I can have zuul take actions based on those events.

- Sequential listing of the commits in a topic, i.e. retain ordering

and show summary of commit scores.

Yes

- Ability to review individual commits, and the topic in its entirety.

This is essential

- Due to testing/validating tips of topics, it should only be possible

to approve/merge the tested topic tips.

- The merge commit of a topic branch needs to be created by Gerrit. We

don't want users to have to create their own merge commits.

Yes * 1000. Let me rephrase- I am not going to turn on the ability to submit merge commits, so it's not just that I don't want users to have to do this, my users cannot do this.

- All changes in a topic branch should be able to be merged in a single step.

Yes. Although, quite honestly, if the UI for the other bits is there, zuul can handle merging them in a single step for me, so meh.

If we change achieve this with only minor changes to Gerrit then great :-)

I'd like to add that it is essential that multiple people be able to push commits to a topic. 

[Chris Harris]

On Mon, Jan 7, 2013 at 2:26 AM, Monty Taylor <mor...@inaugust.com> wrote:

On Monday, December 17, 2012 5:01:16 PM UTC, Nasser Grainawi wrote:

On Dec 17, 2012, at 9:55 AM, Chris Harris wrote:

I wanted to try and take a step back by list what our basic requirements are, this is our motivation for adding topic review support

Thanks. And also thanks for your work on this. On behalf of the OpenStack project, let me cast my vote for getting this upstreamed. It's the single most complained about and missing feature. (ok, that's not true - single page diff might be tied)

Thanks for the feedback/support. We agree that this is an important feature to get into Gerrit, although it looks like we still have many to convince ;-) We will take your comments/input on board as we determine what our next step will be. Would anyone at OpenStack be interested in working with us on any future development? 

 

 - Ability for developers to submit a topic branch for review with a

single push to Gerrit.

Yes.

- Summary page showing topics awaiting review, merged topics, etc.

This could be merged in with changes, but in such a case it would be

desirable to only show the topics rather than all commits in each

topic (which risks flooding the view).

Yes

- Same as above for dashboard and other views where only a single

entry is shown for a topic, ideally with inline expansion to show all

commits using an appropriate UI element.

Yes

- We need to receive an event when the tip of a topic branch changes

so we can trigger CI builds of the tip but not the individual commits

on a topic.

Aren't you just defining your CI system to be greedy here? What does that actually have to do with topics? I might have missed it earlier, but I'm under the assumption that topics stay within a single projects. If so, you could just use the native git dependencies and teach your CI system to only care about the furthest children.

I can't go into details, but we have a CI system that handles dependencies, does verification in batches of changes, and has a (per-project) atomic submit on success. We don't have topic or any other custom dependency stuff built into Gerrit to enable that. 

We have that too - and I'm happy to go in to details on it. We use a tool called zuul  http://ci.openstack.org/zuul/ which does the above. It also tests changes between related sets of projects. It's great. I'm more than happy to expand on it as much as possible.

I will take a look at zuul, thanks for the link. 

[Efabor Fabor]

Hi guys;

I sent an email to Chris regarding the progress of "Topic branch support in Gerrit" and he redirected me to this group.

I went through it and it is crystal cleat that this feature should make it to the mainline of Gerrit code.

I do not get it why it is hard for Gerrit guys to be convinced how badly we need this feature.

A BIG THANKS TO ALL GUYS WHO PUT EFFORT TO IMPLEMENT THIS FEATURE AND ALSO TO THE GERRIT TEAM.

Hopefully we will see this feature SOON.

Thanks

On Thursday, December 13, 2012 10:53:22 PM UTC, Chris Harris wrote:

Some of you may be aware that Deniz Turkoglu and I have been working on getting topic review support upstream. We have been trying to revive the work developed by Codethink and have rebased/rewritten several patches in the sequence:

https://gerrit-review.googlesource.com/#/q/owner:%22Pete+Fotheringham%22+status:open,n,z

We at Kitware have been using this functionality for some time now on various projects.

However, it seems that the general consensus is that this topic is too invasive/large and is unlikely to ever to be upstreamed. Martin Fick suggested rethinking the support in a way that will enable more of the existing change functionality to be reused rather than creating a new reviewable entity.

So with this in mind, my colleague Brad King and I have come up with a possible alternative (see below). We would very much appreciate feedback/comments/thoughts. We would like to try to determine if the community feels this is a viable solution before investing any time in implementation.

Many Thanks, 

Chris Harris

Definitions/Background

  - A “Topic” allows a group of one or more Changes to be reviewed together.

  - Each Topic has an external “Topic Name” for reference by humans and during pushes.

  - Each Topic has an internal “Topic-Id” representing the topic as a whole.

  - Only one Topic with a given Topic Name may be open at any one time.

      - Each open Topic Name will be mapped to its internal Topic-Id.

      - Once merged/abandoned the name can be reused and will get a new id.

  - A “Topic Rev” is one revision of a Topic, defined by its head commit and merge base(s), and conceptually equivalent to a Patch Set in a normal Change.

  - Topics can overlap i.e. a Change can appear in multiple open topics.

  - A Topic that contains an out-of-date Patch Set of a Change cannot be Submitted.

Approach

In order to reuse existing infrastructure for review, Gerrit will construct a Change to represent a Topic. For example, when a user pushes a topic branch as

 git push gerrit HEAD:refs/for/master/$topic_name

Gerrit will lookup/generate its Topic-Id and construct a commit equivalent to

 git commit-tree $topic_head^{tree} -p $topic_base

where $topic_head is the head commit of the topic and $topic_base is the merge base (or bases) against master.  The committer will be the configured “Code Review” user and the author will be the uploader.  The commit message will store meta data about the topic e.g.

 Topic

 (blank line)

 Topic-Id: $topic_id

 Topic-Rev: $topic_rev

 Topic-Head: $topic_head

 Topic-Name: $topic_name

where $topic_id is the Topic-Id and $topic_rev is the Topic Rev number.  Each push to a new or active Topic will construct a new Topic Rev and add a reference to it as

 refs/topics/$topic_id/1

 refs/topics/$topic_id/2

 refs/topics/$topic_id/3

 … 

The constructed “topic changes” in most senses will be treated as changes like any other, they would appear in dashboard etc. However, the rendering of the change page for a Topic will be a bit different:

  - The raw meta data commit message will not be rendered.  Instead a commit message will be generated that looks a lot like what might be generated when the topic is merged e.g. a list of commits in the topic as “%h %s” lines.

  - Each revision of the topic will be shown in a “Topic Rev $N” block similar to a normal “Patch Set $N” block.

  - Each “Topic Rev” block will list the changes associated with the Topic as links to the individual Patch Sets on their corresponding Change pages.

  - Each “Topic Rev” block will also show the normal file-wise diff computed from the artificial topic commit to show the net change.  It will be reviewable with line-wise comments like any other Patch Set.

The “topic change” can be review and merged at which point Gerrit would then also label the changes associated with it as merged.