(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
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?
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
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.
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
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
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:
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
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.
lilypond-devel mailing list