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:
Am 9. April 2018 20:06:20 MESZ schrieb James Lowe <james.l...@runbox.com>:
>Hello,
>
>On Mon, 9 Apr 2018 12:56:57 -0500, Karlin High <karlinh...@gmail.com>
>wrote:
>
>> On 4/9/2018 11:06 AM, Federico Bruni wrote:
>> >
>> > I don't want to revamp this old discussion:
>> >
>http://lists.gnu.org/archive/html/lilypond-devel/2013-10/msg00095.html
>> >
>http://lists.gnu.org/archive/html/lilypond-devel/2013-10/msg00140.html
>>
>> I should have read those BEFORE making my other post.
>>
>> > Current obstacle is that there's no way to import
>Allura/SourceForge
>> > issues into Gitlab. This is the issue to follow:
>> > https://gitlab.com/gitlab-org/gitlab-ce/issues/45007
>> >
>> > If you have a Gitlab account, click on the thumb up icon, as
>popular
>> > 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
staging.
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 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.
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.
Best
Urs
Thanks,
Carl
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
