(CCing the list again)

Am 10.04.2018 um 01:07 schrieb Carl Sorensen:

On 4/9/18, 4:20 PM, "Urs Liska" <li...@openlilylib.org> 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" 
<lilypond-devel-bounces+c_sorensen=byu....@gnu.org on behalf of li...@openlilylib.org> 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


_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Reply via email to