Re: streamline github dev process

2017-05-31 Thread Mojca Miklavec
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

2017-05-31 Thread Helmut K. C. Tessarek
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

2017-05-31 Thread Joshua Root

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

2017-05-31 Thread Joshua Root

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

2017-05-31 Thread Helmut K. C. Tessarek
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

2017-05-31 Thread Helmut K. C. Tessarek
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

2017-05-31 Thread Daniel J. Luke
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

2017-05-31 Thread Rainer Müller
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

2017-05-31 Thread Zero King

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

2017-05-31 Thread Rainer Müller
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

2017-05-31 Thread Zero King

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

2017-05-31 Thread Craig Treleaven
> 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

2017-05-31 Thread Zero King

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

2017-05-31 Thread Rainer Müller
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

2017-05-31 Thread Mojca Miklavec
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

2017-05-30 Thread Jeremy Lavergne
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

2017-05-30 Thread Helmut K. C. Tessarek

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

2017-05-30 Thread Sterling Smith

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

2017-05-30 Thread Ryan Schmidt
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

2017-05-30 Thread Ryan Schmidt

> 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?