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.
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
--
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.
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/masterI 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)
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2ByH71eCQ5QJ8LaZWSG1Fzwd1LfHZBmAaBjTk4scQ5NLtYw1zA%40mail.gmail.com.
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.
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/masterI 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)
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.
> 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.
-- 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.
Also the comments here make me realize that I'm probably the only one who cares about this. so... it's all good.
It could be "rebase if necessary" if the repository doesn't need Cr-Commit-Position (for example, NaCl or Catapult).
I'd like to know more of what you mean by "supports CL dependencies though"
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 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.
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.
--
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.
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?
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.
--
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.
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.
+1 to friendlier language if possible. NOT LGTM (+ the red coloring of your CL) leads to a very unpleasant and possibly unwelcoming experience.
SummaryThe 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).FAQCan 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
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:
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:
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.
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.
+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.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
+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.
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.
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.
Gerrit's scoring system has never made any sense to me. Is there something that explains how it works?
--
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.
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.
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?
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
--
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.
--
--
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.
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 concerns0 = Comment+1 = LGTM, wait for approval+2 = Clear to landIsn'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.
--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.
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 concerns0 = Comment+1 = LGTM, wait for approval+2 = Clear to landIsn'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.
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 concerns0 = Comment+1 = LGTM, wait for approval+2 = Clear to landIsn'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 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.
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 concerns0 = Comment+1 = LGTM, wait for approval+2 = Clear to landIsn'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.
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.
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?
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.
--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.
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.
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.
--
--
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.
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.
--SummaryThe 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).FAQCan 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.
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).
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.