[PATCH] RFC: CONTRIBUTING: Embrace gitlab

Thu Sep 13 10:09:02 UTC 2018

On Thu, Sep 13, 2018 at 11:35 AM, Jani Nikula <jani.nikula at intel.com> wrote:
> On Thu, 13 Sep 2018, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>> On Thu, Sep 13, 2018 at 10:54 AM, Jani Nikula <jani.nikula at intel.com> wrote:
>>> On Wed, 12 Sep 2018, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>>>> On Wed, Sep 12, 2018 at 7:50 PM, Sean Paul <sean at poorly.run> wrote:
>>>>> On Wed, Sep 12, 2018 at 1:34 PM Lucas De Marchi
>>>>> <lucas.demarchi at intel.com> wrote:
>>>>>> - What should we do regarding the a-b, r-b? Is gitlab going to add those
>>>>>> automatically?
>>>>
>>>> No automation, and right now I'd suggest we squash them in and update
>>>> the merge request. We definitely need to squash them in for the
>>>> kernel, so this is also one of those thing I'd want to look into
>>>> automating.
>>>>
>>>>>> I would not check the 2 boxes to disallow direct pushes until those are
>>>>>> figured out.
>>>>>
>>>>> Given the size of this repo and the number of contributors, I'm not
>>>>> convinced either of these should be blockers. We should avoid merge
>>>>> commits since the volume is low enough that having to rebase should
>>>>> be quite rare. Reviews and acks will be recorded in the merge request,
>>>>> which is easily accessed from the UI.
>>>>
>>>> Not sure recording review&acks on the merge request is going to fly
>>>> for kernel. And that's kinda what I want to test-drive here, in 1st
>>>> gear :-)
>>>
>>> If the acks and reviews aren't recorded in git log for each commit, they
>>> don't exist.
>>>
>>> For submitting patches and merging them I can live with having to point
>>> and click on a web site.
>>>
>>> Both of those seem like something that can be automated too.
>>>
>>> But by far the biggest change that is being proposed here between the
>>> lines is moving code review to a gitlab web site, in a one-size-fits-all
>>> model. I could no longer choose and use the tools I prefer for code
>>> review. Tools that I can and have customized for my needs and
>>> preferences. I could no longer review code using the same tools I use to
>>> read and write code (as well as email, and pretty much everything
>>> else). That would be a severe performance hit for me.
>>>
>>> Perhaps I could tolerate that for maintainer-tools, but I won't endorse
>>> gitlab based reviews for any kernel work.
>>>
>>> With that, I think the question should be how we adapt gitlab to our
>>> needs, not how we adapt our needs to gitlab. How do we preserve email
>>> based review while trying to get as much of the benefits of gitlab as
>>> possible? And shouldn't the trials of (at least some of the) smaller
>>> projects such as maintainer-tools be taking that into account, instead
>>> of test driving something that won't fly in kernel?
>>
>> "How to do review?" Is one of the things I want to figure out. One of
>> the things we _need_ to figure out, before we can roll this out more
>> widely. Dogfooding seems like a good way to go about that. There's
>> also a mail gateway somewhere, which I haven't really tried yet at all
>> (also I think it's not yet set up).
>>
>> We could also do a middle thing, where we do both (i.e. you can do
>> gitlab merge request, or traditional email). And then see where people
>> gravitate towards.
>
> That gauges where the *submitters* gravitate, not where the *reviewers*
> gravitate. You should submit *both* a merge request and emailed patches
> to see where the reviewers go. That seems like something that could be
> scripted, and you'd get the benefits of email based review and the
> gitlab CI and merge, even if without much integration between the two.

Yeah, that should be doable. I also started playing around a bit with
the review stuff, and for simple one-off patches it seems to work ok.
But for huge piles of commits the flow in gitlab seems seriously less
useful than what github can do - the per-commit review flow looks very
unpolished. Or I'm blind.

So for anything substantial/detailed we'll probably need to keep the
email per-patch review anyway.

> Regarding merge requests, how would I as a reviewer merge it to my local
> tree for testing? As it is, applying patches from email is a matter of
> typing '| git am' for me.

There's a magic remote refs structure. For this one:

git fetch maintainer-tools refs/merge-requests/3/head

There's also some suggestions for auto-fetching these, if you care:

https://docs.gitlab.com/ee/user/project/merge_requests/#checkout-locally-by-modifying-git-config-for-a-given-repository

So totally untested, but what about the following as an idea for a
workflow we could try to script and see how awful it is:

Create a new tool glim for gitlab inglorious maintainer (I'm bad at
naming, who new). I think would be good to detach it a bit from dim,
to avoid confusion and hopefully it's useful for more than just
maintainer-tools itself. Also, we could use that as the excuse to
start using python :-)

Add a subcommand glim create-mr which:
- Creates a local + remote branch in your gitlab personal fork for your upstream
- Plus creates a merge request out of that to the place you forked
from. Bonus for firing up a text editor for the cover letter in the
merge request.

Add a subcommand glim send-mr which:
- Checks whether there's a merge request on your branche's remote
- Grabs the cover letter for the same (hooray for finally being able
to store these somewhere)
- Automatically adds links to the mr UI + the right special remote so
you can grab it.
- Dumps the mails onto a mailing list (we could vacuum up all the
other arguments for git send-email) for detailed review.

One downside is that if you review on the m-l, this will not
automatically create an open discussion point on the mr, so not
perfect integration. Which is a bummer, since one thing that annoys me
seriously with mail-based reviews is that the contributor can just
resend, and all my comments are outdated/lost. With a gitlab mr, they
get automatically moved forward (if the relevant code line still
exists in the new version). So would have been nice if that's
possible.

Another potential issue: glim send-mr wouldn't do in-reply-to of
individual commits (or well doing that would be hard to script I
think). So might make the everybody-spams-full-resends issue we have a
notch worse.

And for single-patches this is probably a bit overkill (cover letter
and everything) but oh well.

Thoughts?
-Daniel

>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch