Hi everyone,
This is something I've been meaning to bring up for some time, but have just been delaying getting it done. For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit. This doesn't affect other branches, just master. There's a couple of big reasons this is a problem right now, and why we need to change:
1) Eventually, we'll be running tests for all extensions (at the very least linting if no phpunit tests have been written). Jenkins doesn't have the change to -1 a commit if you skip review. 2) It doesn't give anyone a place to complain about the patch. Every commit to master needs a place to say "Hey wait a minute" -- even if it's already been merged. 3) Changes that are directly pushed aren't searchable from Gerrit. This is more a feature request for Gerrit, but one that's easily worked around by just pushing through Gerrit.
I realize that feature branches don't necessarily need the same level of scrutiny, but the "primary" branch for a repository needs to be as public as possible--direct pushing makes this difficult. I don't plan on changing this requirement for any branch that's not "master" or "wmf/" (the latter relating to deployment config). Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
Thanks for any feedback you can give.
-Chad
Am 29.08.2012 21:58, schrieb Chad:
Hi everyone,
This is something I've been meaning to bring up for some time, but have just been delaying getting it done. For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit....
soooo much information. Life was much easier before the big switch-over from svn to git, and gerrit in spring 2012.
Chad,
how will this affect my workflow? I suspect it'll complicate it further and make me do some other funky things before I can submit a change. Right now this is how I work with "my" extensions:
(0. clone the git repo) 1. edit the code 2. commit the changes 3. push the committed changeset/patchset/whatever you want to call it
If it matters, I'm on Windows (7 for the time being) using TortoiseGit.
On Wed, Aug 29, 2012 at 11:06 PM, Thomas Gries mail@tgries.de wrote:
soooo much information. Life was much easier before the big switch-over from svn to git, and gerrit in spring 2012.
I do miss the time when things Just Worked™ and I could focus on coding instead of messing around with the VCS. (And if this sounds personal to any of you, it's not; I just very much dislike git, gerrit and whatever our current workflow for core MW + WMF-deployed extensions is. I hardly have any motivation for committing code to core MW and for that, the setup is to be "thanked".)
Thanks and regards, -- Jack Phoenix MediaWiki developer
On Wed, Aug 29, 2012 at 12:58 PM, Chad innocentkiller@gmail.com wrote:
Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
As long as people that were previously able to push are still able to +2 (which is probably true, unless the ACLs are really weird), and as long as self-review isn't prevented by the software in the future (there was some discussion about this at some point), I think this'll be fine, because extension maintainers that used to use direct push can submit their change for review, then approve it themselves.
I maintain that self-review is evil for core and deployed extensions, but for undeployed extensions that don't have multiple active maintainers I think it's fine.
Also, didn't Shawn say there were vague plans to make direct pushes actually create changes and stuff? If and when that happens, I'd like to open direct push back up, because really this self-review workflow is a workaround for the fact that Gerrit doesn't handle direct pushes very well.
Roan
Hey,
I see several workflow issues I will have if this was applied to all extensions:
* Creating tags and pushing them - can this be done using gitreview? * Someone makes a pile of commits on some alternate remote or just locally and then wants to push them to the wmf hosted repo. If the stuff has already been reviewed, having to manually self-approve all of them is just ridiculous. * What if you merge a branch of which the commits have already been reviewed into master and then want to push that? Same problem as above?
In any case, I'm mostly using gitreview for my changes so they show up in gerrit, but every so often I run into a usecase where it really makes no sense to use it. I don't see what I would gain by preventing me from doing that for my own extensions, so think extensions owners definitely should keep the right for their own extensions. At least for non-WMF extensions that is.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Wed, Aug 29, 2012 at 4:44 PM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
I see several workflow issues I will have if this was applied to all extensions:
- Creating tags and pushing them - can this be done using gitreview?
This is still doable via normal git commands. Nothing there changes.
- Someone makes a pile of commits on some alternate remote or just locally
and then wants to push them to the wmf hosted repo. If the stuff has already been reviewed, having to manually self-approve all of them is just ridiculous.
- What if you merge a branch of which the commits have already been
reviewed into master and then want to push that? Same problem as above?
Only if they're pushing to master. It would make sense to have done the work on the branch, merge it to master, and then just push the merge commit for review.
-Chad
Hey,
Only if they're pushing to master. It would make sense to have done
the work on the branch, merge it to master, and then just push the merge commit for review.
Forgive my git ignorance here, but will merging this "merge commit" result into all original commits being in the version history, or will it just show as one huge change? In case of the later, I don't think this is an acceptable approach.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Wed, Aug 29, 2012 at 1:55 PM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Forgive my git ignorance here, but will merging this "merge commit" result into all original commits being in the version history, or will it just show as one huge change? In case of the later, I don't think this is an acceptable approach.
It will still show up in the history as individual commits on the right-hand side of the merge commit, same as when you do 'git checkout master; git merge mybranch; git push'.
Roan
On 29/08/12 22:44, Jeroen De Dauw wrote:
I don't see what I would gain by preventing me from doing that for my own extensions, so think extensions owners definitely should keep the right for their own extensions. At least for non-WMF extensions that is.
It's a workaround for gerrit being dumb and not processing the pushed changesets.
Would git push of already-sent changesets still accept them, or should I look for the ssh commands for accepting?
On Wed, Aug 29, 2012 at 7:55 PM, Platonides Platonides@gmail.com wrote:
On 29/08/12 22:44, Jeroen De Dauw wrote:
I don't see what I would gain by preventing me from doing that for my own extensions, so think extensions owners definitely should keep the right for their own extensions. At least for non-WMF extensions that is.
It's a workaround for gerrit being dumb and not processing the pushed changesets.
Would git push of already-sent changesets still accept them, or should I look for the ssh commands for accepting?
I think that depends where it was already sent too. If already in master then you should be able to push with no problem. If just to some other branch then a push to master would probably make a new changeset.
-Jeremy
On Wednesday, August 29, 2012 at 12:58 PM, Chad wrote:
For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit […] Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
E3 is using Phabricator for code-reviews, so this would complicate things for us. We could, I guess, have a separate Gerrit / +2 step. I'm curious, though: why is the scope for this proposal limited to extensions? Are other types of projects exempt?
-- Ori
For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit […] Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
E3 is using Phabricator for code-reviews, so this would complicate things for us. We could, I guess, have a separate Gerrit / +2 step. I'm curious, though: why is the scope for this proposal limited to extensions? Are other types of projects exempt?
I think most projects already have it disabled.
- Ryan
On Wed, Aug 29, 2012 at 10:12 PM, Ryan Lane rlane32@gmail.com wrote:
For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit […] Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
E3 is using Phabricator for code-reviews, so this would complicate things for us. We could, I guess, have a separate Gerrit / +2 step. I'm curious, though: why is the scope for this proposal limited to extensions? Are other types of projects exempt?
I think most projects already have it disabled.
Indeed. Most extensions do as well.
-Chad
Am 29.08.2012 21:58, schrieb Chad:
Hi everyone,
This is something I've been meaning to bring up for some time, but have just been delaying getting it done. For a bunch of reasons, we need to look at disabling direct pushing on the master branch for all extensions in Gerrit. This doesn't affect other branches, just master. There's a couple of big reasons this is a problem right now, and why we need to change:
- Eventually, we'll be running tests for all extensions (at the very least
linting if no phpunit tests have been written). Jenkins doesn't have the change to -1 a commit if you skip review. 2) It doesn't give anyone a place to complain about the patch. Every commit to master needs a place to say "Hey wait a minute" -- even if it's already been merged. 3) Changes that are directly pushed aren't searchable from Gerrit. This is more a feature request for Gerrit, but one that's easily worked around by just pushing through Gerrit.
I realize that feature branches don't necessarily need the same level of scrutiny, but the "primary" branch for a repository needs to be as public as possible--direct pushing makes this difficult. I don't plan on changing this requirement for any branch that's not "master" or "wmf/" (the latter relating to deployment config). Before I make the change though, I wanted to ask about it publicly to make sure there's no major blockers to me doing so.
Thanks for any feedback you can give.
-Chad
Just a note from point of Translatewiki.net staff:
All pushes to Git bypassing Gerrit are lost for Translatewiki.net. I read every merged patchset but and decide what to do in case of changes to i18n files (importing, syncing, fuzzying, mass replaces, moves etc).
Raimond
wikitech-l@lists.wikimedia.org