Re: Allura/SourceForge to Gitlab migration

[Urs Liska]

(CCing the list again)


Am 10.04.2018 um 01:07 schrieb Carl Sorensen:
> On 4/9/18, 4:20 PM, "Urs Liska" <address@hidden> wrote:
>
>      
>      
>      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:
>      
>      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.
>
> The CG says we are to squash them; a brief search of the history at Savannah 
> shows me no merge commits except translation and release/unstable (which is 
> done as part of a release).  Are there more merges happening than I am aware of?

405cba59df2443b14cade4510ad140c77ca58807
0f3898aade7077c15adb2b457e6a5e1238939085
for example.

They are indeed very rare, but if you browse through the output of
git log --merges --pretty=oneline | grep -v translation | less
you'll see that there *are* some.

I recall a conversation where I was told to prefer fast-forward merges 
(or single commits on staging) over merging feature branches. However, 
depending on the situation it might be better to perform a merge if it 
seems useful to preserve the history of a complicated patch.

>      
>      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".
>      
> Un-cleaned-up history messes up git-bisect, doesn't it?

I'm not fully clear about that, but I'd say as long as all commits that 
end up on master are clean it should be fine.

>      [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".
>
> If we have a straightforward way of setting this up, it would be a big 
> advantage.  We used to have scripts to do it, but they went away when we moved 
> to SourceForge/Allura.  I spent some time trying to get them reimplemented, but 
> didn't get it done.  So now James has to do it manually.

Triggering a test build should be straightforward with Gitlab's concept 
of continuous integration or by Git hooks. The Gitlab server can send 
out an HTTP request to some URL as a response to Git events like pushes. 
This can be configured to trigger a test build whenever a Merge Request 
is opened (either on the Gitlab server or on another server). Of course 
this has to be programmed by someone and I don't have a clear idea how 
complex it would become.

I *assume* it should be possible to use the Gitlab API to automatically 
move the MR/issue (internally a Merge Request *is* an Issue) to a "new" 
column in a "review" project, but I have never done anything in that area.

>      
>      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.
>
> James does manual work for all of those transitions.  The extra work is not 
> setting the flags, but in actually doing the merge.

Maybe also this could be automated using the API: move any issue that is 
in column A to column B etc. But again I have no clear idea about the 
effort, and moving stuff manually might be easier since handling 
corner/special cases is of course much easier when done by a human.

>      
>      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.
>
> If merging is something other than a fast-forward, it can get challenging 
> really quickly for a new contributor.

Well, yes, of course.
But I don't see how this should make a difference:
> I've tried to work through merging with several git novices, and all of them 
> have had problems with it.  Our current practice has them struggling with it on 
> their own private branches, and then pushing to staging only once they have it 
> worked out.  I think struggling with merging as a part of applying a patch on 
> staging would be a real challenge for relatively new developers.

We're talking about the case when someone commits to his working branch 
and the changes don't apply cleanly to master/staging.
In this case a tool like Gitlab would state that it can't merge the 
branch automatically and refer the user to fixing it locally (maybe 
there are or will be tools to assist in the web interface but I'd say we 
don't consider them for now).
But in the current workflow the problem is the same: the contributor is 
forced to either merge or rebase his working branch and deal with the 
conflicts. I don't see how this is easier currently than it would be in 
a Merge Request context. (You could even argue that in the Merge Request 
it is much more natural for someone else to take over and help the new 
contributor. But that's not a real argument since you *can* do that as 
well if the contributor has pushed his working branch).

>      
>      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.
>
> It certainly doesn't enforce the patch applied being the same as the reviewed 
> patch.
> But it seems like the same could be said for the GitHub flow -- nothing stops a 
> contributor who is doing his own merge from adding unreviewed commits before 
> doing the merge.

Indeed, in a Gitlab context one would probably not enforce significantly 
more than we're doing right now. And basically a project like LilyPond 
works pretty well based on the trust that contributors more or less do 
The Right Thing.
But when a Merge Request will receive additional commits it will 
immediately be visible while currently it is pretty much hidden.

>      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.
>
> I actually had a hard time seeing how the comments tied to the reviewed code 
> when I looked at the two examples you showed me.  Clearly things were tied in 
> in terms of the reviewed *features*, but I couldn't see where a single comment 
> was made on the reviewed code.  One of the things I like about Rietveld is the 
> ability to comment on the code directly.  I didn't see that applied to the 
> merge request review.  Did I miss something?

Only what I didn't show you ;-)

>      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 certainly take your word for this; you have lots of experience with it 
> that I don’t.
>
>      I'll point you to some examples from recent work on the lyluatex package.
>      Take https://github.com/jperon/lyluatex/pull/155 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.
>      https://github.com/jperon/lyluatex/pull/131 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.
>      
> Again, I saw a *feature* review, but not strong *code* review.  Maybe I'm not looking at it properly.  I'd love to see what I'm missing.

This is a better example:
https://github.com/wbsoft/frescobaldi/pull/1063

You can see how I asked for review (this is handled differently in 
Gitlab, there you can assign a MR to someone if you want), then a 
general comment and then three code comments. These are folded because 
they are out of date (i.e. refer to an earlier version of the code. But 
you can open them to see how they refer to code, and how they spawned 
discussion about each of them, including references to commits I did as 
a response.

You can also see how interactions are automatically displayed in the 
timeline, e.g. approvals, the added commits etc. If we would have moved 
the issue through the columns of the project this would also have 
created such automatic entries that would stay in the timeline.
So if the issue moves to the "push" column, and the contributor adds 
further commits after that it will be immediately visible.

Best
Urs

> Thanks,
>
> Carl
>