[ANN] Moving from Rietveld to Gerrit

[Andrew Bonventre]

Summary

The Chromium project is moving away from Rietveld to a revamped version of Gerrit for our main Code Review tool.

When?

The conservative estimate is Q4 of 2016. There are already some Chromium teams dogfooding the new Polymer-based UI (called PolyGerrit) and we are using their feedback to ensure that there is both a smooth transition and developer needs are met. This process will continue as we onboard more people.

Why?

We (Chromium) are the only customers of Rietveld (codereview.chromium.org and its internal counterpart) and maintenance of the tool has been delegated to a single person. The codebase has become a liability over the years and speed has become a major issue.

Gerrit is a fast, secure, and fully-featured code review tool built as a successor to Rietveld. It is already used by some Chrome subteams along with the Android, Go, and Bazel teams (as well as many others both inside and outside Google). It is also a fully staffed project with heavy investment from Google. This also means full-time UX support.

Its front-end is being re-written using Polymer, a standards-based library that allows us to take advantage of Web Components. This UI is the one that Chromium will be transitioning to (though you are welcome to use the old UI if you like).

FAQ

Can I see current progress on PolyGerrit?

Yes. Download this extension and then visit https://chromium-review.googlesource.com. You can then toggle between the two UIs.

How can I start dogfooding it now?

We still have plenty to do on the infrastructure side of things before many teams can move over. If you have a sub-project you think may qualify for early migration, email me and +Andrii Shyshkalov.

How can I follow progress?

All bugs related to the migration have the label Proj-Gerrit-Migration. Additionally, on the Gerrit project, there is a list of Chromium-specific bugs.

What will happen to Rietveld?

Rietveld will be put in read-only mode to preserve change history. Potential migration of changes to Gerrit will be considered after the switch. A unified CL search interface is being strongly considered.

Why not continue to just maintain Rietveld?

The Chromium infra team is stretched far too thin as it is and keeping the status quo of having one or no owner for the Rietveld codebase isn’t tenable given its fragility. The bus factor is just too low. That coupled with the duplication of effort across the Gerrit and Rietveld projects, it doesn’t make strategic sense. We would rather focus on more important work that will bring immediate benefit to Chromium developers.

Why don’t you move the current Rietveld v2 UI on top of Gerrit?

The current Rietveld v2 UI is written in Polymer 0.5, which is no longer supported by the Polymer team. There is a non-trivial amount of work that will go into porting it to 1.0. In addition, there are certain workflows within Gerrit that must be supported, so blindly porting the new Rietveld UI could potentially omit those workflows and cause confusion.

Will this change my current command-line development workflow?

No. git cl will support a Rietveld-style workflow with Gerrit transparently. You can follow this tracking bug to be informed on feature parity with git cl on Rietveld.

I heard I can’t “LGTM with nits.” This is a huge productivity loss if I’m working with people across timezones. Will I have to approve again if the author uploads another patchset?

No. Being able to submit a commit that doesn't exactly match the one that got approved is a per-project configuration option — you can choose in the config whether approvals are “sticky” or only per-patchset.

Are you going to go inside a hole for six months, return with a new tool, and then force everyone to use it?

No. Developer feedback (both from Chrome devs and current Gerrit users) will be crucial for the UI rewrite to be a success. We will be as transparent as possible with progress and will establish a feedback loop very early on. This isn’t about just alleviating maintenance burden, but making a tool that is crucial to everyone’s workflow better.

If you have any questions or need clarification, please don’t hesitate to reach out.

Andy

[Stefan Zager]

I'm mostly looking forward to this, but I have a couple of questions...

On Tue, Jul 12, 2016 at 2:24 PM, Andrew Bonventre <andy...@chromium.org> wrote:

Will this change my current command-line development workflow?

No. git cl will support a Rietveld-style workflow with Gerrit transparently. You can follow this tracking bug to be informed on feature parity with git cl on Rietveld.

1) Will we be able to use an idiomatic gerrit command-line workflow, e.g.:

$ git remote add gerrit https://chromium-review.googlesource.com/chromium/src

$ git push gerrit HEAD:refs/for/master

?

2) Will you support gerrit-style workflows for chained commits, e.g.:

$ git checkout -b work origin/master

# make changes

$ git commit -a -m 'First commit'

#  make changes

$ git commit -a -m 'Second commit'

$ git push gerrit HEAD:refs/for/master

# Two dependent CL's created.

$ git cl land <first CL>

# First commit lands on refs/pending/heads/master, git numbering daemon synthesizes commit to refs/heads/master

$ git cl land <second CL>

# gerrit magically knows how to rebase this?

[Andrew Bonventre]

Thanks rsleevi for pointing out the second link for Chromium-specific Gerrit issues is broken. Corrected one: https://bugs.chromium.org/p/gerrit/issues/list?q=label:Hotlist-Chromium

For using the idiomatic Gerrit workflow, nothing will stop you from doing so, but mixing it and git cl will be problematic during the transition period. As our tooling gets more focused (git cl supports git, svn, rietveld, AND gerrit) these kinds of workflows will become better supported by our tools.

The goal is to ensure current Rietveld users don’t experience a significant disruption in workflow. Once the migration is complete, then we can improve on them incrementally.

[yan...@google.com]

What will happen to existing code review URLs? These URLs can be found all over the place in the commit history and the code review itself often provide useful historical information.

Yang

[Paweł Hajdan, Jr.]

+infra-dev

[Emil A Eklund]

On Wed, Jul 13, 2016 at 12:35 AM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> +infra-dev
>
> On Wed, Jul 13, 2016 at 7:27 AM, yangguo via blink-dev
> <blin...@chromium.org> wrote:
>>
>> What will happen to existing code review URLs? These URLs can be found all
>> over the place in the commit history and the code review itself often
>> provide useful historical information.

That's one of my biggest concerns as well.

Please make sure that all historic codereview URLs keep working
indefinitely, whether this is through migration, keeping it read only
or dumping static html files for each issue doesn't really matter as
long as the content remains accessible at the original URL.

Code review revisions and comments are often crucial when doing code
archaeology.

Also, I'm excited to finally have a code review tool that's maintained
and has a future! Once it's ready are chromium committers
allowed/encouraged to contribute to make it match our workflows or is
it a separate project that we have little control over?

[Andrew Bonventre]

As noted in the original email:

What will happen to Rietveld?

Rietveld will be put in read-only mode to preserve change history. Potential migration of changes to Gerrit will be considered after the switch. A unified CL search interface is being strongly considered.

We do not intend to break links or lose review history. Don’t worry.

[Andrew Bonventre]

As for contributions, there is already a fairly robust plugin system with a JS plugin system design (that will live on top of the current JS API) in the works, so for specific workflows, I would suggest writing a plugin. For features that would benefit the entirety of Gerrit users, contributions are definitely welcome and encouraged.

[Primiano Tucci]

Generally very excited to see this coming (below why)

Bunch of questions:

1. Any idea about what the config for the chromium/src project is going to be? Is it going to be "rebase if necessary?" (which gives linear history and supports CL dependencies).

2. Will the case of dependent CLs still be supported (so you can git cl try something which is N branches from master and the trybots will do the right thing as they do recently)?

3. What happens to merge workflow? Will it still work? This can be tricky. Today you can update upstream changes by merging (as opposite to rebasing) and everything works (I believe) because rietveld's upload.py just sends the diff against origin/master (or whatever tracking branch). Have you tested what happens in this new gerrit world? Does the recently introduced auto-squash do the right thing?

I am not sure how many people are currently using a merge workflow. One thing I fear is that some people might be merging without realizing because git pull does a merge by default.

4. Are we still keeping gnumbd (the thing that adds the Cr-Commit-Position yada yada) (I hope so). Does it mean that the reviews will actually happen on a different branch (e.g. refs/for/pending/master or similar)?

Now the things that make me very excited are the following, can you confirm if they will actually work or we won't support them for some reason?

- I am guaranteed that I can always check out somebody else's CL and be exactly in their state, I don't have to guess which is the right revision to apply a patch.

- Other people can contribute to my patchsets. this might be tricky from a security/auditing viewpoint but is something I'd like to a certain extent (use case: some colleagues leaves for holidays and they have pending CLs that I need which are almost perfect but need some small final touches)

- cherry pick to a release branch is a one touch thing from the web ui, no need to use any drover cmdline thingy.

- CL description are in sync with my local commit messages.

- The "conflicts with CL" ui in gerrit  (although I have to admit I never understood its heuristics)

- Being able to link CL together in a topic, which makes easier for reviewers to follow chains of CLs, without having to remember to put "see crre.com/nextclnumber comments"

>1) Will we be able to use an idiomatic gerrit command-line workflow, e.g.:

>$ git remote add gerrit https://chromium-review.googlesource.com/chromium/src

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

I really +1 this. The idea of being able to have 1 branch = 1 feature and 1 CL = 1 is IMHO a dream. (I understand that's not a priority and the goal is to be on par with the current workflow, so I guess won't be supported by git cl upload in the first place)

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CADDpn_7XbURB-cwqOWOMWZ-9BRfdiijW_7CLunYPhUK8YiQ95A%40mail.gmail.com.

[Dirk Pranke]

Answers based on my understanding (which might be wrong, hopefully infra folks will correct me as needed), and one comment/question/push back ...

On Wed, Jul 13, 2016 at 11:18 AM, Primiano Tucci <prim...@chromium.org> wrote:

Generally very excited to see this coming (below why)

Bunch of questions:

1. Any idea about what the config for the chromium/src project is going to be? Is it going to be "rebase if necessary?" (which gives linear history and supports CL dependencies).

 I believe so.

2. Will the case of dependent CLs still be supported (so you can git cl try something which is N branches from master and the trybots will do the right thing as they do recently)?

Yes.

 

3. What happens to merge workflow? Will it still work? This can be tricky. Today you can update upstream changes by merging (as opposite to rebasing) and everything works (I believe) because rietveld's upload.py just sends the diff against origin/master (or whatever tracking branch). Have you tested what happens in this new gerrit world? Does the recently introduced auto-squash do the right thing? 

I am not sure how many people are currently using a merge workflow. One thing I fear is that some people might be merging without realizing because git pull does a merge by default.

I believe this should still work.

 

4. Are we still keeping gnumbd (the thing that adds the Cr-Commit-Position yada yada) (I hope so). Does it mean that the reviews will actually happen on a different branch (e.g. refs/for/pending/master or similar)?

Yes.

 

Now the things that make me very excited are the following, can you confirm if they will actually work or we won't support them for some reason?

- I am guaranteed that I can always check out somebody else's CL and be exactly in their state, I don't have to guess which is the right revision to apply a patch.

No idea, but I'd guess this would work?

 

- Other people can contribute to my patchsets. this might be tricky from a security/auditing viewpoint but is something I'd like to a certain extent (use case: some colleagues leaves for holidays and they have pending CLs that I need which are almost perfect but need some small final touches)

No idea.

 

- cherry pick to a release branch is a one touch thing from the web ui, no need to use any drover cmdline thingy.

No idea.

 

- CL description are in sync with my local commit messages.

I don't think this directly works, but this might depend on something further down in the thread ...

 

- The "conflicts with CL" ui in gerrit  (although I have to admit I never understood its heuristics)

I don't know what this is.

 

- Being able to link CL together in a topic, which makes easier for reviewers to follow chains of CLs, without having to remember to put "see crre.com/nextclnumber comments"

Or this.

 

>1) Will we be able to use an idiomatic gerrit command-line workflow, e.g.:

>$ git remote add gerrit https://chromium-review.googlesource.com/chromium/src

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

I really +1 this. The idea of being able to have 1 branch = 1 feature and 1 CL = 1 is IMHO a dream. (I understand that's not a priority and the goal is to be on par with the current workflow, so I guess won't be supported by git cl upload in the first place)

If I understand you correctly here, you're referring to the fact that Gerrit really wants 1 CL to be one commit on one branch, not multiple commits on a branch, right? It's quite debatable as to whether that's ideal or not ;).

-- Dirk

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2ByH71eCQ5QJ8LaZWSG1Fzwd1LfHZBmAaBjTk4scQ5NLtYw1zA%40mail.gmail.com.

[Matt Giuca]

Thanks for being open about the process and soliciting feedback.

I'd like to throw in my +1 for the merge workflow. It is important to me that I don't lose local history and rebase doesn't always work (auto-squashing is not a good solution for a failed rebase).

I hope that I will be able to continue with the current workflow of "1 local branch = 1 CL" with the tools not caring what I have in my local branches. (My local commits are mine and I don't want them pushed to Gerrit as individual patch sets, separate CLs, whatever.) The current behaviour of "git cl" which just compares my current branch to the upstream branch (in figuring out what the patch set should contain) is ideal. [What I'm reading above sounds promising but I'd like to make sure.]

Matt

[Dirk Pranke]

Actually, I don't think I'm understanding you (Primiano) correctly. I understand the "idiomatic gerrit" workflow to be "one branch has one commit which ends up being one CL". In chromium-land, we get around this by squashing branches as part of pushing them. 

I don't know which thing you (Primiano) think is "a dream", since I think you dropped a key word in that sentence. I believe the tooling in git-cl actually supports both models, though.

-- Dirk

[Matt Giuca]

I believe (through only very minimal experience) there is a Gerrit workflow where you have 1 local branch = many CLs and 1 commit = 1 CL. So if you are working on a chain of CLs that need to land individually, in Chromium land we would have a chain of dependent Git branches. In Gerrit land, you would have a single Git branch for the whole project and carefully manage each of the commits in those branch (via rebasing) so that each commit can land as a CL in master.

Those two workflows are largely incompatible (though a tool could in theory support both by asking the user). I would hope we maintain the 1 branch = 1 CL workflow by default, and have the new "git cl" tool deal with this by creating a fake git repo in a temp directory with all the commits squashed to push to Gerrit (and not actually squash my local commits).

The reason I prefer this workflow (1 = local branch = many local commits = one CL) is because CLs in Chromium can take weeks of work and end up being very large. I often maintain (properly pruned and ordered via rebase) my own list of local commits of small logical steps just to keep myself sane, even if these logical steps aren't individually landable as CLs. Being forced to squash my local commits forces me almost back into an SVN-like workflow where it becomes more stressful and chaotic the longer you work on a CL. (Incidentally, this is why I detest the git rebase-update flow of squashing your local commits at the first sign of an imperial cruiser, and am working on my own replacement for rebase-update that behaves more sanely; PM if interested.)

[Elliott Sprehn]

Yeah I hope we can keep the 1 branch = 1 CL workflow, I actually much prefer the commit message to be disjoint of my local commits which tend to be more my thought process and local experimentation.

I also want to be able to edit my commit messages (and others) directly from the Code review UI and from my phone (which works great today) as well.

[Primiano Tucci]

> I don't know which thing you (Primiano) think is "a dream", since I think you dropped a key word in that sentence. I believe the tooling in git-cl actually supports both models, though.

Oh you are right, I missed a key word :).

What I meant to say is that I like the 1 CL == 1 Commit model, while in Rietveld today 1 CL == 1 branch.

Now before going on, I've the feeling that I just created general panic here, sorry for that. My understanding is that the current 1 branch = 1 cl model is going to be preserved, and that's the all point of the recent PSAs about git cl and autosquash (I imagine what git cl will do is produce a shadow commit which is the result of squashing everything in 1 commit and pushing that to refs/for/bla, and probably still keeping the change id in the branch config).

Now, if you are curious about why it makes a difference to me, in details:

Both Gerrit and Rietveld require that the client keeps some local metadata to reference the CL (the issue ID or however you want to spell it).

In the case of Rietveld this metadata is stored in .git/config under the per-branch section. Each branch remembers what is its rietveld issue number (+ a bunch of other unrelated things).

In Gerrit, instead, the issue ID is kept in the commit message (the Change ID: line). Concretely, when you push a stream of commits to the magic refs/for/branchname, Gerrit checks for the Change ID: line (in each commit). If not present it will create a new CL for that commit. If present it will append a patchset to the existing CL.

Now, how does this make a practical difference for a developer?

Say that I am working on a non trivial feature F. For the sake of speeding up reviews I typically split that feature in multiple CLs (say F1, F2 F3).

Today, in times of Rietveld, the way I am use to handle that is having three branches tracking each other (F1 which tracks origin/master, F2 tracking F1, F3 tracking F2). When I receive feedback from reviewers for F1, I amend F1, then rebase F2 on top of the new F1 and F3 on top of the new F2. I never like too much this model but I got used to live with that. *

What I really like, instead, is a model where my feature is a branch (just one), and each CL is a commit. I find way easier to deal with this model of doing things. This is not possible with Rietveld today (% writing some esoteric scripts which turn commits into shadow branches) and should be possible with Gerrit (as long as I don't drop the Change ID line in the commit).

Having said that, I don't want to create general panic. My understanding is that the Change ID is going to be transparently handled by git cl.

Also the comments here make me realize that I'm probably the only one who cares about this. so... it's all good.

Having said that I'm very happy to have a more general discussion about workflows once we get to Gerrit (I never understood the point of creating one extra commit instead of just amending), but I feel that should be a completely different thread.

*I know that  git rebase-update in depot_tools is supposed to support this use case, but every time I tried it it ended up doing a mixture of things that I don't understand and things that I don't like. 

[Andrii Shyshkalov]

(sorry for late reply, I've been busy spamming your CLs with warnings about master prefix :))

Let me also add security to the list of reasons for switching to Gerrit.

Gerrit's strict ACLs enforcement and signed pushes would allow us to fix a few problems in current Rietveld + CQ / direct push workflows.

On Wednesday, July 13, 2016 at 8:19:56 PM UTC+2, Primiano Tucci wrote:

Generally very excited to see this coming (below why)

Bunch of questions:

1. Any idea about what the config for the chromium/src project is going to be? Is it going to be "rebase if necessary?" (which gives linear history and supports CL dependencies).

It could be "rebase if necessary" if the repository doesn't need Cr-Commit-Position (for example, NaCl or Catapult). 

For Chromium, V8, and WebRTC, which use this footer, rebase would always be be necessary, because predicting the value of these footers at the time of upload is futile.

I'd like to know more of what you mean by "supports CL dependencies though"

2. Will the case of dependent CLs still be supported (so you can git cl try something which is N branches from master and the trybots will do the right thing as they do recently)?

Yep, already works with Gerrit. 

3. What happens to merge workflow? Will it still work? This can be tricky. Today you can update upstream changes by merging (as opposite to rebasing) and everything works (I believe) because rietveld's upload.py just sends the diff against origin/master (or whatever tracking branch). Have you tested what happens in this new gerrit world? Does the recently introduced auto-squash do the right thing?

Yes, if you use git cl upload. And yes, I taught the auto-squash to do the right thing. It generates a hidden "synthetic" commit object which contains tree state of whatever is in your branch and sets the parent commit to whatever it is that your current branch is tracking. Special case is when your current branch C tracks a local branch B (ie you have dependent branches), in which case the synthetic squashed commit of C would have syntentic squashed commit of branch B.

Then this synthetic commit is pushed to Gerrit, thus Gerrit understands CL dependencies, but you don't have to worry about Change-Ids and squashing.

There is a potential downside though: now it's up to Gerrit to decide which file was created and which file was moved. There is no way to influence that decision, as git doesn't store this info, instead it's deduced on the fly by comparing two tree states.

FTR, previously you could manipulate some parameters to ultimately influence which files Rietveld would show as new and which modified. In practice though, I think few people actually did this.

I am not sure how many people are currently using a merge workflow. One thing I fear is that some people might be merging without realizing because git pull does a merge by default.

Given above, I am not worried. Personally though, I have this in my .gitconfig to avoid merging unless I really ask to:

[branch]

    autosetuprebase = always 

4. Are we still keeping gnumbd (the thing that adds the Cr-Commit-Position yada yada) (I hope so). Does it mean that the reviews will actually happen on a different branch (e.g. refs/for/pending/master or similar)?

Great question. So, Cr-Commit-Position will still be with us. But I've started working on getting rid of gnumbd and refs/for/pending, exactly as you wrote above because otherwise reviews would happen on refs/for/pending/heads/master. It'd also simplify a few tools and make branching script less prone to failures.

Now the things that make me very excited are the following, can you confirm if they will actually work or we won't support them for some reason?

- I am guaranteed that I can always check out somebody else's CL and be exactly in their state, I don't have to guess which is the right revision to apply a patch.

Correct.

 

- Other people can contribute to my patchsets. this might be tricky from a security/auditing viewpoint but is something I'd like to a certain extent (use case: some colleagues leaves for holidays and they have pending CLs that I need which are almost perfect but need some small final touches)

It's certainly possible with Gerrit, but we didn't think of this yet. Filed: http://crbug.com/628230 - please star if you care :)

- cherry pick to a release branch is a one touch thing from the web ui, no need to use any drover cmdline thingy.

not considered yet. But I like the idea. Filed: http://crbug.com/628232 (star if you care)

- CL description are in sync with my local commit messages.

Hm, not completely sure what you mean here. I guess you mean this:

  If I amend my commit description locally and re-run git cl upload, will description in CodeReview change?

  Then, it depends.

  NO, if you use git cl upload --squash mode (use git cl desc).

  YES, if you use git cl upload --no-squash or manual git push gerrit HEAD:refs/for/refs/..., but at least git cl upload --no-squash isn't really supported with other git cl goodies.

- The "conflicts with CL" ui in gerrit  (although I have to admit I never understood its heuristics)

Yes in old UI, and I think new UI too

- Being able to link CL together in a topic, which makes easier for reviewers to follow chains of CLs, without having to remember to put "see crre.com/nextclnumber comments"

Yes. I have to add the topic option to git cl upload though: http://crbug.com/628237 

>1) Will we be able to use an idiomatic gerrit command-line workflow, e.g.:

>$ git remote add gerrit https://chromium-review.googlesource.com/chromium/src

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

I really +1 this. The idea of being able to have 1 branch = 1 feature and 1 CL = 1 is IMHO a dream. (I understand that's not a priority and the goal is to be on par with the current workflow, so I guess won't be supported by git cl upload in the first place)

It would be in the sense that we don't intend to ban it, but it's indeed not a priority to provide bells and whistles such as command line tryjob or CQ scheduling. 

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

[Andrii Shyshkalov]

To be clear as owner of git cl for Gerrit:

  1 CL = 1 branch will be the supported workflow, just like before with Rietveld.

It already is, actually :)

On Thursday, July 14, 2016 at 12:08:43 PM UTC+2, Primiano Tucci wrote:

> I don't know which thing you (Primiano) think is "a dream", since I think you dropped a key word in that sentence. I believe the tooling in git-cl actually supports both models, though.

Oh you are right, I missed a key word :).

What I meant to say is that I like the 1 CL == 1 Commit model, while in Rietveld today 1 CL == 1 branch.

Now before going on, I've the feeling that I just created general panic here, sorry for that. My understanding is that the current 1 branch = 1 cl model is going to be preserved, and that's the all point of the recent PSAs about git cl and autosquash (I imagine what git cl will do is produce a shadow commit which is the result of squashing everything in 1 commit and pushing that to refs/for/bla, and probably still keeping the change id in the branch config). 

Correct.

 

Now, if you are curious about why it makes a difference to me, in details:

Both Gerrit and Rietveld require that the client keeps some local metadata to reference the CL (the issue ID or however you want to spell it).

In the case of Rietveld this metadata is stored in .git/config under the per-branch section. Each branch remembers what is its rietveld issue number (+ a bunch of other unrelated things).

In Gerrit, instead, the issue ID is kept in the commit message (the Change ID: line). Concretely, when you push a stream of commits to the magic refs/for/branchname, Gerrit checks for the Change ID: line (in each commit). If not present it will create a new CL for that commit. If present it will append a patchset to the existing CL.

Now, how does this make a practical difference for a developer?

Say that I am working on a non trivial feature F. For the sake of speeding up reviews I typically split that feature in multiple CLs (say F1, F2 F3).

Today, in times of Rietveld, the way I am use to handle that is having three branches tracking each other (F1 which tracks origin/master, F2 tracking F1, F3 tracking F2). When I receive feedback from reviewers for F1, I amend F1, then rebase F2 on top of the new F1 and F3 on top of the new F2. I never like too much this model but I got used to live with that. *

As you rightly note in *: git rebease-update && git checkout F1 && git cl upload --dependenciesd

does the same quickly, particularly if rebase don't require conflict resolution.

But regardless, can you really avoid rebases if you modify the F1 commit in your feature branch?

So far, the only diff I see is branch = faeture vs feature = {branches}, but amount of work seems the same. Am I missing smth?

What I really like, instead, is a model where my feature is a branch (just one), and each CL is a commit. I find way easier to deal with this model of doing things. This is not possible with Rietveld today (% writing some esoteric scripts which turn commits into shadow branches) and should be possible with Gerrit (as long as I don't drop the Change ID line in the commit).

Having said that, I don't want to create general panic. My understanding is that the Change ID is going to be transparently handled by git cl.

Correct. 

Also the comments here make me realize that I'm probably the only one who cares about this. so... it's all good.

Having said that I'm very happy to have a more general discussion about workflows once we get to Gerrit (I never understood the point of creating one extra commit instead of just amending), but I feel that should be a completely different thread.

*I know that  git rebase-update in depot_tools is supposed to support this use case, but every time I tried it it ended up doing a mixture of things that I don't understand and things that I don't like. 

This, I very much agree :) 

I have a fork of it locally and I keep changing it back and force. Once I'm satisfied with predictability and simplicity, I may offer it to others. But this certainly deserves a new thread.

 

-- Dirk

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CADDpn_7XbURB-cwqOWOMWZ-9BRfdiijW_7CLunYPhUK8YiQ95A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.

[Joe Mason]

On Thu, Jul 14, 2016 at 6:07 AM, Primiano Tucci <prim...@chromium.org> wrote:

Also the comments here make me realize that I'm probably the only one who cares about this. so... it's all good.

FWIW I also much prefer 1 local commit = 1 CL, and I'd love to have that supported as an option.

Joe

[Andrew Bonventre]

1 local commit == 1 CL is the native Gerrit behavior, so once the transition is made then we can explore solutions that allow git cl to support it and I can’t imagine it will be too difficult.

That said, we want to minimize the amount of workflow disruption since there are a lot of developers moving over to a brand new tool that is different in some small ways (and some not so small). We will revisit this particular topic after everyone is comfortably on Gerrit.

[Primiano Tucci]

On Thu, Jul 14, 2016 at 3:45 PM, Andrii Shyshkalov <tan...@chromium.org> wrote:

It could be "rebase if necessary" if the repository doesn't need Cr-Commit-Position (for example, NaCl or Catapult). 

What else if not? The other options left are:

- "Merge if necessary" / "always merge".

I imagine we are ruling that out as that will quite radically change the shape of our history. Concretely that will puzzle *lot* of developers and I imagine also break lot of tools which have been designed with a linear history in mind.

- Fast Forward Only

With a rate of 300 commits per day that would makes pushing as easy as reproducing a multithreading data race :)

At this point the only other option left seems "Cherry pick", 

This is IMHO the critical point. 

According to Gerrit's doc about cherry-pick config

"Note that Gerrit ignores dependencies between changes when using this submit type unless change.submitWholeTopic is enabled and depending changes share the same topic. So generally submitters must remember to submit changes in the right order when using this submit type."

 

I'd like to know more of what you mean by "supports CL dependencies though"

So, when you have a pile of inter-dependent CLs, "rebase if necessary" rebases also the dependencies, leaving them in the right order.

This is concretely what I mean:

Look at this cl (which is based on origin/master) and this other one (which is based on the first).

The very nice feature here is that due to the parent linkage gerrit knows that they are related and shows the Relation Chain: (look at the right pane)

If I read the doc correctly I think you lose this by setting​ the project config to "cherry-pick".

And honestly I think that that "Relation chain" can help reviews a lot, as you can move in the space of dependent CLs very intuitively.

> It could be "rebase if necessary" if the repository doesn't need Cr-Commit-Position (for example, NaCl or Catapult). 

This really depends on how you implement it. If you keep the model where committers and CQ push to "pending" and ghumbd rewrites those in the real branch (heads/master) it will just work.

I don't know too much of Gerrit internals for the case of no-pending/no-gnumd. Do you have any sort of server-side pre-push hooks? Because if so you could teach gerrit to add the CrCommitPosition string when doing that.

> For Chromium, V8, and WebRTC, which use this footer, rebase would always be be necessary, because predicting the value of these footers at the time of upload is futile.

This is orthogonal. I am not talking about rebasing local branches, I was talking about the project config.

As you say, once you manipulate the commit, it will be different than the local one. But the same applies to "rebase if necessary". Once you click the button and the server rebases that commit for you you lose the relationship between the local commit object and what gets pushed to master.

 AFAIK the only config that guarantees that your commits match the ones upstream is "merge" (or FF-Only). But, see above.

> It would be in the sense that we don't intend to ban it, but it's indeed not a priority to provide bells and whistles such as command line tryjob or CQ scheduling. 

Yup completely understand. Agree this is a bonus feature. To be honest, given the links above, looks like I am already able to achieve what I want :)

Thanks a lot for working on this and handling it in a transparent manner! It's all exciting.

 

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CADDpn_7XbURB-cwqOWOMWZ-9BRfdiijW_7CLunYPhUK8YiQ95A%40mail.gmail.com.

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/1fcf94fc-d648-4a75-9b3e-6dd0607dccba%40chromium.org.

[Emil A Eklund]

On Wed, Jul 13, 2016 at 10:53 AM, Andrew Bonventre
<andy...@chromium.org> wrote:
> As for contributions, there is already a fairly robust plugin system with a
> JS plugin system design (that will live on top of the current JS API) in the
> works, so for specific workflows, I would suggest writing a plugin. For
> features that would benefit the entirety of Gerrit users, contributions are
> definitely welcome and encouraged.

Thanks, good to know.

I was thinking more along the lines of css changes given that
side-by-side diffs right now are unusable on screens less than 1500px
wide.

[Yoichi Osato]

Does Gerrit has collapse diff view?

This is very useful.

2016年7月15日(金) 7:19 Emil A Eklund <e...@chromium.org>:

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Yoichi Osato]

Does Gerrit has collapse diff view?

This is very useful.

2016年7月15日(金) 7:19 Emil A Eklund <e...@chromium.org>:

On Wed, Jul 13, 2016 at 10:53 AM, Andrew Bonventre

[Primiano Tucci]

Following the instructions in the beginning of this thread (install the PolyGerrit extension) it looks like it does.

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAAEV3pmOyADGWz4hXoyqnonPRkoOikC6w4hYkFGEDNptkehy7g%40mail.gmail.com.

[Guido Urdaneta]

Will the review process remain the same (wait for lgtm comments from OWNERS), or will we use the Gerrit's numerical scoring system (-2 to +2).

If the latter, will there be a guideline on how to use the 5 different values?

Will the CQ/Revert/SelectTrybots buttons be integrated into Gerrit?

[Andrew Bonventre]

On Fri, Jul 15, 2016 at 11:13 AM Guido Urdaneta <gui...@chromium.org> wrote:

Will the review process remain the same (wait for lgtm comments from OWNERS), or will we use the Gerrit's numerical scoring system (-2 to +2).

If the latter, will there be a guideline on how to use the 5 different values?

We’ll be using the scoring system. Labels will aid in establishing guidelines but a +2 from an OWNER will still be required.

 

Will the CQ/Revert/SelectTrybots buttons be integrated into Gerrit?

Yes to all three. Try jobs are already displayed (scheduling is coming) and there will be buttons for CQ dry run/Submit to CQ:

[Julie Parent]

Was the unusableness you saw in regular Gerrit, or using the PolyGerrit extension?

For things like this, that would effect all users and generally be just improvements, contributions are certainly welcomed.  It is an open source project after all :)

[Brett Wilson]

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Brett

[Julie Parent]

[Dirk Pranke]

+1 (somewhat ironically, I suppose). In chromium-land, I don't think we particularly have or have needed the distinction

between +1 and +2, or -1 and -2. It would be nice if we didn't have to expose that for our projects.

-- Dirk

 

Brett

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CABiGVV-VbQpBw3rwPXHYcQxASrALKA4DWMEZ%3D8eQDAsW7ZkwVg%40mail.gmail.com.

[Primiano Tucci]

> Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

The way I've used in other projects is:

0 neutral drive-by comment

+1 I still want somebody else to take a look and that before you land (or I cannot +2, see below).

+2 LGTM

The deal with +1/+2 is that you can have group of people who are allowed to only +1.

If you think about that is pretty much the same today in Rietveld with non-committers LGTM.

0 vs 1 is really the same of what I see today with "drive by"

The negative ones instead make less sense to me. Never seen a practical use of -1. in reality I've seen only -2, which is really the equivalent of our NOT LGTM.

"Verified" instead is optional and can be disabled per-project. That one is really smokey.

[Will Harris]

Does +1 plus +1 == +2?

Will

[Julie Parent]

This might be a good chance to change the language we use for code review to make it a bit more, uh, friendly?  "NOT LGTM" and "-2" both convey a fair amount of aggression.  I propose "Awesomesauce" for +2 :)  Seriously though, we could choose more descriptive terms that are also a bit more playful.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2BFVm4tNk5zo_CzFfxqHH2EZjt2rrOggEJ66G0RxmBrgb44HBQ%40mail.gmail.com.

[Primiano Tucci]

On Fri, Jul 15, 2016, 19:38 Will Harris <w...@chromium.org> wrote:

Does +1 plus +1 == +2?

no. you need somebody with +2 powers to explicitly +2. I imagine in chrome land they would map to committers (no, i don't think there is any builtin support for having per-folder acls of +2 powers) 

[Charles Harrison]

+1 to friendlier language if possible. NOT LGTM (+ the red coloring of your CL) leads to a very unpleasant and possibly unwelcoming experience.

[David Benjamin]

+1 and +2 could be "LGTM" and "LGTM, Approved" perhaps? They're relatively short so perhaps we can show them in place of the numbers without much UI fiddling.

The current long form descriptions for -1 and -2 are "I'd prefer this not land as-is" and "This shall not be merged". -2's existence is awkward. That would mean the reviewer went out of their way to give you a -2 over a -1, which is pretty unpleasant however you slice it. Perhaps we should ditch -2 and make -1 ACL'd as what -2 would have been (I think it's blocks submission, only committers can do it?[*]) but still labeled a variant of "I'd prefer this not land as-is".

David

[*] Can non-committers give not lgtms in Rietveld? I'm actually not sure.

[Miriam Gershenson]

A nice advantage of Gerrit's system is that it reduces the need for "not lgtm". In Rietveld you have to "not lgtm" to undo an accidental lgtm, in Gerrit you can just set it back to 0, avoiding the sad red comments in the CL. I also like David's suggestion about -1/-2.

Are non-committer lgtms different between Rietveld and Gerrit? In Rietveld a non-committer can lgtm a committer's patch and other reviewers aren't necessary if the committer is also an OWNER for the relevant files. (I assume non-committers can also "not lgtm" to undo an lgtm in Rietveld but this seems unnecessary in Gerrit.)

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Bruce]

> no. you need somebody with +2 powers to explicitly +2

Thanks for that clarification. I've used Gerritt various times over the last two years but that was never clear to me.

It seems like it is a design mistake to use +1 and +2 when they can't actually be added - it gives an (IMHO) misleading appearance. I like the idea of lgtm and lgtm, approved or some-such linguistic labeling instead of numerical labeling. Or, just have tool-tips that clarify the meaning to avoid confusion for future new users.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Emil A Eklund]

On Fri, Jul 15, 2016 at 12:03 PM, Charles Harrison <cshar...@chromium.org> wrote:

+1 to friendlier language if possible. NOT LGTM (+ the red coloring of your CL) leads to a very unpleasant and possibly unwelcoming experience.

Google standard is "LGTM" and "Approve". A lot clearer than +1 and +2, +2 seems to imply that it is the equivalent of two +1s.

What's the difference of -1 and -2 and is that distinction really needed?

[Rachel Blum]

At what point would you like to get UI feedback? Now? When it's dogfoodable?

On Tue, Jul 12, 2016 at 2:24 PM, Andrew Bonventre <andy...@chromium.org> wrote:

Summary

The Chromium project is moving away from Rietveld to a revamped version of Gerrit for our main Code Review tool.

When?

The conservative estimate is Q4 of 2016. There are already some Chromium teams dogfooding the new Polymer-based UI (called PolyGerrit) and we are using their feedback to ensure that there is both a smooth transition and developer needs are met. This process will continue as we onboard more people.

Why?

We (Chromium) are the only customers of Rietveld (codereview.chromium.org and its internal counterpart) and maintenance of the tool has been delegated to a single person. The codebase has become a liability over the years and speed has become a major issue.

Gerrit is a fast, secure, and fully-featured code review tool built as a successor to Rietveld. It is already used by some Chrome subteams along with the Android, Go, and Bazel teams (as well as many others both inside and outside Google). It is also a fully staffed project with heavy investment from Google. This also means full-time UX support.

Its front-end is being re-written using Polymer, a standards-based library that allows us to take advantage of Web Components. This UI is the one that Chromium will be transitioning to (though you are welcome to use the old UI if you like).

FAQ

Can I see current progress on PolyGerrit?

Yes. Download this extension and then visit https://chromium-review.googlesource.com. You can then toggle between the two UIs.

How can I start dogfooding it now?

We still have plenty to do on the infrastructure side of things before many teams can move over. If you have a sub-project you think may qualify for early migration, email me and +Andrii Shyshkalov.

How can I follow progress?

All bugs related to the migration have the label Proj-Gerrit-Migration. Additionally, on the Gerrit project, there is a list of Chromium-specific bugs.

What will happen to Rietveld?

Rietveld will be put in read-only mode to preserve change history. Potential migration of changes to Gerrit will be considered after the switch. A unified CL search interface is being strongly considered.

Why not continue to just maintain Rietveld?

The Chromium infra team is stretched far too thin as it is and keeping the status quo of having one or no owner for the Rietveld codebase isn’t tenable given its fragility. The bus factor is just too low. That coupled with the duplication of effort across the Gerrit and Rietveld projects, it doesn’t make strategic sense. We would rather focus on more important work that will bring immediate benefit to Chromium developers.

Why don’t you move the current Rietveld v2 UI on top of Gerrit?

The current Rietveld v2 UI is written in Polymer 0.5, which is no longer supported by the Polymer team. There is a non-trivial amount of work that will go into porting it to 1.0. In addition, there are certain workflows within Gerrit that must be supported, so blindly porting the new Rietveld UI could potentially omit those workflows and cause confusion.

Will this change my current command-line development workflow?

No. git cl will support a Rietveld-style workflow with Gerrit transparently. You can follow this tracking bug to be informed on feature parity with git cl on Rietveld.

I heard I can’t “LGTM with nits.” This is a huge productivity loss if I’m working with people across timezones. Will I have to approve again if the author uploads another patchset?

No. Being able to submit a commit that doesn't exactly match the one that got approved is a per-project configuration option — you can choose in the config whether approvals are “sticky” or only per-patchset.

Are you going to go inside a hole for six months, return with a new tool, and then force everyone to use it?

No. Developer feedback (both from Chrome devs and current Gerrit users) will be crucial for the UI rewrite to be a success. We will be as transparent as possible with progress and will establish a feedback loop very early on. This isn’t about just alleviating maintenance burden, but making a tool that is crucial to everyone’s workflow better.

If you have any questions or need clarification, please don’t hesitate to reach out.

Andy

[Andrew Bonventre]

When it's dogfoodable. UX resources were just recently allocated. In the meantime please answer the survey you see in Rietveld if it pops up :)

[Torne (Richard Coles)]

Having used Gerrit on other projects, there's a couple of use cases for the range of -2..2 that I don't think have been mentioned on the thread so far:

1) Lots of projects allow anybody to +1/-1 a change (like, literally any random person on the internet), just so they can express their overall opinion about a change along with their comments, but only project members/committers/etc can +2/-2.

2) -2 must be explicitly *removed* before a change can be submitted, just like "not lgtm" on rietveld: once someone has done it, they have to come back and change their minds or else the change can't go ahead. -1 can be used instead if you want to allow the author/other reviewers discretion on when to actually go ahead and submit an updated change without having to ask you to rereview.

[Joe Mason]

I think two levels of negative are useful. (I'll give them names instead of -1 and -2 since I'm not sure Gerrit's -1 and -2 actually work this way.)

"I would prefer this not be committed", with no enforcement, allows me to raise an objection to code that's in an area I don't know well (or an objection that I know will be controversial) without actually blocking submission if people who have the power to LGTM that area disagree. This is especially useful for reviewers in a different time zone - it's possible for me to leave a comment with this level of negative, and have others debate it fully and agree my objections are unwarranted and submit anyway without having to wait for me to withdraw the objection.

"Do not submit", with enforcement that overrides any +2's, allows me to block submission if I spot a clear and important bug that other reviewers missed. The enforcement here is important because after an LGTM's been given it's easy to miss a "NO WAIT DON'T DO THAT AAAAUGH" comment. Use of this should be rare, since it doesn't allow other reviewers who disagree to override it.

On the other hand, if there's no enforcement of the "I would prefer this not be committed" level, I suppose it doesn't need a dedicated UI element - just a comment with an explanation would do.

On Fri, Jul 15, 2016 at 3:26 PM, David Benjamin <davi...@chromium.org> wrote:

[Torne (Richard Coles)]

Yes, that's exactly how -2 and -1 work in Gerrit.

[Joe Mason]

Resending from the right address... 

[Ben Henry]

Cool code review tool you got, there.

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAH%3DT95SzPGt5dm%2Bc2Z5ytCA%3De2e2azYcLf%3DMzH3UbdBGqqK-%3DA%40mail.gmail.com.

[John Abd-El-Malek]

I think switching with the +1/+2/-1/-2 would be a regression, since it's unintuitive. We haven't had issues with folks submitting cls with negative comments, and regardless that could be handled with ensuring that there are no outstanding "not lgtm"s.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2Bju%2B_2m_tHFUSyObt1Oy8tYLF5Ns6kSNGFM_rCrhad8XX16Lw%40mail.gmail.com.

[John Abd-El-Malek]

Oh also re the +1 vs +2, that's handled by our tooling which uses OWNERS to enforce that only specific people can lgtm code based on the directory. i.e. we don't have the issue of random passerbys saying lgtm.

[Mike Lawther]

+2, er I mean awesomesauce!

Thanks for the idea Julie - this is an excellent observation about our processes. As an open source project we need to friendly and welcoming to all contributors.

A bad experience on a first CL that is mandated by our processes ('NOT LGTM' is quite aggressive sounding) could mean lose a valuable community member. So let's bake in that friendly playfulness wherever we can.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Brett Wilson]

There are two uses of "not LGTM": #1 undoing a previous LGTM that you did accidentally (I've pushed the wrong button sometimes) or #2 making it clear that you want to have your comment be blocking when other people's reviews might otherwise be sufficient for checkin.

What I think I'd like is to change "reviewers" to "required reviews", keep the current "CC" list, and maintain a table of "LGTM" for each. Presumably the table of review bits is the case internally, but it should be surfaced in the UI as such so you can tell what's left to do. Before checkin, each of the required reviewers must have checked LGTM, in addition to the normal owners coverage check.

"Not LGTM" case #1 becomes unchecking LGTM. "Not LGTM" case #2 becomes adding your name to the list of required reviewers and just not checking LGTM. Potentially adding LGTM in a comment could auto-check the box if we want to avoid changes in workflows. One can add drive-by comments and clearly indicate they're not blocked by a followup LGTM (another cause of confusion) by just not adding oneself to the blocking reviewers list.

This also solves another problem: Once you have enough owners coverage, you can check in regardless of other code reviews. The current state causes two issues:

 - People in higher-level owners files' reviews are sufficient for checkin, regardless of whether they meant their review to cover the whole thing. Often a higher-level owner is required for a trivial build file change, but the actual review is done by somebody else. When patches get more complicated with many owners, it's easy to get confused and check in the patch before everybody is done. This does happen sometimes. With required reviewers, this can't happen by accident.

- People are currently incentivised to add duplicate overlapping reviewers, since whoever does it first will be sufficient. This makes extra work for the other reviewers to open it and see that they don't have to do anything now. This used to happen a lot to me (though anecdotally it seems to be somewhat less of a problem lately since I usually write a comment saying to please not do that). Having a required reviewer list will instead incentivise the minimum spanning set of reviews (which I think is good).

Brett

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Emil A Eklund]

On Mon, Jul 18, 2016 at 3:05 PM, Mike Lawther <mikel...@google.com> wrote:

+2, er I mean awesomesauce!

Thanks for the idea Julie - this is an excellent observation about our processes. As an open source project we need to friendly and welcoming to all contributors.

A bad experience on a first CL that is mandated by our processes ('NOT LGTM' is quite aggressive sounding) could mean lose a valuable community member. So let's bake in that friendly playfulness wherever we can.

Big -1, using silly words is even worse than the number system.

While the silliness might seem playful it is ambiguous and misleading. What does "awesomesauce" even mean? Even having looked up the "definition" I have absolutely no idea how that relates to a code review. Does it mean the feature is cool or that the change itself is deemed acceptable? 

Having clear, well understood and well defined terms communicates the intent and allows outside contribution without the tax of having to learn a new set of terms just because we though "Approved" or "Allowed to land" sounded too boring.

[Mike Lawther]

I saw 'awesomesauce' as a strawman suggestion. We can have the tools say "Approved to land. Thanks!", or "Please don't land this (not lgtm)".

I don't think 'friendliness' implies 'silliness'. It's not a question of 'boring'. We can be friendly and welcoming and professional all at the same time.

[Dirk Pranke]

On Mon, Jul 18, 2016 at 3:32 PM, Brett Wilson <bre...@chromium.org> wrote:

There are two uses of "not LGTM": #1 undoing a previous LGTM that you did accidentally (I've pushed the wrong button sometimes) or #2 making it clear that you want to have your comment be blocking when other people's reviews might otherwise be sufficient for checkin.

What I think I'd like is to change "reviewers" to "required reviews", keep the current "CC" list, and maintain a table of "LGTM" for each. Presumably the table of review bits is the case internally, but it should be surfaced in the UI as such so you can tell what's left to do. Before checkin, each of the required reviewers must have checked LGTM, in addition to the normal owners coverage check.

"Not LGTM" case #1 becomes unchecking LGTM. "Not LGTM" case #2 becomes adding your name to the list of required reviewers and just not checking LGTM. Potentially adding LGTM in a comment could auto-check the box if we want to avoid changes in workflows. One can add drive-by comments and clearly indicate they're not blocked by a followup LGTM (another cause of confusion) by just not adding oneself to the blocking reviewers list.

Although we don't tend to encourage this, it's not uncommon to specify R=alice,bob with the intent that *either* Alice or Bob can review things, but both don't have to.

Would you suggest that in that scenario required reviewers should be empty and Alice and Bob should be cc'ed, or Alice and Bob should both be required even though really only one of them is, and the author/committer should remove the other from the required list, or we should just not worry about this case since we don't encourage people to do this?

My primary concern is that in my experience most people ignore codereview messages most of the time when they're on the cc' list.

-- Dirk

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CABiGVV9Wud1nhzdYmdoNCfmaMsysphQ9wadkMYnnpCgbsY936Q%40mail.gmail.com.

[Thiago Farina]

Since I used Gerrit for the first time, this was the thing I wanted most on Rietveld, the scoring system, although I don't get instantly what -1 could mean. But it seems pretty straight forward:

-2 => NOT LGTM

-1 => ?

0 => Neutral, you reviewed the change and think it is fine.

+1 => LGTM, but someone else must approve

+2 => LGTM, approved.

I much prefer it than the current wording because there is no possible other variation, you don't have to type (Lgtm, lgtm, lg tm, LGTM) and the meaning can be coded in the UI.

Then +2 could be available only to reviewers that are listed in the OWNERS files and so on.

--

Thiago Farina

[Stéphane Marchesin]

On Mon, Jul 18, 2016 at 4:42 PM, Thiago Farina <tfa...@chromium.org> wrote:
>
>
> On Tuesday, July 19, 2016, Emil A Eklund <e...@chromium.org> wrote:
>>
>> On Mon, Jul 18, 2016 at 3:05 PM, Mike Lawther <mikel...@google.com>
>> wrote:
>>>
>>> +2, er I mean awesomesauce!
>>>
>>> Thanks for the idea Julie - this is an excellent observation about our
>>> processes. As an open source project we need to friendly and welcoming to
>>> all contributors.
>>>
>>> A bad experience on a first CL that is mandated by our processes ('NOT
>>> LGTM' is quite aggressive sounding) could mean lose a valuable community
>>> member. So let's bake in that friendly playfulness wherever we can.
>>
>> Big -1, using silly words is even worse than the number system.
>> While the silliness might seem playful it is ambiguous and misleading.
>> What does "awesomesauce" even mean? Even having looked up the "definition" I
>> have absolutely no idea how that relates to a code review. Does it mean the
>> feature is cool or that the change itself is deemed acceptable?
>>
>> Having clear, well understood and well defined terms communicates the
>> intent and allows outside contribution without the tax of having to learn a
>> new set of terms just because we though "Approved" or "Allowed to land"
>> sounded too boring.
>>
> Since I used Gerrit for the first time, this was the thing I wanted most on
> Rietveld, the scoring system, although I don't get instantly what -1 could
> mean. But it seems pretty straight forward:

In gerrit, -1 isn't sticky while -2 is: if you mark something -1 then
reupload a different change on top, the -1 goes away. If it's a -2, it
sticks around.

Stéphane

>
> -2 => NOT LGTM
> -1 => ?
> 0 => Neutral, you reviewed the change and think it is fine.
> +1 => LGTM, but someone else must approve
> +2 => LGTM, approved.
>
> I much prefer it than the current wording because there is no possible other
> variation, you don't have to type (Lgtm, lgtm, lg tm, LGTM) and the meaning
> can be coded in the UI.
>
> Then +2 could be available only to reviewers that are listed in the OWNERS
> files and so on.
>
>
> --
> Thiago Farina
>

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

> ---

> You received this message because you are subscribed to the Google Groups

> "Chromium-dev" group.

> To unsubscribe from this group and stop receiving emails from it, send an

> email to chromium-dev...@chromium.org.

[Emil A Eklund]

On Mon, Jul 18, 2016 at 4:39 PM, Mike Lawther <mikel...@google.com> wrote:
> I saw 'awesomesauce' as a strawman suggestion. We can have the tools say
> "Approved to land. Thanks!", or "Please don't land this (not lgtm)".
>
> I don't think 'friendliness' implies 'silliness'. It's not a question of
> 'boring'. We can be friendly and welcoming and professional all at the same
> time.

Looks like we're on the same page then. Great!

[Peter Kasting]

On Fri, Jul 15, 2016 at 11:09 AM, Brett Wilson <bre...@chromium.org> wrote:

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Given all the comments on this subject, it seems to me that a reasonable first action would be to leave Gerrit's existing functionality unchanged, and simply change the labels from being numerical (which, as others have mentioned, results in confusion since +1 and +1 != +2, and the numbers are unexplained) to verbal.  Then we can try it, and if additional changes still turn out to be necessary, discuss those separately.

Here, I did my best to be clear without being unfriendly:

-2 = Do not land

-1 = I have concerns

0 = Comment

+1 = LGTM, wait for approval

+2 = Clear to land

PK

[Dirk Pranke]

I continue to think that we don't need either -1 or +1 (in Chromium; other projects may certainly work differently), and I believe that having them makes the UI more complicated than it needs to be. I haven't really seen any good counterarguments to this, but maybe I missed something?

-- Dirk

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAAHOzFBLNHSfpQ6hr-7DUnQCfRFVsb%3DdVCauoD48YeRV1j7_Ww%40mail.gmail.com.

[Tomasz Mikolajewski]

Isn't having comments equal to having concerns? I always thought it's something like that:

-2 = This CL is clearly wrong. Not fixable.

-1 = This CL has serious flaws, requires major rework.

0 = I have concerns / comments.

+1 = LGTM, wait for approval

+2 = Clear to land

 

PK

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Peter Kasting]

On Mon, Jul 18, 2016 at 5:21 PM, Dirk Pranke <dpr...@chromium.org> wrote:

I continue to think that we don't need either -1 or +1 (in Chromium; other projects may certainly work differently), and I believe that having them makes the UI more complicated than it needs to be. I haven't really seen any good counterarguments to this, but maybe I missed something?

I don't necessarily disagree, but I'm concerned that proposing to change not only the labeling but the functionality is more prone to being derailed/bikeshedded than proposing just a labeling change, then seeing if indeed things are noticeably too complicated.  The goal is to make more progress faster :)

(And honestly, I'm not sure the 5-state world is so much worse than a three-state one, especially if the tool can enforce that, for example, the "Clear to land" option is only available when the reviewer or author are OWNERs.  That would remove some ambiguity for newer people.)

PK

[John Abd-El-Malek]

On Mon, Jul 18, 2016 at 5:26 PM, 'Peter Kasting' via infra-dev <infr...@chromium.org> wrote:

On Mon, Jul 18, 2016 at 5:21 PM, Dirk Pranke <dpr...@chromium.org> wrote:

I continue to think that we don't need either -1 or +1 (in Chromium; other projects may certainly work differently), and I believe that having them makes the UI more complicated than it needs to be. I haven't really seen any good counterarguments to this, but maybe I missed something?

+1

It seems the simplest thing is to maintain our current vocabulary from Rietveld: i.e. a checkbox for LGTM. Reviewers can specify whatever extra states they want through code comments, which seems to have been sufficient for years.

I don't necessarily disagree, but I'm concerned that proposing to change not only the labeling but the functionality is more prone to being derailed/bikeshedded than proposing just a labeling change, then seeing if indeed things are noticeably too complicated.  The goal is to make more progress faster :)

(And honestly, I'm not sure the 5-state world is so much worse than a three-state one, especially if the tool can enforce that, for example, the "Clear to land" option is only available when the reviewer or author are OWNERs.  That would remove some ambiguity for newer people.)

PK

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAAHOzFAJqgZsQyC3tNfz-vNdioXS949jeWeWk7NZR74%3DtZ2SUQ%40mail.gmail.com.

[Tomasz Mikolajewski]

So, we'd let everyone do +1, but +2 only to OWNERs? This is an interesting idea. It may significantly improve reviewing speed, but can affect quality.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Peter Kasting]

On Mon, Jul 18, 2016 at 5:24 PM, Tomasz Mikolajewski <mto...@chromium.org> wrote:

On 19 July 2016 at 09:18, 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

On Fri, Jul 15, 2016 at 11:09 AM, Brett Wilson <bre...@chromium.org> wrote:

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Given all the comments on this subject, it seems to me that a reasonable first action would be to leave Gerrit's existing functionality unchanged, and simply change the labels from being numerical (which, as others have mentioned, results in confusion since +1 and +1 != +2, and the numbers are unexplained) to verbal.  Then we can try it, and if additional changes still turn out to be necessary, discuss those separately.

Here, I did my best to be clear without being unfriendly:

-2 = Do not land

-1 = I have concerns

0 = Comment

+1 = LGTM, wait for approval

+2 = Clear to land

Isn't having comments equal to having concerns?

No.  Concern implies a necessity to fix.

 

I always thought it's something like that:

-2 = This CL is clearly wrong. Not fixable.

-1 = This CL has serious flaws, requires major rework.

0 = I have concerns / comments.

Both your -2 and -1 say more than they should, IMO.  I have Not LGTMed changes that are fixable, just should not be landed as-is, or should not be landed without some further discussion among OWNERs to make sure they're OK.  And I think encoding the severity of the reviewer's concerns in the text forces people to pick from items when maybe none of them reflect the reviewer intent.

Honestly, I wouldn't mind being in a world where I could use the "-1" state as a non-hostile way of expressing that I've reviewed and the CL needs another pass, instead of this being collapsed into the 0 state.

If we end up deciding we don't want that granularity, then ultimately I would wind up arguing we don't need the distinction between your concept of -1 and -2, and we should move towards what Dirk proposes, where we just have states equivalent to LGTM/nothing/Not LGTM.  I was simply hoping to punt that argument down the road in case (a) it's easier to do this and 9b) we get a chance to use the additional granularity and decide we like it, before throwing it out.

PK

[Dirk Pranke]

I was thinking that keeping the 3-state model would make it easier for us to migrate from Rietveld, but this is to some extent a question of UIs.

I guess I didn't say this in my initial comments, but I've committed to a number of different Gerrit-based repos, and I continually find the 5-state model confusing. It's never clear to me when I should say +1 or +2 (assuming I have the right to say +2, that is). Perhaps others feel differently.

Of course, I find multiple other aspects of the Gerrit UI confusing as well, and so I'm hopefully that they'll all be reworked and addressed before we're ready to switch, in which case maybe it'll be less of an issue.

-- Dirk

PK

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAAHOzFD3iyjpwZHQ3bt0bqT3cCBmXwomWWkaRvrJyUK_Xyi1VA%40mail.gmail.com.

[Tomasz Mikolajewski]

On 19 July 2016 at 09:34, Peter Kasting <pkas...@google.com> wrote:

On Mon, Jul 18, 2016 at 5:24 PM, Tomasz Mikolajewski <mto...@chromium.org> wrote:

On 19 July 2016 at 09:18, 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

On Fri, Jul 15, 2016 at 11:09 AM, Brett Wilson <bre...@chromium.org> wrote:

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Given all the comments on this subject, it seems to me that a reasonable first action would be to leave Gerrit's existing functionality unchanged, and simply change the labels from being numerical (which, as others have mentioned, results in confusion since +1 and +1 != +2, and the numbers are unexplained) to verbal.  Then we can try it, and if additional changes still turn out to be necessary, discuss those separately.

Here, I did my best to be clear without being unfriendly:

-2 = Do not land

-1 = I have concerns

0 = Comment

+1 = LGTM, wait for approval

+2 = Clear to land

Isn't having comments equal to having concerns?

No.  Concern implies a necessity to fix.

From my experience it's very rare to write comments during review, which do not convey any concern.

 

 

I always thought it's something like that:

-2 = This CL is clearly wrong. Not fixable.

-1 = This CL has serious flaws, requires major rework.

0 = I have concerns / comments.

Both your -2 and -1 say more than they should, IMO.  I have Not LGTMed changes that are fixable, just should not be landed as-is, or should not be landed without some further discussion among OWNERs to make sure they're OK.  And I think encoding the severity of the reviewer's concerns in the text forces people to pick from items when maybe none of them reflect the reviewer intent.

Honestly, I wouldn't mind being in a world where I could use the "-1" state as a non-hostile way of expressing that I've reviewed and the CL needs another pass, instead of this being collapsed into the 0 state.

My descriptions for -2 and -1 are based on experience on other projects with gerrit. Not sure if it's what we want, but it feels like a common practice.

Personally, giving "-1" feels like writing "not lgtm" in rietveld. I think it was rare practice for Chromium, and done only when things were pretty bad.

[Tomasz Mikolajewski]

To clarify, I find gerrit 5-state confusing, and since we have so many interpretations of these states may be a proof that this is indeed confusing. I'd be completely happy if we just had three states.

[Ken Rockot]

On Mon, Jul 18, 2016 at 5:41 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Mon, Jul 18, 2016 at 5:34 PM, 'Peter Kasting' via infra-dev <infr...@chromium.org> wrote:

On Mon, Jul 18, 2016 at 5:24 PM, Tomasz Mikolajewski <mto...@chromium.org> wrote:

On 19 July 2016 at 09:18, 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

On Fri, Jul 15, 2016 at 11:09 AM, Brett Wilson <bre...@chromium.org> wrote:

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Given all the comments on this subject, it seems to me that a reasonable first action would be to leave Gerrit's existing functionality unchanged, and simply change the labels from being numerical (which, as others have mentioned, results in confusion since +1 and +1 != +2, and the numbers are unexplained) to verbal.  Then we can try it, and if additional changes still turn out to be necessary, discuss those separately.

Here, I did my best to be clear without being unfriendly:

-2 = Do not land

-1 = I have concerns

0 = Comment

+1 = LGTM, wait for approval

+2 = Clear to land

Isn't having comments equal to having concerns?

No.  Concern implies a necessity to fix.

 

I always thought it's something like that:

-2 = This CL is clearly wrong. Not fixable.

-1 = This CL has serious flaws, requires major rework.

0 = I have concerns / comments.

Both your -2 and -1 say more than they should, IMO.  I have Not LGTMed changes that are fixable, just should not be landed as-is, or should not be landed without some further discussion among OWNERs to make sure they're OK.  And I think encoding the severity of the reviewer's concerns in the text forces people to pick from items when maybe none of them reflect the reviewer intent.

Honestly, I wouldn't mind being in a world where I could use the "-1" state as a non-hostile way of expressing that I've reviewed and the CL needs another pass, instead of this being collapsed into the 0 state.

If we end up deciding we don't want that granularity, then ultimately I would wind up arguing we don't need the distinction between your concept of -1 and -2, and we should move towards what Dirk proposes, where we just have states equivalent to LGTM/nothing/Not LGTM.  I was simply hoping to punt that argument down the road in case (a) it's easier to do this and 9b) we get a chance to use the additional granularity and decide we like it, before throwing it out.

I was thinking that keeping the 3-state model would make it easier for us to migrate from Rietveld, but this is to some extent a question of UIs.

I guess I didn't say this in my initial comments, but I've committed to a number of different Gerrit-based repos, and I continually find the 5-state model confusing. It's never clear to me when I should say +1 or +2 (assuming I have the right to say +2, that is). Perhaps others feel differently.

I strongly agree with this sentiment. I find 5-state review unintuitive. It's definitely unconventional. I don't see how the extra granularity adds nearly enough value to justify straying from either intuition or convention. Can we please not change this?

[Peter Kasting]

On Mon, Jul 18, 2016 at 5:41 PM, Dirk Pranke <dpr...@chromium.org> wrote:

I guess I didn't say this in my initial comments, but I've committed to a number of different Gerrit-based repos, and I continually find the 5-state model confusing. It's never clear to me when I should say +1 or +2 (assuming I have the right to say +2, that is). Perhaps others feel differently.

To what degree is this because a 5-state model is inherently confusing, versus being due to the amazingly poor (IMO) choice to label these with numbers?

I completely agree with you that the current Gerrit numeric system is bad.  I'm not convinced that's because it has five states, instead of just having bad UI.

PK 

[Peter Kasting]

On Mon, Jul 18, 2016 at 5:46 PM, Tomasz Mikolajewski <mto...@chromium.org> wrote:

On 19 July 2016 at 09:34, Peter Kasting <pkas...@google.com> wrote:

On Mon, Jul 18, 2016 at 5:24 PM, Tomasz Mikolajewski <mto...@chromium.org> wrote:

On 19 July 2016 at 09:18, 'Peter Kasting' via Chromium-dev <chromi...@chromium.org> wrote:

On Fri, Jul 15, 2016 at 11:09 AM, Brett Wilson <bre...@chromium.org> wrote:

Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?

Given all the comments on this subject, it seems to me that a reasonable first action would be to leave Gerrit's existing functionality unchanged, and simply change the labels from being numerical (which, as others have mentioned, results in confusion since +1 and +1 != +2, and the numbers are unexplained) to verbal.  Then we can try it, and if additional changes still turn out to be necessary, discuss those separately.

Here, I did my best to be clear without being unfriendly:

-2 = Do not land

-1 = I have concerns

0 = Comment

+1 = LGTM, wait for approval

+2 = Clear to land

Isn't having comments equal to having concerns?

No.  Concern implies a necessity to fix.

From my experience it's very rare to write comments during review, which do not convey any concern.

The vast majority of my review comments are things which I think could improve the code, but which the author is free to reject at will.

I also sometimes leave drive-by comments on other reviews, often in reply to questions there.  I don't know whether this happens more often to me because I end up CCed on or aware of so many reviews as a c/b/ui OWNER.

I always thought it's something like that:

-2 = This CL is clearly wrong. Not fixable.

-1 = This CL has serious flaws, requires major rework.

0 = I have concerns / comments.

Both your -2 and -1 say more than they should, IMO.  I have Not LGTMed changes that are fixable, just should not be landed as-is, or should not be landed without some further discussion among OWNERs to make sure they're OK.  And I think encoding the severity of the reviewer's concerns in the text forces people to pick from items when maybe none of them reflect the reviewer intent.

Honestly, I wouldn't mind being in a world where I could use the "-1" state as a non-hostile way of expressing that I've reviewed and the CL needs another pass, instead of this being collapsed into the 0 state.

My descriptions for -2 and -1 are based on experience on other projects with gerrit. Not sure if it's what we want, but it feels like a common practice.

Right, I'm intentionally avoiding duplicating the practices of other projects.  I don't think those practices make very good use of the negative states, and even if they did, I don't think we need to be so specific about codifying them in the option text.  Make things broad enough that reviewers can find an appropriate button and say more if they want.

Personally, giving "-1" feels like writing "not lgtm" in rietveld. I think it was rare practice for Chromium, and done only when things were pretty bad.

Not LGTM is indeed pretty rare, but that's partly because there's no real way to say anything between that and a normal review.  Whereas, imagine if the tools would distinguish between, say, 0 and -1 in determining who/what to email when a person landed a CL (e.g. mail the -1 folks, don't mail the 0 folks).  Or if having outstanding -1s allowed the author to commit, but the tool prompted them with something like "$REVIEWER had concerns (link); are those resolved?" before landing.

I guess what I'm trying to say is that the simple Rietveld system requires us to do a lot of things manually, and make sure people follow the rules, when perhaps we could get the tooling to handle some of this stuff more intelligently for us.

PK 

[Andrew Bonventre]

For simplicity’s sake, the Code-Review label will have the values [-1, 0, +1]. These map to the current Rietveld states and introducing more will no doubt cause confusion during the transition.

Much like the debate about 1 Commit = 1 CL vs 1 Branch = 1 CL, we can revisit this once users are onboarded and get more familiar with Gerrit.

[Tomasz Mikolajewski]

I don't want to do derail the discussion, but another issue with gerrit which is bothering me even more is lack of cc'ing. Currently reviewers and cc'd people are put on one list, which is quite confusing. Can we add this feature?

[Peter Kasting]

On Mon, Jul 18, 2016 at 6:08 PM, Andrew Bonventre <andy...@chromium.org> wrote:

For simplicity’s sake, the Code-Review label will have the values [-1, 0, +1]. These map to the current Rietveld states and introducing more will no doubt cause confusion during the transition.

Fair enough.  Can we at least rename these to either match the Rietveld names or have slightly friendlier versions?  It's still very confusing to use numbers here, since they don't add up or cancel each other out.

PK 

[Andrew Bonventre]

CC is a blocking feature for the transition and is currently being worked on.

[Dirk Pranke]

On Mon, Jul 18, 2016 at 5:53 PM, Peter Kasting <pkas...@google.com> wrote:

On Mon, Jul 18, 2016 at 5:41 PM, Dirk Pranke <dpr...@chromium.org> wrote:

I guess I didn't say this in my initial comments, but I've committed to a number of different Gerrit-based repos, and I continually find the 5-state model confusing. It's never clear to me when I should say +1 or +2 (assuming I have the right to say +2, that is). Perhaps others feel differently.

To what degree is this because a 5-state model is inherently confusing, versus being due to the amazingly poor (IMO) choice to label these with numbers?

I thought I had written this up before, but apparently I didn't send it out. 

To me, the distinction between "LGTM" and "Clear to land" is something the tool should handle automatically, rather than make the user have to be explicit about.

There have been cases where I've said "LGTM but I want X to review as well", but in that case I'd be just as inclined to make that a comment and not say +1, if we had separate locations in the UI. Similarly, I would leave a comment but not say "I have concerns". 

To me, CLs tend to be black, white, or grey. I can see how one might think that "dark grey", "medium grey", and "light grey" might be useful to convey nuance, but to me nuance is better conveyed in the comments, which you should be adding anyway.

At any rate, andybons@ has spoken, so I'll let this die off now ;)

-- Dirk

[Andrew Bonventre]

Yes. Everything is configurable in Gerrit.

[Mike Lawther]

On 19 July 2016 at 10:59, 'Peter Kasting' via blink-dev <blin...@chromium.org> wrote:

The vast majority of my review comments are things which I think could improve the code, but which the author is free to reject at will.

I think this is pretty common, and I would dearly love for 'lgtm with nits' to be a standard button/option/thing for the times when a review solely consists of 'reject at will' comments. We talked about clarity before on this thread, and this very clearly conveys the reviewers intent ('I trust you to evaluate these, make the necessary changes and you're good to go'). I'm sure I've had reviews from Peter like this, and it's a good feeling. For such a distributed project as ours, this can also literally save a day over the case where the reviewer doesn't give an lgtm at the same time as the comments.

    mike

[Dirk Pranke]

Do you think that "lgtm w/ nits" in the comments doesn't convey this today? Wouldn't checking an "lgtm" box be sufficient in the future? It the box is checked, it seems pretty clear that the reviewer is either assuming you'll address things as appropriate and don't need a follow-on review.

-- Dirk

    mike

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAC7btF76q3FHCr3HfQZCj%3Dn9G1MtXYnmLLLcYtuDfrn9yNMf%3DA%40mail.gmail.com.

[Elliott Sprehn]

On Mon, Jul 18, 2016 at 8:32 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Mon, Jul 18, 2016 at 7:59 PM, Mike Lawther <mikel...@chromium.org> wrote:

On 19 July 2016 at 10:59, 'Peter Kasting' via blink-dev <blin...@chromium.org> wrote:

The vast majority of my review comments are things which I think could improve the code, but which the author is free to reject at will.

I think this is pretty common, and I would dearly love for 'lgtm with nits' to be a standard button/option/thing for the times when a review solely consists of 'reject at will' comments. We talked about clarity before on this thread, and this very clearly conveys the reviewers intent ('I trust you to evaluate these, make the necessary changes and you're good to go'). I'm sure I've had reviews from Peter like this, and it's a good feeling. For such a distributed project as ours, this can also literally save a day over the case where the reviewer doesn't give an lgtm at the same time as the comments.

Do you think that "lgtm w/ nits" in the comments doesn't convey this today? Wouldn't checking an "lgtm" box be sufficient in the future? It the box is checked, it seems pretty clear that the reviewer is either assuming you'll address things as appropriate and don't need a follow-on review.

Yeah I'd really like to avoid adding more buttons and choices. I'm not a big fan of the 5 state system in Gerrit, and I tried quite hard to cut down on UI complexity in the Polymer Reitveld UI which lots of people were very happy with. Both Rietveld and even more so Gerrit have an explosion of choice in the UI. I hope when we switch to PolyGerrit we can keep with the spirit of the non-deprecated Rietveld UI and have a streamlined experience. :)

- E

[Andrew Bonventre]

While a worthwhile goal, PolyGerrit is not a solution only for chromium. It is a complete replacement for the Gerrit UI. As such it’s important to set expectations that we have to support workflows that lie outside of chrome-land. This means having functionality that may seem superfluous to chrome devs, but is necessary for some subset of the 500k+ users of Gerrit worldwide.

[Primiano Tucci]

IMHO, whether it's called +1, "yay you're cool" or LGTM is the kind of detail that the brain is really good at adapting to in ~3 codereviews and stop even realizing afterwards.

As folks have pointed out in various ways, three states should be sufficient at this point (I'm just commenting, LGTM, NOT LGTM... in whatever spelling).

Five states seem to generate quite some confusion and the reality is that they don't really address all the use cases we need.

+1 vs +2 were originally introduced in gerrit as a way to express "I'm okay but I'd like also this person to take a look". But you still have to write the "I want person X to take a look" part manually and Gerrit would not enforce it. If we really have to make some bigger change to Gerrit, Brett's proposal seems the right way to address it. Until then -2 0 +2 should keep us on par. 

Note that technically we'd need -2 to stay on par, as NOT-LGTM today is sticky-per-person in rietveld, as in you need the same person to undo that with a LGTM. Which is the same semantic of Gerrit's -2.

As regards non-committers, I don't think is a big deal not having the ability for them to express a numeric score. They can still write "LGTM" or "NOT LGTM" and the reality is that will have the same final effect of -1/+1 (that is, nothing real).

[Torne (Richard Coles)]

The way gerrit actually works "underneath" is that you can set the range for a given thing to be between any two integers, and then the requirement for the commit is "it must have at least one of the largest integer, and must not have any of the most negative integer". So it's not that we would have +2 and -2: if we decide we just want three states, they would be -1/0/+1 and would function exactly the same as +2 and -2 do in the 5-state system :)

[Anthony Berent]

One other thing to remember; whether we like it or not most gerrit setups in the world use the 5 state system. Switching between a 3 state system on our repository, and a 5 state system on every other repository could cause confusion, particularly for new Chromium committers, or for people moving from Chromium to other projects.

[Joe Mason]

That's an argument for not using the labels "-1, 0, +1", since -1 and +1 won't mean the same thing as in other gerrit systems.

[Joe Mason]

That's an argument for not using the labels "-1, 0, +1", since -1 and +1 won't mean the same thing as in other gerrit systems. I think switching from a 5-state mindset to a 3-state mindset is easy as long as there's no overlap in state labels.

[Rahul Chaturvedi]

+1 to consistency w/ a caveat.

A five state system is, I think almost everyone would agree, quite an overkill. Instead of us staying with an undesirable system, maybe we should switch to a 3 state system and then encourage Chrome OS and Android to do the same?

I've seen a lot of arguments against the 5 state system in the last 75 messages but none for. Do we have arguments for keeping that 5 state system, anywhere?

/rkc

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Andrew Bonventre]

I don’t believe it’s appropriate to propose changes in other teams’ workflows when we haven’t even moved over to Gerrit yet.

This thread has gone long enough with the discussion of Code-Review label ranges. I’d like to place a moratorium on this in favor of questions relating to the transition that have not already been answered.

[Mike Lawther]

On 19 July 2016 at 13:32, Dirk Pranke <dpr...@chromium.org> wrote:

On Mon, Jul 18, 2016 at 7:59 PM, Mike Lawther <mikel...@chromium.org> wrote:

On 19 July 2016 at 10:59, 'Peter Kasting' via blink-dev <blin...@chromium.org> wrote:

The vast majority of my review comments are things which I think could improve the code, but which the author is free to reject at will.

I think this is pretty common, and I would dearly love for 'lgtm with nits' to be a standard button/option/thing for the times when a review solely consists of 'reject at will' comments. We talked about clarity before on this thread, and this very clearly conveys the reviewers intent ('I trust you to evaluate these, make the necessary changes and you're good to go'). I'm sure I've had reviews from Peter like this, and it's a good feeling. For such a distributed project as ours, this can also literally save a day over the case where the reviewer doesn't give an lgtm at the same time as the comments.

Do you think that "lgtm w/ nits" in the comments doesn't convey this today? Wouldn't checking an "lgtm" box be sufficient in the future? It the box is checked, it seems pretty clear that the reviewer is either assuming you'll address things as appropriate and don't need a follow-on review.

It does *if the reviewer checks the box*.

This has been causing frustrations for years. Reviewers who are used to having fast turnarounds from people in their timezone don't always say lgtm until all comments are addressed. That's just the workflow they have, and habits die hard. They're not trying to be annoying, but it happens.

A button/checkbox/whatever for this makes it clear that it's a possibility, and that our project accepts/encourages this practice to reviewers (new and old) who may not have considered the need for it.

[Dirk Pranke]

I'm well aware of the frustration, and have experienced it myself plenty of times. Maybe having a dedicated button for this would help, dunno (I'm not totally convinced but would be willing to try it).

-- Dirk

[PhistucK]

I just tried Gerrit (Infra is using it for Monorail code reviews) and I noticed two severe issues that make the experience much worse.

- It is really, really slow. Saving a comment takes a few seconds(!), for example. In the non Polymer based Gerrit, going from a specific file to the issue also takes a few seconds, which is really bad because...

- In order to publish my comments, I must go to the issue (instead of having a "Publish+Mail Comments" option in the specific file view).

(The non Polymer based version also looks extremely cluttered, but the Polymer based version make it better)

☆PhistucK

On Wed, Jul 13, 2016 at 12:24 AM, Andrew Bonventre <andy...@chromium.org> wrote:

Summary

The Chromium project is moving away from Rietveld to a revamped version of Gerrit for our main Code Review tool.

When?

The conservative estimate is Q4 of 2016. There are already some Chromium teams dogfooding the new Polymer-based UI (called PolyGerrit) and we are using their feedback to ensure that there is both a smooth transition and developer needs are met. This process will continue as we onboard more people.

Why?

We (Chromium) are the only customers of Rietveld (codereview.chromium.org and its internal counterpart) and maintenance of the tool has been delegated to a single person. The codebase has become a liability over the years and speed has become a major issue.

Gerrit is a fast, secure, and fully-featured code review tool built as a successor to Rietveld. It is already used by some Chrome subteams along with the Android, Go, and Bazel teams (as well as many others both inside and outside Google). It is also a fully staffed project with heavy investment from Google. This also means full-time UX support.

Its front-end is being re-written using Polymer, a standards-based library that allows us to take advantage of Web Components. This UI is the one that Chromium will be transitioning to (though you are welcome to use the old UI if you like).

FAQ

Can I see current progress on PolyGerrit?

Yes. Download this extension and then visit https://chromium-review.googlesource.com. You can then toggle between the two UIs.

How can I start dogfooding it now?

We still have plenty to do on the infrastructure side of things before many teams can move over. If you have a sub-project you think may qualify for early migration, email me and +Andrii Shyshkalov.

How can I follow progress?

All bugs related to the migration have the label Proj-Gerrit-Migration. Additionally, on the Gerrit project, there is a list of Chromium-specific bugs.

What will happen to Rietveld?

Rietveld will be put in read-only mode to preserve change history. Potential migration of changes to Gerrit will be considered after the switch. A unified CL search interface is being strongly considered.

Why not continue to just maintain Rietveld?

The Chromium infra team is stretched far too thin as it is and keeping the status quo of having one or no owner for the Rietveld codebase isn’t tenable given its fragility. The bus factor is just too low. That coupled with the duplication of effort across the Gerrit and Rietveld projects, it doesn’t make strategic sense. We would rather focus on more important work that will bring immediate benefit to Chromium developers.

Why don’t you move the current Rietveld v2 UI on top of Gerrit?

The current Rietveld v2 UI is written in Polymer 0.5, which is no longer supported by the Polymer team. There is a non-trivial amount of work that will go into porting it to 1.0. In addition, there are certain workflows within Gerrit that must be supported, so blindly porting the new Rietveld UI could potentially omit those workflows and cause confusion.

Will this change my current command-line development workflow?

No. git cl will support a Rietveld-style workflow with Gerrit transparently. You can follow this tracking bug to be informed on feature parity with git cl on Rietveld.

I heard I can’t “LGTM with nits.” This is a huge productivity loss if I’m working with people across timezones. Will I have to approve again if the author uploads another patchset?

No. Being able to submit a commit that doesn't exactly match the one that got approved is a per-project configuration option — you can choose in the config whether approvals are “sticky” or only per-patchset.

Are you going to go inside a hole for six months, return with a new tool, and then force everyone to use it?

No. Developer feedback (both from Chrome devs and current Gerrit users) will be crucial for the UI rewrite to be a success. We will be as transparent as possible with progress and will establish a feedback loop very early on. This isn’t about just alleviating maintenance burden, but making a tool that is crucial to everyone’s workflow better.

If you have any questions or need clarification, please don’t hesitate to reach out.

Andy

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Andrew Bonventre]

Please file bugs with these specific issues at https://bugs.chromium.org/p/gerrit/issues/entry?template=PolyGerrit%20Issue&labels=Hotlist-Chromium

On Fri, Sep 9, 2016 at 4:54 PM PhistucK <phis...@gmail.com> wrote:

I just tried Gerrit (Infra is using it for Monorail code reviews) and I noticed two severe issues that make the experience much worse.

- It is really, really slow. Saving a comment takes a few seconds(!), for example. In the non Polymer based Gerrit, going from a specific file to the issue also takes a few seconds, which is really bad because...

- In order to publish my comments, I must go to the issue (instead of having a "Publish+Mail Comments" option in the specific file view).

FYI, while there is no link, you can press ‘a’ and it will do what you want.

[PhistucK]

Some day, since -
"This bug tracker is currently unavailable due to database issues."

☆PhistucK

[Andrew Bonventre]

It appears to be back up now.

[PhistucK]

Is there any intention to (re-?)implement support for feeds like Rietveld has (granted, it is not the best support and I had to create a feed for the feed due to the too-slowly-updating Feedly, but it is still something)?

☆PhistucK

[Andrew Bonventre]

There is no bug on file so, at the moment, there are no plans for RSS support.

Filing a bug on crbug.com/gerrit under the PolyGerrit component with your intended use case for the feed is your best bet, however we currently don't consider it to be launch-blocking at the moment.

[PhistucK]

Here - https://crbug.com/gerrit/5445.

☆PhistucK

[Andrew Bonventre]

Thank you