On Thu, Sep 13, 2018 at 11:35 AM, Jani Nikula <[email protected]> wrote: > On Thu, 13 Sep 2018, Daniel Vetter <[email protected]> wrote: >> On Thu, Sep 13, 2018 at 10:54 AM, Jani Nikula <[email protected]> wrote: >>> On Wed, 12 Sep 2018, Daniel Vetter <[email protected]> wrote: >>>> On Wed, Sep 12, 2018 at 7:50 PM, Sean Paul <[email protected]> wrote: >>>>> On Wed, Sep 12, 2018 at 1:34 PM Lucas De Marchi >>>>> <[email protected]> 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 _______________________________________________ dim-tools mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dim-tools
