[ANN] Please use Gerrit from now on for Patch Review

classic Classic list List threaded Threaded
90 messages Options
Next » 12345 « Prev
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On Tue, Jun 19, 2012 at 02:05:05PM +0200, Jan Holesovsky wrote:
> > The second step however does not yet work as long as gerrit does not own the
> > repository. Thats why it is so damn important, that everyone gets his gerrit
> > account set up as requested a month ago, so that we can switch over completely.
>
> "damn important" will not get you there, I am afraid - only explaining,
> and making it no harder than the current workflow will do.

That mail one month ago was just asking to login to gerrit once and setup your
key -- not yet using it/playing with it (although that is appreciated too),
which is what I ask people to do now. I hope that we can switch over to gerrit
completely in another month and if there are a lot of people suddenly
"surprised" by the change(*), we might run out of smart, unsurprised people who
actually had a look at what is coming before.

So I guess the lesson is: Be smart, dont be one of the surprised people. ;)

Best,

Bjoern

(*) read not having set up and tested their keys
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please -play-with- Gerrit from now on for Patch Review ...

In reply to this post by Michael Meeks-2
On Tue, Jun 19, 2012 at 10:30:22AM +0100, Michael Meeks wrote:
> Personally I'd like to see where we're at with gerrit, how it works,
> get people trained up in the command-line tooling, and get a final
> sign-off from the ESC before we encourage world+wife to deploy this.

Note though that it is hard to directly judge most of the benefits of gerrit
right now.  Personally, I think the biggest gain will be in the better use of
tinderboxes for testbuilding something before it hits master. We are not having
the improvements there now, but we need gerrit as a prerequisite for that.

Also note that the workflows described on the wiki are more complex than they
will need to be in the end: Right now, instead of doing an approving review
(which will trigger gerrit to automatically push the change), you still have to
manually push to fd.o.

We should move through this intermediate state as fast as possible to get to a
pure gerrit setup in the end. Lets fight the "Nothing is as permanent as a
temporary solution" anti-pattern viciously here.

And indeed, everyone training themselves to use the command line tooling and
provide feedback and improvements on the current setup are the best way to get
there without hitting too many bumps in the road.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lubos Lunak Lubos Lunak
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Bjoern Michaelsen
On Tuesday 19 of June 2012, Bjoern Michaelsen wrote:

> On Tue, Jun 19, 2012 at 02:05:05PM +0200, Jan Holesovsky wrote:
> > > The second step however does not yet work as long as gerrit does not
> > > own the repository. Thats why it is so damn important, that everyone
> > > gets his gerrit account set up as requested a month ago, so that we can
> > > switch over completely.
> >
> > "damn important" will not get you there, I am afraid - only explaining,
> > and making it no harder than the current workflow will do.
>
> That mail one month ago was just asking to login to gerrit once and setup
> your key

 Which is the problem. Besides asking just to do that it should have also said
why one should do that. I'm one of the people who haven't done that,
because 'Do it because.' scores pretty low with my motivation. I wasn't
joking when I mentioned in some other mail that it would be nice if you
finally told us something about this Gerrit stuff that wasn't just a random
remark. I still have yet to see a good description of what Gerrit is actually
supposed to do for us exactly.

 And while I myself decided I wouldn't bother until it comes, right now it
appears to me like nobody has been really told, and that does bother me.

> -- not yet using it/playing with it (although that is appreciated
> too), which is what I ask people to do now. I hope that we can switch over
> to gerrit completely in another month

 And I hope that this 'completely' is the daydreaming part. I'm a bit hazy on
the details (since, you know, I've just picked them up randomly here and
there), but if I understand it correctly, we are asked to move exclusively to
a different workflow with an unproven tool. In a month. And I thought it was
a running joke that this codebase breaks every single tool that's use on it.

> and if there are a lot of people
> suddenly "surprised" by the change(*), we might run out of smart,
> unsurprised people who actually had a look at what is coming before.

 Sorry, but I think you got this backwards. If we run out of smart unsurprised
people to look at what's coming, it's not coming. Simple as that. Tools are
there for people, not people for tools.

> So I guess the lesson is: Be smart, dont be one of the surprised people. ;)

 Quite so. Seriously.

--
 Lubos Lunak
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-2 Michael Meeks-2
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review


On Tue, 2012-06-19 at 17:04 +0200, Lubos Lunak wrote:
>  Which is the problem. Besides asking just to do that it should have also said
> why one should do that. I'm one of the people who haven't done that,
> because 'Do it because.' scores pretty low with my motivation.

        My hope is that by encouraging the use of gerrit in parallel with the
mailing list, the benefits will become sufficiently obvious over time
that the old way of merging patches mailed to the list (while still an
option) will seem to be the more annoying way to do things ;-)

        So - I think we should give gerrit a break; and play with it and see
what happens - though I agree the absence of a 10 bullet TLDR; rational
has been a bit of a frustration; I'm sure we'll get past that.

        Either way - we'll discuss this at the ESC on Thursday if you want to
join in.

        HTH,

                Michael.

--
[hidden email]  <><, Pseudo Engineer, itinerant idiot

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Lubos Lunak
Hi,

On Tue, Jun 19, 2012 at 05:04:58PM +0200, Lubos Lunak wrote:
>  Which is the problem. Besides asking just to do that it should have also said
> why one should do that. I'm one of the people who haven't done that,
> because 'Do it because.' scores pretty low with my motivation.

I hoped "as you otherwise will be completely locked out of commiting, once we
switched over!" (as from that other mail) scores better in your motivation.

> I wasn't joking when I mentioned in some other mail that it would be nice if
> you finally told us something about this Gerrit stuff that wasn't just a
> random remark. I still have yet to see a good description of what Gerrit is
> actually supposed to do for us exactly.

http://nabble.documentfoundation.org/ANN-Please-use-Gerrit-from-now-on-for-Patch-Review-td3990754.html#a3991006

for starters, nothing of that being new, actually most of it being as old as:

LibreOffice Conference 2011, Friday, October 14, 2011 - Presentation "Crowdsourcing Code Reviews (Bjoern Michaelsen, Canonical Ltd)".
http://wiki.documentfoundation.org/images/8/85/Crowdsourcing_code_reviews.odp

>  And while I myself decided I wouldn't bother until it comes, right now it
> appears to me like nobody has been really told, and that does bother me.

Thats hardly helpful. There is a lot to room to shape, improve and form the way
we are using gerrit. "I dont know what it is, but I dont like its color" is not
a valid argument.

>  And I hope that this 'completely' is the daydreaming part.

No its not. The last thing we need is two repositories and admining them both
with fragile syncs etc.

> I'm a bit hazy on the details (since, you know, I've just picked them up
> randomly here and there), but if I understand it correctly, we are asked to
> move exclusively to a different workflow with an unproven tool.

No you are not. You will be able to bypass review on gerrit and commit directly
to master as you did before, _if_ you set up an account and allow us to get you
the ACL in gerrit set up right for you. Everyone who hasnt done that since a
month ago, is only hurting himself and costing the gerrit team a lot of nerves.

> In a month.  And I thought it was a running joke that this codebase breaks
> every single tool that's use on it.

Hmm, so how about getting used to the new possiblities _now_ so that we are not
doing hop over to reviewing patches on gerrit with 400 developers on _one_ day
with limited people available to spread the knowledge? Dont you think it might
help doing that over a month, rather than on one day? It might smooth things
out a bit, you know ...

So again, what we are doing now is:
- trying to get everyone get an account on a TDF-controlled git host, which in
  addition will enable us to continue to do all of the above
- Nothing but the remote host to push to changes by doing this
- Putting things under review in gerrit is currently not planned to ever be
  required -- but it is assumed to be soon the only/canonical way to do reviews
  for the benefit of all (and without major drawback as reviews will be doable
  from the commandline, from the web, possibly from IRC or via mail later too)

So this enables us to push things forward without ever taking anything from
you. You will be able to continue to push to master directly under gerrit.

However, I and I guess the rest of the gerrit team are rather confident, that
at one point peer pressure will make you wisen up: It is a lot more comfortable
to get one mail in private from a tinderbox that you broke the build, than one
that goes out to 50 people. If you made fragile changes and the the other 49
people did do the pre-master tinderbox builds, they have every right to be
annoyed by you.

So: There havent been and wont be any policy changes with regard to code
review. Once we have the tinderboxes lined up for this, I assume this to solve
itself and people doing the sane thing.

> Tools are there for people, not people for tools.

Which is exactly why we are constantly in trouble finding enough reviewers.
Because the current tooling -- or lack thereof -- sucks and I am confident I am
not alone with that opinion.

So dont complain against some daydreamed phantom of what you imagine gerrit to
be, but help us make it something doing what you need.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Petr Mladek Petr Mladek
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Bjoern Michaelsen
Bjoern Michaelsen píše v Út 19. 06. 2012 v 13:38 +0200:

> On Tue, Jun 19, 2012 at 11:13:45AM +0200, Petr Mladek wrote:
> > It means that gerrit should be able to detect patches for review on the
> > mailing list, integrate them, and make them ready for review.
> >
> > My expectation would be that it sends a replay to the mailing list with
> > a link to diff, link to build results and commands to approve it.
>
> Please provide constructive feedback to for example:
>
>  https://bugs.freedesktop.org/show_bug.cgi?id=51159
Ah, this bug is about a daily digest. I think that we first need to
decide how much we want to modify the current work flow. Do we want to
really move most discussions from the mailing list to gerrit?

How people will be noticed about news in gerrit? Is one day digest
really enough?

Also, you know, there is the "hello/thanks/see you" much more involved
in the mailing list. I think that such things are important for the
community live.

Note that the cooperation with mailing list is even in the plan, see:

        + setup mail notifications to the developer mailing list
        + setup patch drop mailbox
        + scripting to automagically identify patches on the mailing
          list

at
http://wiki.documentfoundation.org/Development/GerritMigration#Action_items


I think that gerrit should send mail to the mailing list immediately
when someone needs review (people without commit access, people that are
not sure about their fix).

For inspiration, I attach mail from the openSUSE Build Service. It
includes:

        + description of changes
        + diff of the spec file
        + commands how to get full diff, approve or reject the change



>  http://nabble.documentfoundation.org/gerrit-for-release-branches-bots-ftw-tp3990804.html

I am going to answer it but I am busy with some other things.

> implementing this "into the blind" just for it to be rejected by those who
> where silent before is not the way to go.

I think that my request is nothing new for you. I think that it was
actually one of the most important requests since the beginning.


Best Regards,
Petr



   home:fstrba/libcdr -> devel:libraries:c_c++/libcdr


   https://build.opensuse.org/request/show/124460

   Description: New upstream release

changes files:
--------------
--- libcdr.changes
+++ libcdr.changes
@@ -1,0 +2,7 @@
+Mon Jun 11 14:32:55 CEST 2012 - [hidden email]
+
+- Update to upstream version 0.0.8
+  * initial text support
+- Remove upstreamed patch
+
+-------------------------------------------------------------------

old:
----
  libcdr-0.0.7-clang.patch
  libcdr-0.0.7.tar.xz

new:
----
  libcdr-0.0.8.tar.xz

spec files:
-----------
--- libcdr.spec
+++ libcdr.spec
@@ -28,13 +28,12 @@
 BuildRequires:  gcc-c++ pkgconfig xz zlib-devel liblcms2-devel
 BuildRequires:  libwpd-devel >= 0.9.0 libwpg-devel >= 0.2.0
 Summary:        Library for parsing the Corel Draw file format structure
-Version:        0.0.7
+Version:        0.0.8
 Release:        0
 License:        LGPLv2+, GPLv2+, MPL1.1
 Group:          Productivity/Publishing/Word
 Url:            http://www.freedesktop.org/wiki/Software/libcdr
 Source:         <a href="http://dev-www.libreoffice.org/src/libcdr-%">http://dev-www.libreoffice.org/src/libcdr-%{version}.tar.xz
-Patch0:         libcdr-0.0.7-clang.patch
 BuildRoot:      %{_tmppath}/%{name}-%{version}-build
 
 %description
@@ -86,7 +85,6 @@
 
 %prep
 %setup -q
-%patch0 -p1
 
 %build
 %configure --disable-static --docdir=%_docdir/%name

other changes:
--------------

++++++ libcdr-0.0.8.tar.xz (new)

++++++ deleted files:
--- libcdr-0.0.7-clang.patch
--- libcdr-0.0.7.tar.xz


To REVIEW against the previous version:
   osc request show --diff 124460

To ACCEPT the request:
   osc request accept 124460 --message="reviewed ok."
   
To DECLINE the request:
   osc request decline 124460 --message="declined for reason xyz (see ... for background / policy / ...)."

To REVOKE the request:
   osc request revoke 124460 --message="retracted because ..., sorry / thx / see better version ..."
--
Hermes messaging (http://hermes.opensuse.org)
openSUSE Build Service (https://build.opensuse.org/)
Collaboration: http://en.opensuse.org/Build_Service/Collaboration



_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

Hi Petr,

On Tue, Jun 19, 2012 at 06:14:18PM +0200, Petr Mladek wrote:
> Ah, this bug is about a daily digest. I think that we first need to
> decide how much we want to modify the current work flow. Do we want to
> really move most discussions from the mailing list to gerrit?

IMHO yes, they are noise on the mailing list as you need deep context to follow
them. In gerrit you can even comment inline in the patch.

> How people will be noticed about news in gerrit?

Reporters and Reviewer will get mails about "their" patches. Unless they prefer
a different workflow in which case they can change their preferences. A lot
better than what we have now.

> Is one day digest really enough?

For the mailing list, yes. But I might make sense to teach one of our IRC bots
to blurp out updates in realtime to #libreoffice-dev.
One might consider it useful to send _one_ mail to the list once the patch goes
in with a summary of the history, but I personally guess the majority is not
interested in reading that and the few that are can look it up in gerrits web
interface.

> Also, you know, there is the "hello/thanks/see you" much more involved
> in the mailing list. I think that such things are important for the
> community live.

Well, we should still try to get people on the mailing list (and it will be
easier with less traffic there). Asking a first-time contributor to subscribe
(and send his license blurp) is a good thing. A lot of volunteers told me
though they unsubscribed because of the volume of mails on the dev list. And
the PATCH mails arent exactly those that are socially attractive.

> + setup mail notifications to the developer mailing list

Yes, and Robert is on that bug (ETA: next week). Otherwise, I will jump in.

> + setup patch drop mailbox
yes, on my todo and halfway there:
 https://launchpad.net/~r-gerrit-0 (bot-id)
 [hidden email] (bot email)

Design proposals on the email format etc. welcome.


> + scripting to automagically identify patches on the mailing
>           list

IMHO, we should drop that in favor of explicit forward to
[hidden email]. "Guessing" the target branch is tricky, and a simple
forward (with possibly tweaking the subject to something botreadable) shouldnt
be too hard.

> For inspiration, I attach mail from the openSUSE Build Service. It
> includes:
>
> + description of changes
> + diff of the spec file
> + commands how to get full diff, approve or reject the change

I still think we should reduce the traffic on the dev ml, but having that as
IRC bot would be nice. Such mails are kinda nice for full-time employees, but
for everyone else they are overkill IMHO.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Jan Holesovsky
On Tue, Jun 19, 2012 at 8:09 AM, Jan Holesovsky <[hidden email]> wrote:

> Hi Bjoern,
>
> On 2012-06-19 at 12:50 +0200, Bjoern Michaelsen wrote:
>
>> (*) That is, if you did the initial setup there on time. You were all asked to
>> get yourself an account on gerrit more than a month ago. Dont blame the admins
>> if you stumble in with 400 others in the last minute.
>
> The wiki page says you have to create a ssh key; I hope it is not
> necessary, and one can reuse his fd.o key?  If yes - can we automate
> that in any way, to save the admins work?

The 'admin' work is to put people in the right group...
but to do that the user need to be created which is done when someone
regsiter with an openid
that is not related with the ssh-key which each user set-up on his own.
(and yes you can use which ever key you want.. you can even have more than one)

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Petr Mladek Petr Mladek
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Bjoern Michaelsen
Bjoern Michaelsen píše v Út 19. 06. 2012 v 18:40 +0200:
> Hi Petr,
>
> On Tue, Jun 19, 2012 at 06:14:18PM +0200, Petr Mladek wrote:
> > Ah, this bug is about a daily digest. I think that we first need to
> > decide how much we want to modify the current work flow. Do we want to
> > really move most discussions from the mailing list to gerrit?
>
> IMHO yes, they are noise on the mailing list as you need deep context to follow
> them. In gerrit you can even comment inline in the patch.

Sounds good but how many people would know about the comments? How hard
would be to find them?

> > How people will be noticed about news in gerrit?
>
> Reporters and Reviewer will get mails about "their" patches. Unless they prefer
> a different workflow in which case they can change their preferences. A lot
> better than what we have now.

Sure but who will be the reviewer? The mailing list has the advantage
that people step in when they are interested. It helps to balance the
workload. Also it is very open for new reviewers. I am afraid that
gerrit could make some people fell like reviewing robots.

> > Is one day digest really enough?
>
> For the mailing list, yes. But I might make sense to teach one of our IRC bots
> to blurp out updates in realtime to #libreoffice-dev.
> One might consider it useful to send _one_ mail to the list once the patch goes
> in with a summary of the history, but I personally guess the majority is not
> interested in reading that and the few that are can look it up in gerrits web
> interface.

Let's discuss this on the ESC meeting. I am afraid that people will not
look into gerrit regularly. For example, I open bugzilla only when I get
a mail about a change or when I see an interesting bug number in mail
conference or irc.

People have only limited time to monitor mailing lists, irc, other
tools. We need to be careful about adding too many channels to monitor.


> Well, we should still try to get people on the mailing list (and it will be
> easier with less traffic there). Asking a first-time contributor to subscribe
> (and send his license blurp) is a good thing. A lot of volunteers told me
> though they unsubscribed because of the volume of mails on the dev list. And
> the PATCH mails arent exactly those that are socially attractive.

It sounds reasonable. The question is if people will still monitor the
mailing list if there are not interesting patches. Well, I do not want
to be too negative. We need to try it. On the other hand, we need to be
ready to solve these problems ;-)

> > + setup mail notifications to the developer mailing list
>
> Yes, and Robert is on that bug (ETA: next week). Otherwise, I will jump in.

Sounds good


> > + setup patch drop mailbox
> yes, on my todo and halfway there:
>  https://launchpad.net/~r-gerrit-0 (bot-id)
>  [hidden email] (bot email)
>
> Design proposals on the email format etc. welcome.

I am sure that people will have many good ideas once they see how it is
supposed to work.


> > + scripting to automagically identify patches on the mailing
> >           list
>
> IMHO, we should drop that in favor of explicit forward to
> [hidden email]. "Guessing" the target branch is tricky, and a simple
> forward (with possibly tweaking the subject to something botreadable) shouldnt
> be too hard.

If the gerrit commandline interface is easy, anyone would be able to
push it for review quickly. We need easy interface anyway.


> > For inspiration, I attach mail from the openSUSE Build Service. It
> > includes:
> >
> > + description of changes
> > + diff of the spec file
> > + commands how to get full diff, approve or reject the change
>
> I still think we should reduce the traffic on the dev ml, but having that as
> IRC bot would be nice. Such mails are kinda nice for full-time employees, but
> for everyone else they are overkill IMHO.

We must not increase the traffic. If active reviewers want notification,
we need to provide it. Let's discuss this on ESC.


Best Regards,
Petr

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

Hi,

On Tue, Jun 19, 2012 at 07:24:30PM +0200, Petr Mladek wrote:
> Sure but who will be the reviewer? The mailing list has the advantage
> that people step in when they are interested. It helps to balance the
> workload. Also it is very open for new reviewers. I am afraid that
> gerrit could make some people fell like reviewing robots.

There is no "default reviewer", so patches would just wait for somebody to pick
them up, just like on the list. Add just like you can CC someone on the list,
you can set a reviewer when submitting the patch. But in addition someone
looking into the submitted change can say "I have no idea about this area of
code, but foo might", just like on the list, but with less noise. If some devs
get to be reviewing robots that is unfortunate and we would need to find a
solution, but I doubt that to be related to gerrit, it would be the same on the
list -- just with more noise.

Note that you can watch for changes matching some criteria:

 http://gerrit.googlecode.com/svn/documentation/2.1.6/user-search.html

and get an email, when one of those arrives. So you can get an email for
patches that touch "^sw/" or "^sc/" without having to wade through the rest. I
know it takes courage to self-inflict yourself to be spammed for incoming
patches, but if we core devs do that with changes we feel confortable about, we
can make life easier for everyone, as there hopefully will be only few patches
to review left after everyone takes care about the ones he feels comfortable
about (and we shorten a lot of the "I dont know about that one, foo care to
take a look?" noise). It makes you make your quota of reviews with patches you
actually care about, which is kinda nice, I guess.

> Let's discuss this on the ESC meeting. I am afraid that people will not
> look into gerrit regularly. For example, I open bugzilla only when I get
> a mail about a change or when I see an interesting bug number in mail
> conference or irc.
>
> People have only limited time to monitor mailing lists, irc, other
> tools. We need to be careful about adding too many channels to monitor.

see above, I guess a daily digest to the ml and a watch on the modules you care
about will get better focused information for everyone. Also note that some
people might prefer to browse the patches on gerrit. I for one would prefer to
wade through some patches en bloc in the morning and then proceed to the
interesting discussions on the dev list rather than having both sprinkled into
each other.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Tor Lillqvist-2 Tor Lillqvist-2
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

I find it peculiar that in the discussion here people keep talking
about patches, as if gerrit was only a "patch" review tool. At least I
understand "patch" to mean a relatively local change to the code in
order to fix some specific bug. But if the intent is that *all*
changes are to go through gerrit, surely the majority of changes
(number-of-lines-wise, not number-wise) going through it will be
feature work and cleanups, not patches?

Will this mean people will start doing less refactoring cleanups, for
instance, in order to make their change sets smaller, to increase the
possibility of somebody reviewing the, eh, "patch"?

--tml
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
David Ostrovsky David Ostrovsky
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Petr Mladek
Hi Petr, all,

i am using gerrit for a while now and gathered some experience with it
already and would like
to share it with you.

On 19.06.2012 19:24, Petr Mladek wrote:

> Bjoern Michaelsen píše v Út 19. 06. 2012 v 18:40 +0200:
>> Hi Petr,
>>
>> On Tue, Jun 19, 2012 at 06:14:18PM +0200, Petr Mladek wrote:
>>> Ah, this bug is about a daily digest. I think that we first need to
>>> decide how much we want to modify the current work flow. Do we want to
>>> really move most discussions from the mailing list to gerrit?
>> IMHO yes, they are noise on the mailing list as you need deep context to follow
>> them. In gerrit you can even comment inline in the patch.
> Sounds good but how many people would know about the comments? How hard
> would be to find them?
https://gerrit.libreoffice.org/#/c/179/4/
(may be you need to login into gerrit with your openId)
You can see it immediatelly: if and how much and for wich file exactly.

For me this is one of the most valuable features of gerrit: inline comments.
On comment column from 17 files Michael has commented in 5 files.
This is already really good, isnt't? But it going to be even better:
the submitter can respond (and he surely will, if he doesn't understand
what the reviewer meant):
in the context of this file/line.

Still not convinced? How about this:
12 comments from different peoples on only one line?
https://gerrit-review.googlesource.com/#/c/35380/3/gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java

You can try to simulate this flow in mail with patch(es)...

>>> How people will be noticed about news in gerrit?
>> Reporters and Reviewer will get mails about "their" patches. Unless they prefer
>> a different workflow in which case they can change their preferences. A lot
>> better than what we have now.
> Sure but who will be the reviewer?
In my case (gbuild'ification of dmake module) the usual suspects are
Stefan, David Tardon, Michael Stahl, Matus and Björn.
;-)
I guess every LO-area has their own specalists.

> The mailing list has the advantage that people step in when they are interested.
I agree with you as far as globally interested topics are discussed.
But with my (build specific) questions only build experts were able to help.
But then the context was wrong anyway: Mail to ML with [GERRIT] in topic
or [PATCH] for that matter and reall special questions in the body,
like how to fix broken pyuno bridge on mingw platform (because no python
is currently get packaging in the installation set there).
Those mail created noise on the ML, but those two people who answered
were already on the CC list in the mentioned gerrit patch.
So I would better just commented something there (inline) and they would
be notify anyway and respond to me (without noise) and in the context of
this gerrit patch.

Another most valuable gerrit feature for me is the preserving of patch
history (many change sets on gerrit) and still having only one
commit in the real repository. This magic is achived through the
combination of ChangeId git hook,
git rebase -i command and gerrit's handling of ChangeId as the context
of gerrit patch.

This flow may be not so obvious:

git commit # some changes (git ChangeId hook generate a fresh ChangeId)
git push logerrit HEAD:refs/for/master # gerrit patch with first change
set is created.
git commit # further changes
git rebase -i HEAD~2 # with fixup preserves the same ChangeId
git push logerrit HEAD:refs/for/master # second change set in the same
(!) gerrit patch is created (same ChangeId)

And the best is: you can see the evolution of the patch (with command
line tool, gerrit web interface or gitweb/cgit).

I got one question with gerrit so far:
how can other people contribute code snippet into foreign gerrit patch
(so called extend it)?
During my work on gbuildi'fication of pyuno module Stefan helped me with
some scp2, Windows and Mac OS X specific stuff.
But he can not put a change set into my gerrit patch.
So he created a couple of patches and sent it to ML, I applied the
patches and pushed the next iteration to gerrit.
While is was working, this flow obviously violates the common repository
rule: every contribution should hit repository with the name of the
right contributor.
Sure we could create a new integration branch, like lately gbuild_merge
branch and push our own gerrit patches there...
Ideas on this?

Regards
David
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Tor Lillqvist-2
Hi Tor,

On Tue, Jun 19, 2012 at 10:14:27PM +0300, Tor Lillqvist wrote:
> But if the intent is that *all* changes are to go through gerrit,

Its not. As said repeatedly on this thread already, everyone who has an fd.o
account will be able to continue to push to master. However, the hope is that
the convenience of having a windows build before it hits master and being able
to fix it without time pressure and 20 angry other devs shouting at you on IRC
will convince you to use it for more and more stuff.

> surely the majority of changes (number-of-lines-wise, not number-wise) going
> through it will be feature work and cleanups, not patches?

Once we tune the tinderboxes to build stuff submitted to review there, having
these build will be quite nice esp. for features etc. And if your patch is in
danger of rotting away with nobody giving it love, the tinderboxes where happy
with it, you can still push it directly to master on you own risk. But even
there, I think asking another dev on IRC "hey Im confident with that, the
tinderboxes are fine with it, could you give it a quick review otherwise I
would push myself" would give you what you want. Its all checks and balances:
Nobody will be powerplayed and not get his stuff in, but there still might
someone else at least skimming over it in addition now.

Its a bit different for those who do not have direct commit access -- they can
only hope for somebody reviewing and pushing their change. But that is also not
different from before for them. If anything changes, its that we can give them
direct commit access quicker and with less hassle now. OTOH, the hope is, that
the urgent need for that is shrinking too anyway.

> Will this mean people will start doing less refactoring cleanups, for
> instance, in order to make their change sets smaller, to increase the
> possibility of somebody reviewing the, eh, "patch"?

No.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lionel Elie Mamane Lionel Elie Mamane
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Bjoern Michaelsen
On Mon, Jun 18, 2012 at 12:09:49PM +0200, Bjoern Michaelsen wrote:

>  http://sweetshark.livejournal.com/13298.html

> gerrit is documented and ready to go.

It refuses to take patches (commits) whose author field is not an
email address registered in my account.

How do I submit for review a patch authored by someone else
(e.g. one-off contributor that does not want to go through the hassle
of setting up for OpenID, git, gerrit, ...)?

--
Lionel
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On Wed, Jun 20, 2012 at 4:21 AM, Lionel Elie Mamane <[hidden email]> wrote:
> On Mon, Jun 18, 2012 at 12:09:49PM +0200, Bjoern Michaelsen wrote:
>
>>  http://sweetshark.livejournal.com/13298.html
>
>> gerrit is documented and ready to go.
>
> It refuses to take patches (commits) whose author field is not an
> email address registered in my account.

You need to be added to a group that allow that.
>
> How do I submit for review a patch authored by someone else
> (e.g. one-off contributor that does not want to go through the hassle
> of setting up for OpenID, git, gerrit, ...)?

how does one create a patch without git ?
and isn't the whole point of OpenID is 're-use'.
so you are left with the step of adding a ssh key in your profile...

anyway...
Anonymous Uer are only allow to read
Registered User do not have the authority to 'Forge' Author and/or Committer
people in the Commiter Group can... so you need to be added to it

somehow I cannot find you're id in Gerrit, so I cannot add you to the
right 'group'

So I'm wondering how were you even trying to push a patch ?

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lionel Elie Mamane Lionel Elie Mamane
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On Wed, Jun 20, 2012 at 04:31:21AM -0500, Norbert Thiebaud wrote:
> On Wed, Jun 20, 2012 at 4:21 AM, Lionel Elie Mamane <[hidden email]> wrote:
>> On Mon, Jun 18, 2012 at 12:09:49PM +0200, Bjoern Michaelsen wrote:

>>> gerrit is documented and ready to go.

>> It refuses to take patches (commits) whose author field is not an
>> email address registered in my account.

> Anonymous Uer are only allow to read
> Registered User do not have the authority to 'Forge' Author and/or Committer
> people in the Commiter Group can... so you need to be added to it

Ah, I understand. When the gerrit repos are the "true one source" and
gerrit will do the "push" automatically once someone validates the
patch in the web interface, what will "Committer" be? The one that
uploaded the patch or the one that validated it in the web interface?
IMHO it would be nice if it would be the one that validated in the web
interface.

>> How do I submit for review a patch authored by someone else
>> (e.g. one-off contributor that does not want to go through the hassle
>> of setting up for OpenID, git, gerrit, ...)?

> how does one create a patch without git ?

With "diff --recursive" or quilt or debdiff (after building custom
Debian packages) or ... It is not inconceivable for people to hack on
the sources distributed by their distros rather than upstream sources;
I often do that for software where I just want to fix this one bug,
because "apt-get source package" is so much easier than "find where
the hell upstream sources are, use specific tool I might not be
familiar with (git, darcs, svn, mercurial, ... there are so many)".

Anyway, I meant "setup git for gerrit with SSH keys and all that".

> and isn't the whole point of OpenID is 're-use'.

And? That I might be able to 're-use' on another website in 5 years
does not make it any easier or more attractive to go through a
double-plus-more-complicated setup to get an OpenID identity
(double-plus-more-complicated compared to clicking "register" and
copy-pasting a random password generated by pwgen or "dd
if=/dev/urandom count=1 bs=9 | uuencode -m foo". Plus the though
choice of "OK, who (which OpenID provider) do I want to give the power
to impersonate me"?)

To add insult to injury, I cannot 're-use' *any* of my *existing*
reusable authentication methods. I have an OpenPGP card, SSH keys in
files, I have X.509 client certs (in files, on smartcard, ...),
etc. Noooo, it has to be *yet* *another* method.

Me personally, as a vested LibreOffice contributor, OK, I make efforts
for the common good so that we can have a better patch review /
handling mechanism (although I *do* *not* understand *at* *all* why we
have to require an OpenID, instead of offering it as an
alternative). If I were a first-time contributor, frankly, I'd stop
right there or at best dump the patch to the Mailing List anyway (or
in the bug tracker).

> somehow I cannot find you're id in Gerrit, so I cannot add you to the
> right 'group'

> So I'm wondering how were you even trying to push a patch ?

I'm using a throw-away account for testing / playing, with an OpenID
that is NOT GUARANTEED to be only me, so even if you guess which
account that is, do not give it *any* privilege we don't want "any
random person" to have.

--
Lionel
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On Wed, Jun 20, 2012 at 12:26:23PM +0200, Lionel Elie Mamane wrote:
> Ah, I understand. When the gerrit repos are the "true one source" and
> gerrit will do the "push" automatically once someone validates the
> patch in the web interface, what will "Committer" be? The one that
> uploaded the patch or the one that validated it in the web interface?
> IMHO it would be nice if it would be the one that validated in the web
> interface.

Author will be the whoever was the original author. Commiter will be be the
guy/gal pressing the submit button. Everyone giving a codereview+1 will be a
signoff in the commit.

> With "diff --recursive" or quilt or debdiff (after building custom
> Debian packages) or ... It is not inconceivable for people to hack on
> the sources distributed by their distros rather than upstream sources;

IMHO yes, distro build are a) patched b) usually an older major release and
thus patching created against them are possible trouble. That said, we still
need the patch maildrop, but for exact those reasons its not totally trivial.
Once there though, it will be better than now, as the conflicts are noted early
on (on submittal) and not late when the reviewer is trying to do something with
it.

> I often do that for software where I just want to fix this one bug,
> because "apt-get source package" is so much easier than "find where
> the hell upstream sources are, use specific tool I might not be
> familiar with (git, darcs, svn, mercurial, ... there are so many)".
>
> Anyway, I meant "setup git for gerrit with SSH keys and all that".

Maildrop will be of some help, but in general people should be encouraged to
get to upstream as the conflict changes between majors are too high. (Consider
someone on Ubuntu LTS sending his 3.5 patch in 2 years).

> (although I *do* *not* understand *at* *all* why we have to require an
> OpenID, instead of offering it as an alternative)

http://xkcd.com/927/
OpenId is currently the most common authentication esp. for non-nerds (and we
should try to consolidate on one authenication method, OpenId is currently the
best guess).

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by David Ostrovsky
On 06/19/2012 09:32 PM, David Ostrovsky wrote:

> On 19.06.2012 19:24, Petr Mladek wrote:
>> Sounds good but how many people would know about the comments? How hard
>> would be to find them?
> https://gerrit.libreoffice.org/#/c/179/4/
> (may be you need to login into gerrit with your openId)
> You can see it immediatelly: if and how much and for wich file exactly.
>
> For me this is one of the most valuable features of gerrit: inline
> comments.
> On comment column from 17 files Michael has commented in 5 files.
> This is already really good, isnt't? But it going to be even better:
> the submitter can respond (and he surely will, if he doesn't understand
> what the reviewer meant):
> in the context of this file/line.

Still, this removes the comments from many people's (potential) sight.
The IMO big advantage of the "everything on a single mailing list"
approach is that everybody is forced ;) to see everything (modulo
information overload), so that e.g. a comment given on one contributor's
patch is picked up "by osmosis" by other contributors too (so one would
hope).

I know there is no golden road to spreading information most
effectively, but I personally tend to prefer spreading/consuming too
widely over too narrowly.

> I got one question with gerrit so far:
> how can other people contribute code snippet into foreign gerrit patch
> (so called extend it)?
> During my work on gbuildi'fication of pyuno module Stefan helped me with
> some scp2, Windows and Mac OS X specific stuff.
> But he can not put a change set into my gerrit patch.
> So he created a couple of patches and sent it to ML, I applied the
> patches and pushed the next iteration to gerrit.

To be honest, the main reason I just dumped my changes onto the ML is
that I couldn't get comfortable with the gerrit web UI.  But hopefully
the command line (which I haven't started to use yet) will suite me
better...

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: [libreoffice-projects] [ANN] Please use Gerrit from now on for Patch Review

On Wed, Jun 20, 2012 at 02:11:31PM +0200, Stephan Bergmann wrote:
> Still, this removes the comments from many people's (potential)
> sight. The IMO big advantage of the "everything on a single mailing
> list" approach is that everybody is forced ;) to see everything
> (modulo information overload)

I can assure you that I am not forced to see your comments on the mailing
list. Indeed, unless I am on CC or the subject sounds very thrilling the mail
body never passes my eye.

So, IMHO that advantage not only has its drawbacks (information overload), it
is also mostly an illusion. Peoples forced _potential_ sight isnt really
helping us here at all.

And the drawback (information overload) is hitting those hardest, who have not
finetuned their procmail for month/years -- read: newcomers and volunteers.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lubos Lunak Lubos Lunak
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Bjoern Michaelsen
On Tuesday 19 of June 2012, Bjoern Michaelsen wrote:
> On Tue, Jun 19, 2012 at 05:04:58PM +0200, Lubos Lunak wrote:
> >  Which is the problem. Besides asking just to do that it should have also
> > said why one should do that. I'm one of the people who haven't done that,
> > because 'Do it because.' scores pretty low with my motivation.
>
> I hoped "as you otherwise will be completely locked out of commiting, once
> we switched over!" (as from that other mail) scores better in your
> motivation.

 I might have, had I noticed, but since this is an end of a sentence in the
middle of the mail inbetween two big blocks of commands, that was not the
case. And even if I had noticed, with the info I did (not) have, it's a
question what conclusion I would have drawn from it. As far as I was
concerned, it was a mail about getting accounts for some optional patch
review tool.

 If you don't want people to miss important information, you need to announce
it properly and not as some offhand remark. And announcing properly is not a
talk at conference to a limited audience, not random comments wherever, nor
mail asking for setting up an account without futher info (except for the
well-hidden remark). It may be nothing new to you, since you know it, but it
is new for people you haven't told.

 This is not about Gerrit itself. This is about the way it has (not) been
communicated.

--
 Lubos Lunak
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Next » 12345 « Prev