MozBase check-in policy

[Henrik Skupin]

Hi all,

Given our current docs I'm missing some information:
https://wiki.mozilla.org/Auto-tools/Projects/MozBase

1. Do we require people to rebase their patches first before merging
them to master?

I feel that this is pretty important whenever it comes to regression
tests. It makes it obvious kinda hard to track down issues when there
are interim patches for a feature in the commit list which could be
broken and will not allow us to test this specific changeset, e.g.
https://github.com/mozilla/mozbase/pull/11/files

2. When can a check-in happen and who is responsible for it?

Do we require a final review before code can be checked-in? Which means
that there was a review comment and the patch had to be updated
slightly. Can the new version of the patch be checked-in right away by
the author? Or do we want another review for safety protection? Also
should code check-ins always happen by other people or has the author to
check-in the patch him/herself?

Thanks,

--
Henrik Skupin
Software Engineer in Test
Mozilla Corporation

[David Burns]

On 09/05/2012 09:22, Henrik Skupin wrote:
> 1. Do we require people to rebase their patches first before merging
> them to master?

Rebasing is very uncommon and can make things disappear. Since a rebase
can delete commits, which is a very bad idea I am giving rebasing a -1.
Even the Github help says its bad practise http://help.github.com/rebase/

> 2. When can a check-in happen and who is responsible for it?
>
> Do we require a final review before code can be checked-in? Which means
> that there was a review comment and the patch had to be updated
> slightly. Can the new version of the patch be checked-in right away by
> the author? Or do we want another review for safety protection? Also
> should code check-ins always happen by other people or has the author to
> check-in the patch him/herself?
>
>

If someone has given it a r+, and its from a module owner, then my
opinion is that people should be allowed to commit it. In the pull
request you showed I went against my better judgement to rename the
variable which is where the issue came in.

David

--
David Burns
Email: dbu...@mozilla.com
Twitter: automatedtester

[Henrik Skupin]

David Burns wrote on 5/9/12 12:50 PM:

>> 1. Do we require people to rebase their patches first before merging
>> them to master?
> Rebasing is very uncommon and can make things disappear. Since a rebase
> can delete commits, which is a very bad idea I am giving rebasing a -1.
> Even the Github help says its bad practise http://help.github.com/rebase/

I don't propose to use rebase for public branches but for your own
patches, which most likely no-one else will use as base for other work
until they have been landed in the official repository. That's the way
how we have agreed on after a longish discussion in the past for the
Mozmill check-in policy:

https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_to_Push

I for myself don't like if interim patches gets landed where I have code
in which will definitely break pieces of code, e.g. changing a feature
and doing a commit to save the work before updating all the tests. Same
for review comments and follow-up fixes, which will cause a lot of
unnecessary commits to exist in the official repository.

>> 2. When can a check-in happen and who is responsible for it?
>

> If someone has given it a r+, and its from a module owner, then my
> opinion is that people should be allowed to commit it. In the pull
> request you showed I went against my better judgement to rename the
> variable which is where the issue came in.

Just to prevent those accidents and possible bug fix releases, which
always take time to do, it would be simple for someone else to quickly
glance over a commit and give the ok. I learned from the past and my
mistakes, that doing things quickly just to get them out is a huge
factor of introducing brokenness. That's simply the reason why I brought
this up.

Cheers,

[David Burns]

On 09/05/2012 13:48, Henrik Skupin wrote:
> David Burns wrote on 5/9/12 12:50 PM:
>
>>> 1. Do we require people to rebase their patches first before merging
>>> them to master?
>> Rebasing is very uncommon and can make things disappear. Since a rebase
>> can delete commits, which is a very bad idea I am giving rebasing a -1.
>> Even the Github help says its bad practise http://help.github.com/rebase/
> I don't propose to use rebase for public branches but for your own
> patches, which most likely no-one else will use as base for other work
> until they have been landed in the official repository. That's the way
> how we have agreed on after a longish discussion in the past for the
> Mozmill check-in policy:
>
> https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_to_Push
>
> I for myself don't like if interim patches gets landed where I have code
> in which will definitely break pieces of code, e.g. changing a feature
> and doing a commit to save the work before updating all the tests. Same
> for review comments and follow-up fixes, which will cause a lot of
> unnecessary commits to exist in the official repository.

But this leads to having to review the patch again after it has been
rebased because we could have deleted something by accident. The extra
work, coupled with it being bad practise and very uncommon, really puts
me off the idea.

[William Lachance]

On 12-05-09 04:22 AM, Henrik Skupin wrote:
> Hi all,
>
> Given our current docs I'm missing some information:
> https://wiki.mozilla.org/Auto-tools/Projects/MozBase
>
> 1. Do we require people to rebase their patches first before merging
> them to master?
>
> I feel that this is pretty important whenever it comes to regression
> tests. It makes it obvious kinda hard to track down issues when there
> are interim patches for a feature in the commit list which could be
> broken and will not allow us to test this specific changeset, e.g.
> https://github.com/mozilla/mozbase/pull/11/files

We don't have any policy on this but I think it's a good idea.

Rebasing commits that have already been pushed to the main mozbase
repository (http://github.com/mozilla/mozbase) is obviously a big no-no
as it can cause data loss, but I think it's a good practice for people
to clean things up in their own branches/trees before committing, both
to make finding regressions easier as well as to make the commit log
make more sense.

I know not everyone is comfortable with git rebase. If they aren't, they
should ask for help. ;) It's really not as scary as some pundits have
made it out to be.

> 2. When can a check-in happen and who is responsible for it?
>
> Do we require a final review before code can be checked-in? Which means
> that there was a review comment and the patch had to be updated
> slightly. Can the new version of the patch be checked-in right away by
> the author? Or do we want another review for safety protection? Also
> should code check-ins always happen by other people or has the author to
> check-in the patch him/herself?

The formal review policy is here:

https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Review_Policy

With regards to your specific question, I think the answer is (as it is
in mozilla-central) "use your common sense". This generally means:

1. Very minor fixes/cleanups to an already reviewed patch can go in
without re-review.
2. In general, we accept that occasionally mistakes will be made, and
are a cost of our rapid pace of development. Accidents can and will
happen, but hopefully we learn from them.

Will

[Carl Meyer]

On 05/09/2012 08:19 AM, William Lachance wrote:
> On 12-05-09 04:22 AM, Henrik Skupin wrote:

>> Hi all,
>>
>> Given our current docs I'm missing some information:
>> https://wiki.mozilla.org/Auto-tools/Projects/MozBase
>>
>> 1. Do we require people to rebase their patches first before merging
>> them to master?
>>
>> I feel that this is pretty important whenever it comes to regression
>> tests. It makes it obvious kinda hard to track down issues when there
>> are interim patches for a feature in the commit list which could be
>> broken and will not allow us to test this specific changeset, e.g.
>> https://github.com/mozilla/mozbase/pull/11/files
>

> We don't have any policy on this but I think it's a good idea.
>
> Rebasing commits that have already been pushed to the main mozbase
> repository (http://github.com/mozilla/mozbase) is obviously a big no-no
> as it can cause data loss, but I think it's a good practice for people
> to clean things up in their own branches/trees before committing, both
> to make finding regressions easier as well as to make the commit log
> make more sense.
>
> I know not everyone is comfortable with git rebase. If they aren't, they
> should ask for help. ;) It's really not as scary as some pundits have
> made it out to be.

Rebase doesn't need to be scary, but if the goal is to achieve a clean
mainline history by squashing branch commits, it is kind of overkill.
You can do it simply by merging in your feature branch using the
--squash option to the merge command ("git co master; git merge --squash
myfeature"). You still don't get the direct parent link from the final
mainline commit to the branch it came from (though you can note the
source branch in the commit message), but you lose less history overall
than rebasing does; unlike rebase, a squashed merge is not a destructive
operation, and there's less opportunity for things to go wrong.

FWIW,

Carl

[David Burns]

Learnt something new! I am happy to do that.

Fwiw, I don't find rebasing scary, just a bit overkill, as Carl
mentioned, and prone to human error. If we did a squash and pushed to a
remote server we have to force the update on the remote. That part, if
there has been an error means the person writing the patch has to start
again because they have lost history. Like I said, not scary but know
how error prone humans are.

David

On 09/05/2012 15:47, Carl Meyer wrote:
> On 05/09/2012 08:19 AM, William Lachance wrote:
>> On 12-05-09 04:22 AM, Henrik Skupin wrote:

>>> Hi all,
>>>
>>> Given our current docs I'm missing some information:
>>> https://wiki.mozilla.org/Auto-tools/Projects/MozBase
>>>
>>> 1. Do we require people to rebase their patches first before merging
>>> them to master?
>>>
>>> I feel that this is pretty important whenever it comes to regression
>>> tests. It makes it obvious kinda hard to track down issues when there
>>> are interim patches for a feature in the commit list which could be
>>> broken and will not allow us to test this specific changeset, e.g.
>>> https://github.com/mozilla/mozbase/pull/11/files
>>

>> We don't have any policy on this but I think it's a good idea.
>>
>> Rebasing commits that have already been pushed to the main mozbase
>> repository (http://github.com/mozilla/mozbase) is obviously a big no-no
>> as it can cause data loss, but I think it's a good practice for people
>> to clean things up in their own branches/trees before committing, both
>> to make finding regressions easier as well as to make the commit log
>> make more sense.
>>
>> I know not everyone is comfortable with git rebase. If they aren't, they
>> should ask for help. ;) It's really not as scary as some pundits have
>> made it out to be.
>
> Rebase doesn't need to be scary, but if the goal is to achieve a clean
> mainline history by squashing branch commits, it is kind of overkill.
> You can do it simply by merging in your feature branch using the
> --squash option to the merge command ("git co master; git merge
> --squash myfeature"). You still don't get the direct parent link from
> the final mainline commit to the branch it came from (though you can
> note the source branch in the commit message), but you lose less
> history overall than rebasing does; unlike rebase, a squashed merge is
> not a destructive operation, and there's less opportunity for things
> to go wrong.
>
> FWIW,
>
> Carl

> _______________________________________________
> tools mailing list
> to...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/tools

[Jeff Hammel]

On 05/09/2012 07:47 AM, Carl Meyer wrote:
> On 05/09/2012 08:19 AM, William Lachance wrote:
>> On 12-05-09 04:22 AM, Henrik Skupin wrote:

>>> Hi all,
>>>
>>> Given our current docs I'm missing some information:
>>> https://wiki.mozilla.org/Auto-tools/Projects/MozBase
>>>
>>> 1. Do we require people to rebase their patches first before merging
>>> them to master?
>>>
>>> I feel that this is pretty important whenever it comes to regression
>>> tests. It makes it obvious kinda hard to track down issues when there
>>> are interim patches for a feature in the commit list which could be
>>> broken and will not allow us to test this specific changeset, e.g.
>>> https://github.com/mozilla/mozbase/pull/11/files
>>

>> We don't have any policy on this but I think it's a good idea.
>>
>> Rebasing commits that have already been pushed to the main mozbase
>> repository (http://github.com/mozilla/mozbase) is obviously a big no-no
>> as it can cause data loss, but I think it's a good practice for people
>> to clean things up in their own branches/trees before committing, both
>> to make finding regressions easier as well as to make the commit log
>> make more sense.
>>
>> I know not everyone is comfortable with git rebase. If they aren't, they
>> should ask for help. ;) It's really not as scary as some pundits have
>> made it out to be.
>
> Rebase doesn't need to be scary, but if the goal is to achieve a clean
> mainline history by squashing branch commits, it is kind of overkill.
> You can do it simply by merging in your feature branch using the
> --squash option to the merge command ("git co master; git merge
> --squash myfeature"). You still don't get the direct parent link from
> the final mainline commit to the branch it came from (though you can
> note the source branch in the commit message), but you lose less
> history overall than rebasing does; unlike rebase, a squashed merge is
> not a destructive operation, and there's less opportunity for things
> to go wrong.
>
> FWIW,
>
> Carl
> _______________________________________________
>

Personally, I am not super-convinced that a checkin policy is what
mozbase needs the most of now. That said, I am for a checkin policy if
it is:
1) Easy to do: there shouldn't be a list of instructions I have to cut
and paste or similar. Preferably, this would be one line.
2) Documented. I'm generally of the opinion that policy should be
documented and obvious. I and many other people are turned off by being
yelled at for violating policy that we didn't know existed
A bonus point would be:
*) Fits multiple ways of working. I've often unbitrotted or otherwise
used patches because it was easier than trying to figure out "the right
way of doing things". While I wouldn't necessarily defend this as good
development practice, the end result is a single commit with a pointer
to the bug.

And as long as we're on the subject, I would request that, if possible,
commit messages are in the form:

"Bug 123456: the title of the bug (if applicable);r=whoever"

I don't really care about capitalization or formatting or other nits
(e.g. if there are multiple commits per bug, obviously itd be nice to
differentiate things), but it is nice to be able to find bugs from
commits in the main repo.

[Carl Meyer]

On 05/09/2012 09:01 AM, Henrik Skupin wrote:
> Carl Meyer wrote on 5/9/12 4:47 PM:

>
>> You can do it simply by merging in your feature branch using the
>> --squash option to the merge command ("git co master; git merge --squash
>> myfeature"). You still don't get the direct parent link from the final
>

> Interesting. That's also news for me. Thanks for the hint.
>
> Given that we merge code more and more via the Github website, how could
> we enforce this? Sadly it's not supported by the automatic merge process
> on Github, so everyone would have to merge the branch locally? That
> would mean more hassle when landing patches for other people. Or is
> there an option I also miss?

It's true that having someone rebase their branch down to one commit
allows using the merge button on github, whereas merge --squash requires
the person merging to do it locally. Generally I don't find that to be a
problem in practice, since presumably the person doing the merge will
run the tests on the new branch first, which already requires them to
have it locally anyway (IOW I find the github merge button to be an
anti-pattern for all except simple docs typo fixes) - but maybe
mozmill/mozbase have other review/test policies that cover this a
different way.

Carl

[David Burns]

On 09/05/2012 16:08, Carl Meyer wrote:
> On 05/09/2012 09:01 AM, Henrik Skupin wrote:
>> Carl Meyer wrote on 5/9/12 4:47 PM:
>>
>>
>

> (IOW I find the github merge button to be an anti-pattern for all
> except simple docs typo fixes)
>

+1

In WebQA it was banned from being used when I was in there so that if
another person did the merge they ran the tests.

David

[Henrik Skupin]

David Burns wrote on 5/9/12 8:34 PM:

>> (IOW I find the github merge button to be an anti-pattern for all
>> except simple docs typo fixes)
>>
> +1
>
> In WebQA it was banned from being used when I was in there so that if
> another person did the merge they ran the tests.

Sounds like we should really stop in using this button and go back to
the roots by merging manually and testing the patch before check-in.
That way we could really ensure to merge via --squash.

How do others think about it? Is it something we could consider as a
check-in policy?

[Jeff Hammel]

On 05/09/2012 12:46 PM, Henrik Skupin wrote:
> David Burns wrote on 5/9/12 8:34 PM:
>
>>> (IOW I find the github merge button to be an anti-pattern for all
>>> except simple docs typo fixes)
>>>
>> +1
>>
>> In WebQA it was banned from being used when I was in there so that if
>> another person did the merge they ran the tests.
> Sounds like we should really stop in using this button and go back to
> the roots by merging manually and testing the patch before check-in.
> That way we could really ensure to merge via --squash.
>
> How do others think about it? Is it something we could consider as a
> check-in policy?
>

Personally, I'd like not to use pull requests for anything (either
pulling or merging). Unless we switch to using github exclusively for
issue tracking (which again, I'd personally dislike), it is at best
confusing/annoying to have comments in multiple "threads": one on
bugzilla and one on github.

Short of that, I'm fine with not using the magical-merge button. Again,
if we want this as policy it should be noted in the wiki.

[Henrik Skupin]

Jeff Hammel wrote on 5/9/12 11:00 PM:

> Personally, I'd like not to use pull requests for anything (either
> pulling or merging). Unless we switch to using github exclusively for
> issue tracking (which again, I'd personally dislike), it is at best
> confusing/annoying to have comments in multiple "threads": one on
> bugzilla and one on github.

Bugzilla has such an annoying and broken interdiff feature which is
broken for me most of the time. Tracking different revisions of a patch
makes it nearly impossible to follow the changes. Seeing the individual
commits on Github with the real code changes and the syntax highlighting
is wonderful for reviewing code.

One example would be the patches on:
https://bugzilla.mozilla.org/show_bug.cgi?id=586286

One idea would be to use the bugs for tracking individual features and
to collect all necessary requirements. Reviews could then happen on
Github with a final review on Bugzilla.

> Short of that, I'm fine with not using the magical-merge button. Again,
> if we want this as policy it should be noted in the wiki.

I will do that once we all are in a consensus.

[hsk...@gmail.com]

On Wednesday, May 9, 2012 10:22:31 AM UTC+2, Henrik Skupin wrote:

Given the latest discussions I have landed a version of the checkin policy on the wiki:

https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Check-in_policy

Please read through and tell me if something is missing or should be changed/updated.

Thanks!

[hsk...@gmail.com]