[Mesa-dev] [PATCH] docs: Document optional GitLab code review process

[Jordan Justen]

On 2018-11-28 06:59:42, Eric Engestrom wrote:
> On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:
> > On 2018-11-27 19:20:09, Matt Turner wrote:
> > > 
> > > Discussion point: I think attempting to have simultaneous review in
> > > two places is a recipe for wasted time.
> > 
> > That's possible. It also happens on email sometimes. But, I want to
> > say that maybe the usual problem is too little code review, and not
> > too much? :)
> 
> I think duplicating the review possibilities might very well lead to
> less review as people might (unconsciously) think "it has been/will be
> reviewed on the other one".

Maybe they were looking for an excuse to not do code review? :)

I agree with your point to some extent, but I think it's better to try
to work out a transition. To not jump immediately to forcing people to
go to the web page.

> > 
> > > Strawman: maybe we can only email the cover letter to the mailing
> > > list and include in it a link to the MR?
> > 
> > I was hoping to make a smaller step and see what happens. Maybe this
> > will give people the chance to try out MR based reviews, but not take
> > away email based reviews just yet.
> > 
> > I don't think we should move away from email based reviews until we
> > are sure MR's actually work better. (I'm far from convinced on this
> > point. :)
> 
> I think one way to solve this is to allow both, but only one at a time.
> Devs can choose to send their patches/series as either an MR _or_ as
> emails, but not both. One can still send a cover-letter style email to
> the ML to present the MR with a link to it.

I would prefer to keep emails as required for now. Let people
optionally try it out for some time.

Perhaps as a next step we can let the poster consider using a MR and
only a cover letter. (Assuming the MR method proves useful. If it
doesn't we should revert back to email only.)

> I also agree with Matt that we need to disable everything that shouldn't
> be used, but unfortunately we can't disable the merge button without
> disabling merge requests.

I guess we can't do this. Can we just tell people with push access to
not do that again if they make a mistake?

Can merge request approvals be used to only prevent the web page from
merging things? (But, still allow pushes.)
https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html

Maybe require approval from "dont-merge-use-git-push"? :)

> As for auto-closing issues and MRs by adding a special string to the
> commit message, GitLab doesn't support that for commits that are pushed
> behind its back, but it does support it it GitLab is used.

Are you sure? Pushes seem to work this way on github, and my reading of
https://docs.gitlab.com/ee/user/project/issues/automatic_issue_closing.html
is that on gitlab pushes do the same.

> Adding a post-receive hook that does that should be possible, I'm not
> sure how much work that would be and if it would be worth it.
> DanielS?
> 
> Other things that need to be present in this 'GitLab MR' document are:
> 
> - Review process: when you change something in your MR, force-push the
>   new version, don't add fixup commits. The history of the MR at any one
>   time must be "clean" in the sense that it would be acceptable to push
>   it as is.

We don't document this for email based reviews, but people seem to
figure it out. :)

Is this more of a concern because they might not know that pushing the
same branch updates the MR?

If we only use git push, isn't this also unlikely to cause a problem
in the main branch?

> - WIP MRs: prefix your MR title with "WIP" to indicate that it's a work
>   in progress and is not ready to be merged as is (Side note: GitLab
>   will not offer to merge an MR that is prefixed with "WIP")

If we are saying that merge should not be used from the web page, then
I don't think this is in scope yet.

> - I think we should split the list in two between the dev side and the
>   reviewer side.

I'm not sure I understand what you mean, but it sounds bad. :)

> - When an issue is raised, who closes it? The dev to indicate that the
>   new revision should fix it, or the reviewer to indicate that they have
>   verified the fix?

The dev has to fix issues with getting their code merged.

If you are talking about a case of someone posting an MR that doesn't
have push access, then still the dev. In trivial cases the person with
push access can choose to go out of their way to fix up a minor issue.

We don't document the similar case with email patches, so I don't
think this is something separate to consider documenting here.

-Jordan