Topic review functionality, redux

[Joel Dillon]

Hi,

  I've been tasked by Codethink with taking over Diego Celix's work on topic review functionality and getting it upstreamed. This work was a response to this issue -

http://code.google.com/p/gerrit/issues/detail?id=51

  and his original thread is here -

https://groups.google.com/d/topic/repo-discuss/5oRra_tLKMA/discussion

I've taken his work and integrated it with recent Gerrit upstream, available here -

https://github.com/jotheberlock/gerrit

I'm aware that Nokia have also taken and extended this work, and I've looked at their tree, but it also seems to include a lot of non-topic-review related changes. That being the case, what would be the best approach to getting either my tree or Nokia's upstreamed?

Regards,

    Joel

[Magnus Bäck]

On Wednesday, April 04, 2012 at 04:58 EDT,
Joel Dillon <joelr...@gmail.com> wrote:

> I've been tasked by Codethink with taking over Diego Celix's work on
> topic review functionality and getting it upstreamed. This work was a
> response to this issue -
>
> http://code.google.com/p/gerrit/issues/detail?id=51
>
> and his original thread is here -
>
> https://groups.google.com/d/topic/repo-discuss/5oRra_tLKMA/discussion
>
> I've taken his work and integrated it with recent Gerrit upstream,
> available here -
>
> https://github.com/jotheberlock/gerrit

Um, where are the original commits from Diego? It looks like you've
rebased his commits on the tip of the branch but replaced the commit
messages with less-than-useful messages. It also looks like many (most?)
of the commits should be squashed while at the same time some of them
should probably be split into logical units. From what I remember the
topic review feature is a rather big one, but without knowing much about
the feature or its impact on Gerrit I'd expect it be reasonably
representable by no less than 5 to 10 commits.

> I'm aware that Nokia have also taken and extended this work, and
> I've looked at their tree, but it also seems to include a lot of
> non-topic-review related changes. That being the case, what would be
> the best approach to getting either my tree or Nokia's upstreamed?

Like all other contributions, these commits should be uploaded for
review. Those related to the topic review functionality should probably
use Gerrit's current topic feature to keep them connected (though I
expect most commits will be in the same series anyway). Unrelated
commits should be extracted from the topic series to avoid unnecessary
dependencies.

I suppose Nokia's work needs to be uploaded by someone at Nokia with a
contributor agreement.

--
Magnus Bäck
ba...@google.com

[Joel Dillon]

Um, where are the original commits from Diego? It looks like you've
rebased his commits on the tip of the branch but replaced the commit
messages with less-than-useful messages. It also looks like many (most?)
of the commits should be squashed while at the same time some of them
should probably be split into logical units. From what I remember the
topic review feature is a rather big one, but without knowing much about
the feature or its impact on Gerrit I'd expect it be reasonably
representable by no less than 5 to 10 commits.

 I am, I will admit, fairly new to git for anything larger than my own personal
projects so I've probably done things in a rather suboptimal manner. I was
aware that Diego's previous submission wasn't considered atomic and
fine-grained enough, so I figured I should get an updated tree working with
his code then refactor the patches into something that would be useful and
acceptable for upstream (since this was going to be necessary in any case).
I was just trying to figure out where I should start.

[Marcus D. Hanwell]

There is a tree that we have been rebasing, testing in production, and
fixing existing bugs on. We announced its location quite a while back,
and would be happy to work with you and the Gerrit community in
getting Diego's patches and ours merged. Unfortunately, we are
currently pressed for time but that should change in the next week or
two.

https://github.com/cjh1/gerrit

Git and Gerrit are at the center of our workflow at Kitware, and we
have a long term interest in seeing the topic review functionality
merged. We are continuing to work on additional features that were not
in the original topic, and fix bugs as we find them. Kitware has an
agreement with Google in place, and so we can contribute our patches
to Gerrit. As many of them depend on the previous commits from Code
Think we have not been able to pursue upstreaming them.

Thanks,

Marcus

[Magnus Bäck]

On Thursday, April 05, 2012 at 09:22 EDT,

"Marcus D. Hanwell" <marcus....@kitware.com> wrote:

> There is a tree that we have been rebasing, testing in production,
> and fixing existing bugs on. We announced its location quite a while
> back, and would be happy to work with you and the Gerrit community
> in getting Diego's patches and ours merged. Unfortunately, we are
> currently pressed for time but that should change in the next week
> or two.

Famous last words :-)

> https://github.com/cjh1/gerrit

This looks much better and seems reviewable, but its base is fairly
old so a rebase is probably best before you upload it. At the same
time you can squash a couple of typo-correcting commits.

I don't see any documentation updates on the topic-reviews branch.
Documentation of this major feature could perhaps serve as a useful
introduction for reviewers and make it easier to discover flaws in
the workflow or its terminology?

[...]

--
Magnus Bäck
ba...@google.com