[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Allura/SourceForge to Gitlab migration

From: Urs Liska
Subject: Re: Allura/SourceForge to Gitlab migration
Date: Tue, 10 Apr 2018 00:20:03 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

Am 09.04.2018 um 21:15 schrieb Carl Sorensen:
On 4/9/18, 1:04 PM, "lilypond-devel on behalf of Urs Liska" <address@hidden on 
behalf of address@hidden> wrote:

Am 9. April 2018 20:06:20 MESZ schrieb James Lowe <address@hidden>:
     >On Mon, 9 Apr 2018 12:56:57 -0500, Karlin High <address@hidden>
     >> On 4/9/2018 11:06 AM, Federico Bruni wrote:
     >> >
     >> > I don't want to revamp this old discussion:
     >> >
     >> >
     >> I should have read those BEFORE making my other post.
     >> > Current obstacle is that there's no way to import
     >> > issues into Gitlab. This is the issue to follow:
     >> >
     >> >
     >> > If you have a Gitlab account, click on the thumb up icon, as
     >> > issues should have higher chances to be tackled sooner than later.
     >> Created account, upvoted the issue. Thanks for pointing this out.
     >Does Gitlab really only just have 2 status for an 'issue' (Open and
     >Closed) or can this be refined/configured so I (as Patch Meister) can
     >keep track of what is 'making its way through' the patch countdown and
     >is at the one of the three basic states?
     >For example how would one know of the ~1000 issues which ones need to
     >be reviewed and which ones are ready to push?
Gitlab (like GitHub) uses Merge Requests for the process of patch review. The nice thing (compared to the current workflow) is that what is reviewed is what will eventually be merged, so it's not at the discretion of the contributor to recreate the patch on staging, rewriting the commits etc. I think combining Merge Requests with the projects as shown by Carl would be a good match to the current strategy. Urs I'm really concerned about using Merge Request rather than commits as a means of applying patches.

I'm not going to argue for the Merge Request approach since there are too many people in the boat with so much more experience than me. But I think that I should point out how most of the concerns you express in your reply stem from either comparing apples with oranges or from pointing out risks that we already have right now. Probably this won't introduce hard arguments *for* the MR approach but should at least disseminate your concerns *against* it.

Right now, our current process ensures that every commit on master builds 
properly -- master never gets broken.  The only branch that can get broken is 

This is not true. Currently we have an *agreement* that nobody pushes to master, but IIRC we actually did have issues occasionally when someone acidentally did so. The idea behind the safeguard you mention is that everybody pushes to *staging*, and this is only merged (automatically) into master after a successful build. This strategy can equally be implemented with the Merge Request approach when the Merge Requests have to be created against staging ("user X wants to merge branch Y into staging"). It is also possible to "protect" branches (e.g. the master branch) against forced pushes or against *any* pushes. So this part's safety is completely equal if not better (depending on whether 'master' is protected on the Savannah server or not (I don't want to try that out ...)). Running a test and merging automatically should be possible to implement on Gitlab too, at least in the self-hosted version (i.e. scripting on the server).

If we use merges instead of single commits, we know that the final result (the 
merge commit) is OK, but we don't know anything about the underlying commits in 
the branch being merged.  Some of those commits may not build properly.  I 
guess those commits won't be on master, but all of the interesting history is 
in those commits, not in the merge commits.

Currently we don't have consistent rules about this either. Small patches are *usually* squashed to a single commit and directly applied to staging, but more complex patches often are applied as merge commits as well.

I admit there is (I think) no straightforward (i.e. intended) way to do fast-forward merges in the web interface, so that *would* probably make a difference in so far as the history would become more "real" and less "cleaned-up".

[Edit: Now that Karlin's research has clarified that Squash-and-merge is an option this isn't any downside anymore.]

Finally, it will require somebody to process the merge requests.  I guess that 
is nice from the point of uniformity, but it will require somebody other than 
the developer who created the patch to be responsible for ensuring that the 
tree is all OK.  I don't know who that person will be.

Not necessarily.
It could be set up that opening a merge request would trigger the test build, upon which the "issue" is moved to the corresponding column ("patch: new") in the review "project".

How do patches *currently* flow through the review process? I.e. how are they changed from "new" to "review" to "countdown" to "push"? Probably this could also be automated, but if James currently does *any* manual step with this it should definitely not be more work to do that on Gitlab.

Also merging doesn't have to be done by someone else, it can still be done by the contributor, that's just a matter of policy. Today as a contributor I have to clean up my branch and apply it to staging, either as a single commit, as a series of commits or as a merge commit. With the Merge Request I would instead simply be allowed to merge my own request.

I find the current process actually pretty unsecure. It *does* ensure that master builds properly, but it doesn't have any serious mechanisms to ensure that what is pushed actually resembles what has been reviewed. It's just the expectation that contributors who have gained sufficient credits to be given push access won't mess around with it. A Merge Request approach doesn't prevent that kind of misbehaviour, but it is generally much more obvious because the review comments are directly integrated with the reviewed code. This makes it easier and more permanent to follow the relation of review and code modfiication. And BTW working this way actually encourages to collaborate on features during review. Maybe that's not what everybody wants but it can really stimulate collective creativity. Currently someone pushes a patch, others comment on it and ask for improvements, the original contributor has to update the patch, others comment on the updated "patch set". In a Merge Request others can not only comment on the patch but also add to the branch, or open "secondary" merge requests with code suggestions. This may sound somewhat complicated but - given some level of trust between the participants - can be truly stimulating, leading to improved results.
I'll point you to some examples from recent work on the lyluatex package.
Take as a quite complex example (I think you don't have to look into it in any detail). After I opened the merge request (on Github they're called Pull Requests) we had much discussion, and I added numerous additional commits - but Jacques occasionally added his own commits as well. shows a pull request that actually triggered further enhancements by the other contributor. I think if I'd look more thoroughly I'd find other pull requests where the process was even more of a give-and-take.

I may just be an old dog who doesn't want to learn new tricks, but I feel like 
the current process is really secure as far as keeping master working, and puts 
the onus on the developer who is creating the patch to see that it's done 
right.  And I think that's the proper place for it.

I'm certainly willing to learn from people like Urs, who have experience in 
running a GitLab project with multiple contributors.  I could be convinced that 
it's better.  I just haven't heard an argument for why it's better.

As said I won't make that argument here, but I have come to enjoy advantages of the Merge Request approach that seems to actually improve the results through the collaborative nature and the fact that the review process is transparently visible directly from the Git history. Currently if you're interested in any discussion around a given patch you'll have to look into the commit message to locate the issue number, go to the issue tracker to locate the Rietveld URL, perhaps check if the code on Rietveld actually corresponds to the code in the Git commits and finally read the comments.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]