This page is no longer maintained — Please continue to the home page at www.scala-lang.org

Re: Workflow for failed pull requests

11 replies
daniel
Joined: 2008-08-20,
User offline. Last seen 44 weeks 14 hours ago.

Linear history is nice, but rebasing public history is a big no-no in terms of Git best practices.

I was talking about private branches in my example.

Ah…  Never mind then!  :-)

Daniel
daniel
Joined: 2008-08-20,
User offline. Last seen 44 weeks 14 hours ago.
Re: Workflow for failed pull requests

I don't agree with this. There is no rule that _any_ branch in _any_ public repository can't be rebased. They don't do that for the kernel either.

There are of course exceptions to these rules.  But in general, if you have a public branch, I think you need to leave its history alone.  Rebasing in public sometimes works without disruption, but you can never be really sure until you start hearing complaints on twitter.
 
It doesn't make sense to say that one has to keep their work private just so that they have the flexibility to rebase. It's important to make it clear to users that those branches can be rebased.

Actually, this is exactly what Linus advises.  If you want to rebase your work (squashing, reparenting, reordering, etc), keep that branch private.  As soon as you have shared the branch with anyone, either directly or by simply pushing the branch somewhere public, you have given up your right to rebase those commits.  If you think about it, this is really the only way to prevent disruption others due to munging of history.  The only thing you're really giving up here is the ability to clean up history after the fact (i.e. linearization).  This can be annoying, granted, but it's really not that bad.  Merge works really well, we shouldn't be afraid to use it!

Daniel
Ismael Juma 2
Joined: 2011-01-22,
User offline. Last seen 42 years 45 weeks ago.
Re: Workflow for failed pull requests
On Fri, Dec 2, 2011 at 6:04 PM, Daniel Spiewak <djspiewak [at] gmail [dot] com> wrote:

I don't agree with this. There is no rule that _any_ branch in _any_ public repository can't be rebased. They don't do that for the kernel either.

There are of course exceptions to these rules.  But in general, if you have a public branch, I think you need to leave its history alone.  Rebasing in public sometimes works without disruption, but you can never be really sure until you start hearing complaints on twitter.

Just name the branches appropriately: i_will_rebase_these/<branch_name>.  
Actually, this is exactly what Linus advises.

Just because Linus says it doesn't mean it's right. ;)
If you want to rebase your work (squashing, reparenting, reordering, etc), keep that branch private.  As soon as you have shared the branch with anyone, either directly or by simply pushing the branch somewhere public, you have given up your right to rebase those commits.  If you think about it, this is really the only way to prevent disruption others due to munging of history.

The logic is flawed. If people end up not publishing their branches because of the rule, it's a loss. I'd rather have access to the code even if it means that I have to deal with rebases. As an aside, I have experience working on a branch shared with one other person where rebases would happen and it was painless for the most part. I am, of course, not suggesting that this should be done in public branches where there are many people (some unknown) working on them.  
  The only thing you're really giving up here is the ability to clean up history after the fact (i.e. linearization).  This can be annoying, granted, but it's really not that bad.  Merge works really well, we shouldn't be afraid to use it!

It can be really bad actually. Some people like to have _lots_ of small commits at the start, for example. And those commits should definitely be squashed before merging them into a long-lived branch. But if I have a branch that is like that and I don't have the time to clean it up, it makes no sense to tell me that I can't put it somewhere public so others can look at it without losing the right to fix the history later.
Best,Ismael
daniel
Joined: 2008-08-20,
User offline. Last seen 44 weeks 14 hours ago.
Re: Workflow for failed pull requests
I guess this is just one of those things we'll have to figure out as a community.  I don't anticipate it being a serious source of friction.  Just prepare for a large napalm dump in the event that I base work on a branch that gets rebased out from under me!  ;-)

Daniel

On Fri, Dec 2, 2011 at 12:17 PM, Ismael Juma <ismael [at] juma [dot] me [dot] uk> wrote:
On Fri, Dec 2, 2011 at 6:04 PM, Daniel Spiewak <djspiewak [at] gmail [dot] com> wrote:

I don't agree with this. There is no rule that _any_ branch in _any_ public repository can't be rebased. They don't do that for the kernel either.

There are of course exceptions to these rules.  But in general, if you have a public branch, I think you need to leave its history alone.  Rebasing in public sometimes works without disruption, but you can never be really sure until you start hearing complaints on twitter.

Just name the branches appropriately: i_will_rebase_these/<branch_name>.  
Actually, this is exactly what Linus advises.

Just because Linus says it doesn't mean it's right. ;)
If you want to rebase your work (squashing, reparenting, reordering, etc), keep that branch private.  As soon as you have shared the branch with anyone, either directly or by simply pushing the branch somewhere public, you have given up your right to rebase those commits.  If you think about it, this is really the only way to prevent disruption others due to munging of history.

The logic is flawed. If people end up not publishing their branches because of the rule, it's a loss. I'd rather have access to the code even if it means that I have to deal with rebases. As an aside, I have experience working on a branch shared with one other person where rebases would happen and it was painless for the most part. I am, of course, not suggesting that this should be done in public branches where there are many people (some unknown) working on them.  
  The only thing you're really giving up here is the ability to clean up history after the fact (i.e. linearization).  This can be annoying, granted, but it's really not that bad.  Merge works really well, we shouldn't be afraid to use it!

It can be really bad actually. Some people like to have _lots_ of small commits at the start, for example. And those commits should definitely be squashed before merging them into a long-lived branch. But if I have a branch that is like that and I don't have the time to clean it up, it makes no sense to tell me that I can't put it somewhere public so others can look at it without losing the right to fix the history later.
Best,Ismael

dcsobral
Joined: 2009-04-23,
User offline. Last seen 38 weeks 5 days ago.
Re: Workflow for failed pull requests

On Fri, Dec 2, 2011 at 16:17, Ismael Juma wrote:
>> There are of course exceptions to these rules.  But in general, if you
>> have a public branch, I think you need to leave its history alone.  Rebasing
>> in public sometimes works without disruption, but you can never be really
>> sure until you start hearing complaints on twitter.
>
>
> Just name the branches appropriately: i_will_rebase_these/.
>
>>
>> Actually, this is exactly what Linus advises.
>
>
> Just because Linus says it doesn't mean it's right. ;)

Progit says it as well. Rebasing a public branch is poking the eyes
people who have forked it.

Stephen Haberman 2
Joined: 2011-07-23,
User offline. Last seen 42 years 45 weeks ago.
Re: Workflow for failed pull requests

> Progit says it as well. Rebasing a public branch is poking the eyes
> people who have forked it.

I initially replied just to (the other) Daniel since I'm a lurker, but
I'll note that the git project uses a 'pu'/proposed updates branch [1]
that is published to github and rebased frequently [2].

They also rebase their in-progress topic branches very often, but cheat
by using email attachments (not that I'm suggesting that) instead of
pull requests, which means you can share work without sharing commit
hashes.

So, I think all of the stern "don't rebase public branches" talk is so
that people that don't know any better mess things up, but in practice
the git community itself has found ways to work frequent rebasing of
in-progress work into their workflow.

I have no good suggestions of how to do that in github, given how
poorly their pull request feature deals with rebases. But just wanted
to point out that Ismael's notion of "these branches are safe to base
work on" vs. "these branches are not" does have precedence, and within
the git community at that.

- Stephen

[1]: https://github.com/git/git/tree/pu

[2]: http://stackoverflow.com/a/3537980/355031

Ismael Juma 2
Joined: 2011-01-22,
User offline. Last seen 42 years 45 weeks ago.
Re: Workflow for failed pull requests
On Fri, Dec 2, 2011 at 9:00 PM, Stephen Haberman <stephen [dot] haberman [at] gmail [dot] com> wrote:
I initially replied just to (the other) Daniel since I'm a lurker, but I'll note that the git project uses a 'pu'/proposed updates branch [1]
that is published to github and rebased frequently [2].

They also rebase their in-progress topic branches very often, but cheat
by using email attachments (not that I'm suggesting that) instead of
pull requests, which means you can share work without sharing commit
hashes.

So, I think all of the stern "don't rebase public branches" talk is so
that people that don't know any better mess things up, but in practice
the git community itself has found ways to work frequent rebasing of
in-progress work into their workflow.

Thanks Stephen. This is a great example. Thanks for mentioning it.
I have no good suggestions of how to do that in github, given how
poorly their pull request feature deals with rebases. But just wanted
to point out that Ismael's notion of "these branches are safe to base
work on" vs. "these branches are not" does have precedence, and within
the git community at that.

Indeed. And it's common sense too, in my opinion.
Best,Ismael
extempore
Joined: 2008-12-17,
User offline. Last seen 35 weeks 3 days ago.
Re: Workflow for failed pull requests

On Fri, Dec 2, 2011 at 10:17 AM, Ismael Juma wrote:
> Just name the branches appropriately: i_will_rebase_these/.

I proposed in the google doc somewhere that we establish a prefix for
branches you might rebase, like "unstable". Also, pull requests should
be in their own branch and in most cases be the only commit in that
branch, and it's hard to imagine there's an enormous amount of basing
work on peoples' recently submitted pull requests going on.

A pull request has to be subject to rebasing at some point or we will
bury ourselves in trivial commits. We can pick any way of handling
that which people like.

Blair Zajac
Joined: 2009-01-12,
User offline. Last seen 42 years 45 weeks ago.
Re: Workflow for failed pull requests

On 12/2/11 1:00 PM, Stephen Haberman wrote:
>
>> Progit says it as well. Rebasing a public branch is poking the eyes
>> people who have forked it.
>
> I initially replied just to (the other) Daniel since I'm a lurker, but
> I'll note that the git project uses a 'pu'/proposed updates branch [1]
> that is published to github and rebased frequently [2].
>
> They also rebase their in-progress topic branches very often, but cheat
> by using email attachments (not that I'm suggesting that) instead of
> pull requests, which means you can share work without sharing commit
> hashes.
>
> So, I think all of the stern "don't rebase public branches" talk is so
> that people that don't know any better mess things up, but in practice
> the git community itself has found ways to work frequent rebasing of
> in-progress work into their workflow.
>
> I have no good suggestions of how to do that in github, given how
> poorly their pull request feature deals with rebases. But just wanted
> to point out that Ismael's notion of "these branches are safe to base
> work on" vs. "these branches are not" does have precedence, and within
> the git community at that.

I've set up a Gerrit repository containing a clone of scala for people to test
with to see what it provides.

Before checking it out though, watch this 10 minute demo of Gerrit integrated
with Jenkins where the person submits a patch for review, Jenkins finds a bug,
then modifying the patch:

http://alblue.bandlem.com/2011/02/gerrit-git-review-with-jenkins-ci.html

The Scala Gerrit it at:

https://www.orcaware.com:18443/

I've submitted a documentation typo patch:

https://www.orcaware.com:18443/#change,1

I updated the patch to change the commit message, so there's two versions of the
patch.

If you want to try the workflow, I've set it up so any self-registered user has
all rights (let me know if you see something that isn't possible):

- Sign in using Google as an OpenID provider.
- Add your ssh public key so you can pull, just as you did with github.com.
- $ git clone ssh://www.orcaware.com:29418/scala
- $ cd scala
- Add a commit hook to put Commit-Ids in commit log messages [1]
- $ scp -p -P 29418 www.orcaware.com:hooks/commit-msg .git/hooks/
- Make any change
- $ git commit
- $ git push origin HEAD:refs/for/master
- Look at your change at https://www.orcaware.com:18443/

If you submit different commits that are unrelated from different local
branches, then they live in different lines of development in Gerrit and do not
impact each other.

Couple of other notes:

1) Gerrit provides user groups. You could put people that signed a CLA in a
group and give them push rights, other registered users would not get push
rights. This way you don't have to keep track of who signed a CLA.

2) Gerrit lets you do webdiff between submissions to the same Patch-Id [2].

3) Gerrit acts as a git repository itself and an ssh server. It's a single Java
process that's easy to manage.

4) Pushes go to a special ref, e.g. refs/for/master. After the merge, they
appear on the branch.

Blair

[1] http://gerrit.googlecode.com/svn/documentation/2.2.1/user-changeid.html
[2] https://www.orcaware.com:18443/#patch,unified,1,2,/COMMIT_MSG

gkossakowski
Joined: 2010-03-11,
User offline. Last seen 33 weeks 5 days ago.
Re: Workflow for failed pull requests
On 3 December 2011 01:24, Blair Zajac <blair [at] orcaware [dot] com> wrote:

I've set up a Gerrit repository containing a clone of scala for people to test with to see what it provides.

Before checking it out though, watch this 10 minute demo of Gerrit integrated with Jenkins where the person submits a patch for review, Jenkins finds a bug, then modifying the patch:

Hi Blair,
I'm very familiar with Gerrit and I like it personally but I don't think it's good fit especially since we moved to github. It would be very confusing that we host our repo on github and do everything else. Also, as you outlined, Gerrit introduces quite a bit of a process that users need to understand. I think Scala wants to benefit from Github's ease of use and Gerrit introduction would go into contrary direction.
Also, I found customization of Gerrit to be quite involved process.
--
Grzegorz Kossakowski

extempore
Joined: 2008-12-17,
User offline. Last seen 35 weeks 3 days ago.
Re: Workflow for failed pull requests

On Fri, Dec 2, 2011 at 4:24 PM, Blair Zajac wrote:
> I've set up a Gerrit repository containing a clone of scala for people to
> test with to see what it provides.

The effort is much appreciated. I can see that there is some appeal,
but I incline the same way grek does. Gerrit looks like a reasonably
heavy new commitment, and we have enough chaos in the air as it is.
It's something we can keep an eye on while we get used to our new git
center of gravity, but github and mostly typical github project
processes is good for now.

Blair Zajac
Joined: 2009-01-12,
User offline. Last seen 42 years 45 weeks ago.
Re: Workflow for failed pull requests

On Dec 6, 2011, at 7:59 AM, Paul Phillips wrote:

> On Fri, Dec 2, 2011 at 4:24 PM, Blair Zajac wrote:
>> I've set up a Gerrit repository containing a clone of scala for people to
>> test with to see what it provides.
>
> The effort is much appreciated. I can see that there is some appeal,
> but I incline the same way grek does. Gerrit looks like a reasonably
> heavy new commitment, and we have enough chaos in the air as it is.
> It's something we can keep an eye on while we get used to our new git
> center of gravity, but github and mostly typical github project
> processes is good for now.

Thanks for taking a look. I'll shut down the server in a day or two in case anybody else wants to take a look.

Blair

Copyright © 2012 École Polytechnique Fédérale de Lausanne (EPFL), Lausanne, Switzerland