Mostly agree with Thiago answer, I just put some addition. BR, Uze Choi -----Original Message----- From: [email protected] [mailto:iotivity-dev- bounces at lists.iotivity.org] On Behalf Of Thiago Macieira Sent: Thursday, January 12, 2017 4:30 AM To: iotivity-dev at lists.iotivity.org Subject: Re: [dev] visiting "how to use Gerrit" topic
On quarta-feira, 11 de janeiro de 2017 10:50:33 PST Mats Wichmann wrote: > After digging some, and watching a bunch of changes, I'm still trying > to figure out what are the preferred modes of working. If the answers > to these questions provide new information, I will update the wiki > page > > https://wiki.iotivity.org/submitting_to_gerrit > > 1. Do developers develop fixes (as distinguished from large new > features) each in a separate local branch before pushing to gerrit? > Some gerrit-using projects explicitly suggest that, our wiki page is silent. Up to the developer and how much they're familiar with Git. I develop everything in the same branch and this mode probably works for Git experts as well as the casual developer who has very few outstanding changes. So one branch is enough for them. If you're not that good with Git and you have a couple of outstanding, unrelated changes (whether bugfixes or features, it doesn't matter), you may want to have different branches. This is for your own organisation. > 2. If you want to fix several not intimately related things in a file, > is it preferred to make that look like one change or several? My > "learn how this works" commit used the second method, but that looks > sub-optimal to me as far as how gerrit works. To see what I'm talking > about: > > https://gerrit.iotivity.org/gerrit/15961 > https://gerrit.iotivity.org/gerrit/15963 > https://gerrit.iotivity.org/gerrit/15965 > > I guess it comes down to what reviewers find easier? (some communities > are very explicit about "make every topic a distinct patch" behavior). I much prefer one atomic change per Gerrit change. That is, make it as small as you can, and yet complete. Definitely avoid breaking the build, as Jenkins will hate you and that will make our lives difficult later if we ever need to bisect a regression. Another hint is that the commit message needs to explain the "why" of everything you've done in your change. So the changes need to be either obvious to the reviewers and to you (now and 2 years from now) or they need to be explained. If you do unrelated changes in the same commit, you'd have to explain more, to the point that the commit message becomes an essay... [Uz]-> Ideally separation is good, but if they depends each other, one fix need to be changed then others need to be updated also. -> In case of simple commit, we can aggregate them I think. > 3. What is the preferred way to rebase when the target has moved on? > A rebase button will appear in gerrit - should it happen there, or > should you rebase on your local machine? Any tips on rebasing? Rebasing is only necessary if there's a conflict. If there isn't one, then you can just let Gerrit do it for you when it cherry-picks. Our Gerrit is configured for cherry-pick mode, unlike many other Gerrits that you may have come across, so it automatically "rebases" for you when you hit the submit button. If Gerrit does tell you "Cannot merge", then rebase your conflicting commit(s). If you're good with Git, you can try to rebase only the commits that needed to be rebased; otherwise, rebase the entire series. Important: try to do *only* the rebasing, without further changes. If you need to do further changes, rebase first, push to Gerrit, then do the changes and push again. [Uz] -> There were some issues before. Build break happened from merge when there is script change is merged after initial commit done. Because maintainer already ware of commit situation, maintainer can try to rebase to prevent conflict. > 4. Figuring out the right set of reviewers for a change seems daunting > (there was another message on that topic recently, I believe). We do > have a page of maintainers, but that page doesn't have emails, and > gerrit seems to want to match the email address. This note was in IRC > 6/recently: > > <Hauke> how do I find out who maintains which part of the code? > <Hauke> the linux kernel has a nice ./scripts/get_maintainer.pl script > which returns the people that a patch should be send to > > as well as the thought that there could be a MAINTAINERS file in the > codebase itself. Gerrit has a feature to automatically add people. We had that in Tizen, but it added upwards of 50 people, which meant no one ever reviewed anything. So I didn't want that feature turned on for us when we started IoTivity, for fear of the same. But if we can keep it to a minimum (no more than 4 people), then we could turn it on. > 5. You need to identify the person who can approve and merge your > change, certainly, but will also want to identify other appropriate > code reviewers. Any thoughts on how to do that beyond "just knowing"? git log is your friend. I always tell people who submit to projects I work on: do a git log on the file you changed and similar files and see who's been submitting and who's been reviewing changes. Of course, skip trivial changes like whitespace fixes, copyright year updates, global search-and- replace, etc. [Uz] -> We need to clarify reviewer list in the wiki page I think. > 6. Is there a policy on whether commits require a matching Jira ticket? > Mandatory? Suggested? Completely optional? Current policy is that it's optional, except for during RC time. At that time, every change to be accepted into the next RC needs to have a matching JIRA ticket of sufficient priority. RM and QA set the rules. [Uz] -> Better to put the jira code into commit title as rule in case there is associated jira. > 7. Is it expected that reviewers have to re-review after each new > patchset in a fix? That seems kind of inefficient if you've approved > the code changes, then the patch needs rebasing because of other > activity but the changes in the patch were not affected. Or is it > better not to try to make such distinctions and just review every time. What I do when I'm a reviewer is that I compare patchset to patchset and review only the delta. That's why rebasing should only be done in case of conflicts and the actual change should be seperate from the rebase. When you do this, Gerrit shows all the deltas plus any section for which there's a comment. So if you left a comment in the previous patch and it still went unchanged, you'll see it. Sometimes, the change was too much or there was a rebase involved, in which case a full re-review is necessary. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center _______________________________________________ iotivity-dev mailing list iotivity-dev at lists.iotivity.org https://lists.iotivity.org/mailman/listinfo/iotivity-dev
