<div dir="ltr"><div class="gmail_quote"><div>First off, +1 to experimenting with MRs.  I've been working with GitLab MRs in another context for some time and I think the process actually works out really pretty well.  There are issues, of course, but I don't think there's any real show-stoppers as long as we have a bit of process around it such as requiring the submitter to apply Reviewed-by tags manually before merging.<br></div><div dir="ltr"><br></div><div dir="ltr">On Wed, Nov 28, 2018 at 11:35 AM Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2018-11-28 06:59:42, Eric Engestrom wrote:<br>
> On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:<br>
> > On 2018-11-27 19:20:09, Matt Turner wrote:<br>
> > > <br>
> > > Discussion point: I think attempting to have simultaneous review in<br>
> > > two places is a recipe for wasted time.<br>
> > <br>
> > That's possible. It also happens on email sometimes. But, I want to<br>
> > say that maybe the usual problem is too little code review, and not<br>
> > too much? :)<br>
> <br>
> I think duplicating the review possibilities might very well lead to<br>
> less review as people might (unconsciously) think "it has been/will be<br>
> reviewed on the other one".<br>
<br>
Maybe they were looking for an excuse to not do code review? :)<br>
<br>
I agree with your point to some extent, but I think it's better to try<br>
to work out a transition. To not jump immediately to forcing people to<br>
go to the web page.<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> > <br>
> > > Strawman: maybe we can only email the cover letter to the mailing<br>
> > > list and include in it a link to the MR?<br>
> > <br>
> > I was hoping to make a smaller step and see what happens. Maybe this<br>
> > will give people the chance to try out MR based reviews, but not take<br>
> > away email based reviews just yet.<br>
> > <br>
> > I don't think we should move away from email based reviews until we<br>
> > are sure MR's actually work better. (I'm far from convinced on this<br>
> > point. :)<br>
> <br>
> I think one way to solve this is to allow both, but only one at a time.<br>
> Devs can choose to send their patches/series as either an MR _or_ as<br>
> emails, but not both. One can still send a cover-letter style email to<br>
> the ML to present the MR with a link to it.<br>
<br>
I would prefer to keep emails as required for now. Let people<br>
optionally try it out for some time.<br>
<br>
Perhaps as a next step we can let the poster consider using a MR and<br>
only a cover letter. (Assuming the MR method proves useful. If it<br>
doesn't we should revert back to email only.)<br></blockquote><div><br></div><div><div>I agree with Eric on this one.  If a single 
submission has both types of review then you will effectively end up 
with two different groups of reviewers providing potentially conflicting
 feedback without seeing each other's feedback and the submitter has to mediate.  It's better just to have 
any given series have a single point for review.  Yes, this means that 
everyone will be forced to start doing GitHub review as soon as someone 
does an MR that's relevant to them.  I don't see a good way around that 
which doesn't make a mess.</div><div><br></div><div>Certain projects 
could, I suppose, have a requirement that all significant work on that 
project be done on the list or on GitLab.  However, people who feel like
 custodians of Mesa as a whole will have to do both as long as both are 
supported.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> I also agree with Matt that we need to disable everything that shouldn't<br>
> be used, but unfortunately we can't disable the merge button without<br>
> disabling merge requests.<br>
<br>
I guess we can't do this. Can we just tell people with push access to<br>
not do that again if they make a mistake?<br>
<br>
Can merge request approvals be used to only prevent the web page from<br>
merging things? (But, still allow pushes.)<br>
<a href="https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html" rel="noreferrer" target="_blank">https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html</a><br>
<br>
Maybe require approval from "dont-merge-use-git-push"? :)<br></blockquote><div><br></div><div>I don't think we can disable pushing the "merge" button.  However, we can require fast-forward merges and we can require either through process or through a Git hook of some form that they have the right tags.  At that point whether the person rebases, applies tags, force pushes the MR branch, and then clicks merge or pushes directly to master is immaterial.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> As for auto-closing issues and MRs by adding a special string to the<br>
> commit message, GitLab doesn't support that for commits that are pushed<br>
> behind its back, but it does support it it GitLab is used.<br>
<br>
Are you sure? Pushes seem to work this way on github, and my reading of<br>
<a href="https://docs.gitlab.com/ee/user/project/issues/automatic_issue_closing.html" rel="noreferrer" target="_blank">https://docs.gitlab.com/ee/user/project/issues/automatic_issue_closing.html</a><br>
is that on gitlab pushes do the same.<br></blockquote><div><br></div><div>I think pushes do close issues.</div><div><br></div><div>It's also worth throwing out there that GitLab has some sort of Bugzilla interaction interface.  I'm not sure what all it lets you do but it's there if we want to experiment with it.  That said, migrating to GitLab issues may be a better plan.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Adding a post-receive hook that does that should be possible, I'm not<br>
> sure how much work that would be and if it would be worth it.<br>
> DanielS?<br></blockquote><div><br></div><div>GitLab does support hooks but it does so through this web interface which is a bit awkward.  Basically, we'd just need a bit of Python CGI code running in a little web server somewhere.  Shouldn't be all that hard to set up but Daniel would likely need to be involved.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Other things that need to be present in this 'GitLab MR' document are:<br>
> <br>
> - Review process: when you change something in your MR, force-push the<br>
>   new version, don't add fixup commits. The history of the MR at any one<br>
>   time must be "clean" in the sense that it would be acceptable to push<br>
>   it as is.<br>
<br>
We don't document this for email based reviews, but people seem to<br>
figure it out. :)<br>
<br>
Is this more of a concern because they might not know that pushing the<br>
same branch updates the MR?<br></blockquote><div><br></div><div>I think it's worth explicitly documenting.  Many people who are used to the MR workflow are also used to having very sloppy branches because they assume that only the final merge matters.  This is why GitLab and GitHub both have an option for "squash before merge". :-(</div><div><br></div><div>--Jason<br></div></div></div>