Re: streamline github dev process
On 1 June 2017 at 03:43, Helmut K. C. Tessarek wrote: > On 2017-05-31 20:42, Joshua Root wrote: >> That's an unavoidable side effect of rebasing (or squashing) -- it's no >> longer the same commit that you signed. > Right, this is why one usually does a merge with --no-ff. Thus my commit > is intact and the merge commiter gets their own commit. Problem solved. > > There's no reason for doing it any other way. > > Why do a rebase for a PR? Especially in this project it's highly > unlikely that there will be a conflict. But even if there were, the same > conflict would have happened with a rebase. I see no reason for not having a linear history other than perhaps signatures (and complexity of work). But then there are far more cases when some trivial modifications of PRs are needed and the majority of contributors didn't set up signatures anyway. (With Trac/SVN there was even no attribution.) Maybe we could review our guidelines in the future, but at the moment it looks nicer to me to avoid a zillion of merge commits. Mojca
Re: streamline github dev process
On 2017-05-31 20:42, Joshua Root wrote: > That's an unavoidable side effect of rebasing (or squashing) -- it's no > longer the same commit that you signed. Right, this is why one usually does a merge with --no-ff. Thus my commit is intact and the merge commiter gets their own commit. Problem solved. There's no reason for doing it any other way. Why do a rebase for a PR? Especially in this project it's highly unlikely that there will be a conflict. But even if there were, the same conflict would have happened with a rebase. Cheers, K. C. -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */
Re: streamline github dev process
On 2017-6-1 08:19 , Helmut K. C. Tessarek wrote: I would like that the buildbots are triggered for a PR. only when my PR is commited, I can see, if the build actually works. it should be the other way around: I should be able to verify that the PR really does what it is intended to do (since we do not have direct access to the build machines to test stuff). Our current buildbot set-up is not well-suited for this. We have a GSoC project under way to test macports-ports PRs with Travis CI, which uses a clean VM for each build and has existing integration with GitHub. - Josh
Re: streamline github dev process
On 2017-6-1 08:18 , Helmut K. C. Tessarek wrote: Also, I have noticed that whenever you guys commit my PRs, my gpg signature is gone. I believe we should find a proper way to handle this. That's an unavoidable side effect of rebasing (or squashing) -- it's no longer the same commit that you signed. - Josh
Re: streamline github dev process
On 2017-05-31 13:11, Daniel J. Luke wrote: > One other (more radical) idea would be to split the ports tree into > two - one where changes are auto-committed if they pass certain tests > (lint ok, buildbot ok, test suite ok), and the 'curated' tree where > someone has done a review and merged from the auto-committed one. I > don't know if a universe where that exists is better, though (it > would be pretty trivial to create a Portfile that could pass > automated checks but do something bad if run on an end-user's > machine). git allows submodules, but they can be a PAIN. I would like that the buildbots are triggered for a PR. only when my PR is commited, I can see, if the build actually works. it should be the other way around: I should be able to verify that the PR really does what it is intended to do (since we do not have direct access to the build machines to test stuff). e.g. if I had write access to the entire repo, I'd still only commit my own 3 ports - unless somebody specifically asks me to review and test a new or updated port. -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */
Re: streamline github dev process
On 2017-05-31 11:55, Zero King wrote: > We disabled "squash and merge" on GitHub's web interface so we have to > do `git merge --squash` locally to complete the typical workflow, much > harder than clicking a button twice. In certain circumstances (some ppl love to spread out one change into several commits) I like to squasch commits. I don't really use the web UI for anything, because I only use the git command line. Otherwise I rather have more commits than only one. One commit per change. A lot of people use one commit for several changes, which makes it very complicated to back something out. > As such, my personal preference would be that PRs stay in a clean state > (ready for "rebase and merge"), so the typical workflow would worth a > separate Gist. Perhaps I should write about resolving conflicts with > `git mergetool` in another Gist too. I don't how you guys handle this, but I like to use --no-ff for merge commits. Also, I have noticed that whenever you guys commit my PRs, my gpg signature is gone. I believe we should find a proper way to handle this. Cheers, K. C. -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */
Re: streamline github dev process
On May 31, 2017, at 4:40 AM, Mojca Miklavec wrote: > What I believe could help a bit would be to get some > "mentors" assigned to new maintainers. Then those mentors would be > kind of obliged to review submissions from them and keep track of > their progress and vouch for commit rights once applicable. But this > would need a bit more thought. we used to do this (but maybe just for when someone was granted commit access, fkr was my mentor). If there are experienced contributors who are willing to do this, then some focus on this would likely help us get the number of regular contributors up (which would help fix these kinds of issues). > In theory GitHub's pull requests should allow to have *much less* > committers. In theory doing the reviews and merging pull requests > should be much easier that doing the same thing on Trac where the > patches get outdated, cannot be reviewed on line-by-line basis etc. In > practice I need to have a cheatsheet for merging pull requests and do > some not-anywhere-easy-to-remember steps to be able to merge trivial > PRs when some modifications are desired. Streamlining the PR workflow is still maybe a good idea (I don't know enough to recommend any changes here, though). One other (more radical) idea would be to split the ports tree into two - one where changes are auto-committed if they pass certain tests (lint ok, buildbot ok, test suite ok), and the 'curated' tree where someone has done a review and merged from the auto-committed one. I don't know if a universe where that exists is better, though (it would be pretty trivial to create a Portfile that could pass automated checks but do something bad if run on an end-user's machine). -- Daniel J. Luke
Re: streamline github dev process
On 2017-05-31 18:25, Zero King wrote: > On Wed, May 31, 2017 at 06:09:44PM +0200, Rainer Müller wrote: >> On 2017-05-31 16:38, Craig Treleaven wrote: >>> 0) The wiki currently includes the following: >>> >>> https://trac.macports.org/wiki/WorkingWithGit >>> >>> I presume we would adapt your Gist to become something like >>> “WorkingWithGitHubPullRequests”? I think the current page would be >>> unweildy if the gist was tacked on. >> >> Isn't the process already described there? >> >> https://trac.macports.org/wiki/WorkingWithGit#WorkingwithsomeoneelsespullrequestthroughitsID >> > > Pushing to PR submitter's branch is missing there and PR would *not* be > marked as merged unless it's a fast-forward merge. You do not have to push to the PR branch, marking it as closed is covered with the "Closes: #xx" line. > I'm not sure if we should keep all these on a single wiki page. It would > be really lengthy. Agreed. There is quite a lot of general information on git mixed with information on the way we work at MacPorts. I propose to split the sections on "Common git tasks" for ports/base to their own wiki pages: https://trac.macports.org/wiki/WorkingWithGit#Commongittaskswhileworkingwithports https://trac.macports.org/wiki/WorkingWithGit#CommongittaskswhileworkingwithMacPortsbase Rainer
Re: streamline github dev process
On Wed, May 31, 2017 at 06:09:44PM +0200, Rainer Müller wrote: On 2017-05-31 16:38, Craig Treleaven wrote: 0) The wiki currently includes the following: https://trac.macports.org/wiki/WorkingWithGit I presume we would adapt your Gist to become something like “WorkingWithGitHubPullRequests”? I think the current page would be unweildy if the gist was tacked on. Isn't the process already described there? https://trac.macports.org/wiki/WorkingWithGit#WorkingwithsomeoneelsespullrequestthroughitsID Pushing to PR submitter's branch is missing there and PR would *not* be marked as merged unless it's a fast-forward merge. I'm not sure if we should keep all these on a single wiki page. It would be really lengthy. -- Best regards, Zero King Don't trust the From address. smime.p7s Description: S/MIME cryptographic signature
Re: streamline github dev process
On 2017-05-31 16:38, Craig Treleaven wrote: > 0) The wiki currently includes the following: > > https://trac.macports.org/wiki/WorkingWithGit > > I presume we would adapt your Gist to become something like > “WorkingWithGitHubPullRequests”? I think the current page would be > unweildy if the gist was tacked on. Isn't the process already described there? https://trac.macports.org/wiki/WorkingWithGit#WorkingwithsomeoneelsespullrequestthroughitsID Rainer
Re: streamline github dev process
On Wed, May 31, 2017 at 10:38:45AM -0400, Craig Treleaven wrote: On May 31, 2017, at 8:52 AM, Zero King wrote: I wrote a Gist about making changes to PRs, feedback via email is welcome. https://gist.github.com/l2dy/7da9621954ebcf1a19869f391662a41e I currently know very little about the GitHub pull request process. I’d like to understand better and this write-up helps a lot. Some observations, ... 0) The wiki currently includes the following: https://trac.macports.org/wiki/WorkingWithGit I presume we would adapt your Gist to become something like “WorkingWithGitHubPullRequests”? I think the current page would be unweildy if the gist was tacked on. 1) Gist lines 1-18 are similar to the existing Basic Setup and Cloning a Repository sections of the WorkingWithGit (WWG) wiki page. Could we avoid repeating by modifiying the existing page? If adapted to wiki, yes. 2) I like your Disaster Recovery sections. I wonder if we could use wiki formatting to make them standalone boxes with emphasis. 3) Around line 24, I think there should be some preamble giving an overview of the typical workflow for a PR. Ie, after the PR is initially proposed, the initiator and review may go back and forth a number of times amending the PR to address issues, etc. When ready, the intermediate commits are squashed (I don’t know GitHub well, is this the right term?) so that the history in MacPorts doesn’t include extraneous commits. I don’t see how this is addressed in the gist. We disabled "squash and merge" on GitHub's web interface so we have to do `git merge --squash` locally to complete the typical workflow, much harder than clicking a button twice. As such, my personal preference would be that PRs stay in a clean state (ready for "rebase and merge"), so the typical workflow would worth a separate Gist. Perhaps I should write about resolving conflicts with `git mergetool` in another Gist too. 4) Line 101-103 - I don’t understand why one would need to do this? After a PR has been commited, wouldn’t one just delete the local branch? See `man git-gc`. Usually `git gc` would work but if you need to squeeze more disk space out of the repository you can use this *aggressive* command. Note that it shouldn't be used regularly. Craig -- Best regards, Zero King Don't trust the From address. smime.p7s Description: S/MIME cryptographic signature
Re: streamline github dev process
> On May 31, 2017, at 8:52 AM, Zero King wrote: > > On Wed, May 31, 2017 at 10:40:07AM +0200, Mojca Miklavec wrote: >> The new bot could solve some of those problems, but it's still a pity >> that maintainers cannot have slightly higher permissions set. > > What kind of slightly higher permissions? I can make the bot auto-merge > PRs approved by all related maintainers but that sounds dangerous. > >> In theory GitHub's pull requests should allow to have *much less* >> committers. In theory doing the reviews and merging pull requests >> should be much easier that doing the same thing on Trac where the >> patches get outdated, cannot be reviewed on line-by-line basis etc. In >> practice I need to have a cheatsheet for merging pull requests and do >> some not-anywhere-easy-to-remember steps to be able to merge trivial >> PRs when some modifications are desired. > > I wrote a Gist about making changes to PRs, feedback via email is > welcome. https://gist.github.com/l2dy/7da9621954ebcf1a19869f391662a41e I currently know very little about the GitHub pull request process. I’d like to understand better and this write-up helps a lot. Some observations, ... 0) The wiki currently includes the following: https://trac.macports.org/wiki/WorkingWithGit I presume we would adapt your Gist to become something like “WorkingWithGitHubPullRequests”? I think the current page would be unweildy if the gist was tacked on. 1) Gist lines 1-18 are similar to the existing Basic Setup and Cloning a Repository sections of the WorkingWithGit (WWG) wiki page. Could we avoid repeating by modifiying the existing page? 2) I like your Disaster Recovery sections. I wonder if we could use wiki formatting to make them standalone boxes with emphasis. 3) Around line 24, I think there should be some preamble giving an overview of the typical workflow for a PR. Ie, after the PR is initially proposed, the initiator and review may go back and forth a number of times amending the PR to address issues, etc. When ready, the intermediate commits are squashed (I don’t know GitHub well, is this the right term?) so that the history in MacPorts doesn’t include extraneous commits. I don’t see how this is addressed in the gist. 4) Line 101-103 - I don’t understand why one would need to do this? After a PR has been commited, wouldn’t one just delete the local branch? Craig
Re: streamline github dev process
On Wed, May 31, 2017 at 10:40:07AM +0200, Mojca Miklavec wrote: The new bot could solve some of those problems, but it's still a pity that maintainers cannot have slightly higher permissions set. What kind of slightly higher permissions? I can make the bot auto-merge PRs approved by all related maintainers but that sounds dangerous. In theory GitHub's pull requests should allow to have *much less* committers. In theory doing the reviews and merging pull requests should be much easier that doing the same thing on Trac where the patches get outdated, cannot be reviewed on line-by-line basis etc. In practice I need to have a cheatsheet for merging pull requests and do some not-anywhere-easy-to-remember steps to be able to merge trivial PRs when some modifications are desired. I wrote a Gist about making changes to PRs, feedback via email is welcome. https://gist.github.com/l2dy/7da9621954ebcf1a19869f391662a41e -- Best regards, Zero King Don't trust the From address. smime.p7s Description: S/MIME cryptographic signature
Re: streamline github dev process
On 2017-05-31 10:40, Mojca Miklavec wrote: > On 30 May 2017 at 20:23, Helmut K. C. Tessarek wrote: >> Maybe you could allow maintainers to review and approve other pull requests. > > This would be ideal, but I don't know if there is any way to allow > that. This is again a question for the GitHub developers. Anyone on GitHub should be able to review all pull requests. It is also possible to comment on pull requests, and the same applies to tickets on Trac. But if we had people interested in doing reviews, we would not have to discuss this... Rainer
Re: streamline github dev process
On 30 May 2017 at 20:23, Helmut K. C. Tessarek wrote: > Hello, > > Maybe we can streamline the github process a bit. As Mojca mentioned > earlier, the macports project is heavily understaffed. > > Is there a way to allow maintainers to set labels? (e.g. the > 'maintainer' or 'update' label) This is probably a question for GitHub developers. On the other hand, Zero King just started working on a Summer of Code project where one of the goals was to set those labels by a bot in an automated way. One of the problems with "maintainer" label is not that much setting the label itself, but actually knowing that the person who previously identified himself/herself with email is the same as the person with that username who submitted a patch. The problem here is two-fold: - For most of the ports we don't have the GitHub handle in the Portfile yet (and the change that we want this is not documented at all places and sample Portfiles yet) - Even if the GitHub handle *is* in the Portfile, one still needs to manually check the Portfile (or run 'port info') to see who the maintainer is. The new bot could solve some of those problems, but it's still a pity that maintainers cannot have slightly higher permissions set. Nonetheless, the biggest problem is still convincing someone to review the code and merge it. The label alone doesn't help that much if nobody is looking at PRs. > Also you could block merges until 1 or 2 people have reviewed and > approved the change. We already have problems getting a single person to review the changes, getting two people to agree would be an overkill at this point. > Maybe you could allow maintainers to review and approve other pull requests. This would be ideal, but I don't know if there is any way to allow that. This is again a question for the GitHub developers. > This is git. It's very easy to revert a broken commit. Since almost all > of these commits are not conflicting, it's even easier to do so. > I believe that we need more committers, even if you just allow > maintainers to commit their own ports. > > I do understand that a commit to this repo can affect a lot of people, > but on the other hand we need more traction and if a maintainer breaks > his port, he/she will have to fix it anyway. > > Does any of my suggestions make any sense? Ryan explained most points here. In reality we have a bit more "organisational" than "technical" problems here. What I believe could help a bit would be to get some "mentors" assigned to new maintainers. Then those mentors would be kind of obliged to review submissions from them and keep track of their progress and vouch for commit rights once applicable. But this would need a bit more thought. We have *a lot* of commits, but of course that's not enough when we have to deal with tens of thousands of ports. In theory GitHub's pull requests should allow to have *much less* committers. In theory doing the reviews and merging pull requests should be much easier that doing the same thing on Trac where the patches get outdated, cannot be reviewed on line-by-line basis etc. In practice I need to have a cheatsheet for merging pull requests and do some not-anywhere-easy-to-remember steps to be able to merge trivial PRs when some modifications are desired. Mojca
Re: streamline github dev process
You can achieve that nicely by either editing the revert commit after-the-fact (git commit --amend) or you can actually ask revert to not auto-commit (git revert -n). This way you can make further necessary edits while keeping to one solid revert commit. On 05/30/2017 08:34 PM, Helmut K. C. Tessarek wrote: > This is why in step 2 you set it to v1.2 (which was supposed to be the > epoch equivalet in my example). > Sorry, I should have listed the steps more clearly. > > I understand that you have to increment it, otherwise users won't see > the update.
Re: streamline github dev process
On 2017-05-30 20:25, Sterling Smith wrote: >> 1) git revert xyz >> 2) makes changes and sets v1.2 >> 3) commit >> 4) git push >> >> The last 4 lines were local, so it is ok to have the version changed >> temporarily back to pre-1.1 in step 1 (the git revert). > This is only true if the bad commit xyz was not pushed to the master branch > of the macports-ports repo. If it was pushed, then the port's epoch would > need to be changed per Ryan's comment. This is why in step 2 you set it to v1.2 (which was supposed to be the epoch equivalet in my example). Sorry, I should have listed the steps more clearly. I understand that you have to increment it, otherwise users won't see the update. -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */
Re: streamline github dev process
On May 30, 2017, at 8:03PM, Helmut K. C. Tessarek wrote: > On 2017-05-30 17:36, Ryan Schmidt wrote: >> Wouldn't requiring a certain number of reviews make things more >> difficult than they are now, where we don't require reviews? > > Well, this depends. I haven't administered github projects for large > groups, so I don't know if this is even possible with github. > > But I was thinking that new maintainers (with commit access), who still > need to build up a reputation, have to have their PRs reviewed. > Others can commit directly. It is possible to limit who can push to specific branches for a given repo in GitHub. It is possible to require a pull request to push to a given branch in GitHub. It is possible to require positive reviews before a pull request can be merged (or squashed or rebased). > >> It's not allowed to revert a commit that has been pushed. It is >> possible to push a new commit that undoes the changes of a previous >> commit. But MacPorts has additional requirements. For example, if you >> update a port from version 1.0 to version 2.0 and then find that 2.0 >> is broken, you cannot undo it by making another commit that just >> changes the port back to version 1.0 again; you must also increase >> the port's epoch. > > I was rather talking about the process and that it is very easy in git > to rollback commits. > > Let's say commit xyz (v1.1) was bad. > > The maintainer now does the following: > > 1) git revert xyz > 2) makes changes and sets v1.2 > 3) commit > 4) git push > > The last 4 lines were local, so it is ok to have the version changed > temporarily back to pre-1.1 in step 1 (the git revert). Helmut, This is only true if the bad commit xyz was not pushed to the master branch of the macports-ports repo. If it was pushed, then the port's epoch would need to be changed per Ryan's comment. -Sterling > > -- > regards Helmut K. C. Tessarek > lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D > > /* > Thou shalt not follow the NULL pointer for chaos and madness > await thee at its end. > */
Re: streamline github dev process
On 2017-05-30 17:36, Ryan Schmidt wrote: > Wouldn't requiring a certain number of reviews make things more > difficult than they are now, where we don't require reviews? Well, this depends. I haven't administered github projects for large groups, so I don't know if this is even possible with github. But I was thinking that new maintainers (with commit access), who still need to build up a reputation, have to have their PRs reviewed. Others can commit directly. > It's not allowed to revert a commit that has been pushed. It is > possible to push a new commit that undoes the changes of a previous > commit. But MacPorts has additional requirements. For example, if you > update a port from version 1.0 to version 2.0 and then find that 2.0 > is broken, you cannot undo it by making another commit that just > changes the port back to version 1.0 again; you must also increase > the port's epoch. I was rather talking about the process and that it is very easy in git to rollback commits. Let's say commit xyz (v1.1) was bad. The maintainer now does the following: 1) git revert xyz 2) makes changes and sets v1.2 3) commit 4) git push The last 4 lines were local, so it is ok to have the version changed temporarily back to pre-1.1 in step 1 (the git revert). -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */
Re: streamline github dev process
On May 30, 2017, at 16:36, Ryan Schmidt wrote: > On May 30, 2017, at 13:23, Helmut K. C. Tessarek wrote: > >> Maybe you could allow maintainers to review and approve other pull requests. >> >> This is git. It's very easy to revert a broken commit. Since almost all >> of these commits are not conflicting, it's even easier to do so. >> I believe that we need more comitters, even if you just allow >> maintainers to commit their own ports. > > It's not allowed to revert a commit that has been pushed. It is possible to > push a new commit that undoes the changes of a previous commit. But MacPorts > has additional requirements. For example, if you update a port from version > 1.0 to version 2.0 and then find that 2.0 is broken, you cannot undo it by > making another commit that just changes the port back to version 1.0 again; > you must also increase the port's epoch. Also, in regard to your last sentence there, historically, when we granted commit access to our CVS or Subversion repository, it was access to all of it. We've continued that practice under Git, partly since that's what we've always done, and partly because I'm not aware of a way to restrict committers to a subset of the repository, but that may just be due to my unfamiliarity with Git and GitHub. The Guide explains why we don't just grant commit access to maintainers the instant they become maintainers. Maintainers need time to build up their experience with nuances of MacPorts development. Until they've done that, we want to review their changes before they get published to users.
Re: streamline github dev process
> On May 30, 2017, at 13:23, Helmut K. C. Tessarek wrote: > > Hello, > > Maybe we can streamline the github process a bit. Sure. Those familiar with GitHub should chime in. I'm not familiar so I won't answer most of your questions, except: > As Mojca mentioned > earlier, the macports project is heavily understaffed. > > Is there a way to allow maintainers to set labels? (e.g. the > 'maintainer' or 'update' label) > > Also you could block merges until 1 or 2 people have reviewed and > approved the change. Wouldn't requiring a certain number of reviews make things more difficult than they are now, where we don't require reviews? > Maybe you could allow maintainers to review and approve other pull requests. > > This is git. It's very easy to revert a broken commit. Since almost all > of these commits are not conflicting, it's even easier to do so. > I believe that we need more comitters, even if you just allow > maintainers to commit their own ports. It's not allowed to revert a commit that has been pushed. It is possible to push a new commit that undoes the changes of a previous commit. But MacPorts has additional requirements. For example, if you update a port from version 1.0 to version 2.0 and then find that 2.0 is broken, you cannot undo it by making another commit that just changes the port back to version 1.0 again; you must also increase the port's epoch. > I do understand that a commit to this repo can affect a lot of people, > but on the other hand we need more traction and if a maintainer breaks > his port, he/she will have to fix it anyway. > > Does any of my suggestions make any sense?
streamline github dev process
Hello, Maybe we can streamline the github process a bit. As Mojca mentioned earlier, the macports project is heavily understaffed. Is there a way to allow maintainers to set labels? (e.g. the 'maintainer' or 'update' label) Also you could block merges until 1 or 2 people have reviewed and approved the change. Maybe you could allow maintainers to review and approve other pull requests. This is git. It's very easy to revert a broken commit. Since almost all of these commits are not conflicting, it's even easier to do so. I believe that we need more comitters, even if you just allow maintainers to commit their own ports. I do understand that a commit to this repo can affect a lot of people, but on the other hand we need more traction and if a maintainer breaks his port, he/she will have to fix it anyway. Does any of my suggestions make any sense? Cheers, K. C. -- regards Helmut K. C. Tessarek lookup http://pool.sks-keyservers.net for KeyID 0xC11F128D /* Thou shalt not follow the NULL pointer for chaos and madness await thee at its end. */