Bypassing the review process with branches which are not under review

[Matthias Günther]

Hi all

We have a workmode here that we have a branch which people can use to "backup" their changes or do whatever they want. To be more precise, it's all branches under namespace tmp/* where developers can push without a review.

Now we face the following problem. People create e.g. 3 commits on tmp/mybranch. After a while they want to push those changes to refs/for/master. Gerrit says "no new changes" since it knows all SHAs already.

Now I told the developers they should use a rebase which only makes sense here with "--force" to really get new SHAs (without --force the commits could just be fast-forwards).

Until now, no developer could work around the review system for our master branch, but with the following use case you can bypass the system:

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

So my question is, how would you change our workmode? Or in general I thought that a review is tied to the branch, isn't it? Or is there a way to not "track" certain branches like "tmp/*" ? Or is it "just" a bug?

I tried this with 2.5 and 2.6-rc3

Matthias

[Phil Hord]

Matthias,

Are you sure C was merged into 'master' immediately?

I think this is a bug related to bug#1142 (http://code.google.com/p/gerrit/issues/detail?id=1142) but it seems you have triggered a problem with the opposite workflow.

You can probably work around it by rewriting the Change-Id in the commit messages before pushing.  But I don't think there is a way to enforce that.  I had a script once that did this using 'git filter-branch --msg-filter' but I don't know where it went.  It would not be hard to recreate.

Phil

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en
 
---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Saša Živkov]

On Tue, Jun 18, 2013 at 4:37 PM, Matthias Günther <matgnt@gmail.com> wrote:

Hi all

We have a workmode here that we have a branch which people can use to "backup" their changes or do whatever they want. To be more precise, it's all branches under namespace tmp/* where developers can push without a review.

Now we face the following problem. People create e.g. 3 commits on tmp/mybranch. After a while they want to push those changes to refs/for/master. Gerrit says "no new changes" since it knows all SHAs already.

Now I told the developers they should use a rebase which only makes sense here with "--force" to really get new SHAs (without --force the commits could just be fast-forwards).

Until now, no developer could work around the review system for our master branch, but with the following use case you can bypass the system:

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

 

So my question is, how would you change our workmode?

Another possibility would be to create an additional project which would only be used for backups.

Then, your developers can create another remote called "backup" that would push all (or a subset)

of their local branches to that backup project. Each developer could get own branch namespace

into the backup project:

[remote "backup"]

  pushurl = url-of-the-backup-project

  push = refs/heads/*:refs/heads/zivkov/*

to backup my local branches I only need to:

$ git push backup

Saša

[1] https://gerrit-review.googlesource.com/Documentation/user-upload.html#base

 

Or in general I thought that a review is tied to the branch, isn't it? Or is there a way to not "track" certain branches like "tmp/*" ? Or is it "just" a bug?

I tried this with 2.5 and 2.6-rc3

Matthias

[Matthias Günther]

Hi Phil,

you're right, it's not immediately merged into master. The "DE" change (which is under review) shows the "C" commit as a parent. But "C" is NOT in master at this point. The "DE" change doesn't show any dependencies in the Web-UI. And the diff only shows the "DE" changes, but not the diff from the "C" commit.

As soon as I submit "DE" to master, also "C" is visible in master - without ever being reviewed.

The bug#1142 (http://code.google.com/p/gerrit/issues/detail?id=1142) you mentioned, seems to be an issue for us as well. I have to test this a bit more in detail.

Matthias

[Matthias Günther]

Hi Saša

On Wednesday, June 19, 2013 11:03:10 AM UTC+2, zivkov wrote:

On Tue, Jun 18, 2013 at 4:37 PM, Matthias Günther <matgnt@gmail.com> wrote:

Hi all

We have a workmode here that we have a branch which people can use to "backup" their changes or do whatever they want. To be more precise, it's all branches under namespace tmp/* where developers can push without a review.

Now we face the following problem. People create e.g. 3 commits on tmp/mybranch. After a while they want to push those changes to refs/for/master. Gerrit says "no new changes" since it knows all SHAs already.

Now I told the developers they should use a rebase which only makes sense here with "--force" to really get new SHAs (without --force the commits could just be fast-forwards).

Until now, no developer could work around the review system for our master branch, but with the following use case you can bypass the system:

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

Thanks for the hint to %base. I didn't know this. But actually it's not what I want to achieve here. I'd like to also see the "C" commit as a new open change in Gerrit.

But the documentation you're pointing to seems to be the root cause of my problem:

"By default new changes are opened only for new unique commits that have never before been seen by the Gerrit server"

Is this true? There is no association with a branch? I couldn't find a more detailed documentation about what a new change is and how Change-Id, SHA and branches are linked to each other in Gerrit.

 

 

So my question is, how would you change our workmode?

Another possibility would be to create an additional project which would only be used for backups.

Then, your developers can create another remote called "backup" that would push all (or a subset)

of their local branches to that backup project. Each developer could get own branch namespace

into the backup project:

[remote "backup"]

  pushurl = url-of-the-backup-project

  push = refs/heads/*:refs/heads/zivkov/*

to backup my local branches I only need to:

$ git push backup

Yes, this is another way to provide those temporary branch options for the developers. We decided to provide this in the same project to make it easier for the developers because we thought it's possible with the Gerrit rights management.

Matthias

 

[Saša Živkov]

On Wed, Jun 19, 2013 at 11:44 AM, Matthias Günther <mat...@gmail.com> wrote:

Hi Saša

On Wednesday, June 19, 2013 11:03:10 AM UTC+2, zivkov wrote:

On Tue, Jun 18, 2013 at 4:37 PM, Matthias Günther <matgnt@gmail.com> wrote:

Hi all

We have a workmode here that we have a branch which people can use to "backup" their changes or do whatever they want. To be more precise, it's all branches under namespace tmp/* where developers can push without a review.

Now we face the following problem. People create e.g. 3 commits on tmp/mybranch. After a while they want to push those changes to refs/for/master. Gerrit says "no new changes" since it knows all SHAs already.

Now I told the developers they should use a rebase which only makes sense here with "--force" to really get new SHAs (without --force the commits could just be fast-forwards).

Until now, no developer could work around the review system for our master branch, but with the following use case you can bypass the system:

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

Thanks for the hint to %base. I didn't know this. But actually it's not what I want to achieve here. I'd like to also see the "C" commit as a new open change in Gerrit.

Have you tried that?

If your HEAD points at DE and you do:

git push origin HEAD:refs/for/master%base=B

you should get new open changes for C and DE commits.

This is also what the commit message of 5d8a290d0622ce0469fb0abfdab182b5e100189f tells

https://gerrit-review.googlesource.com/44850

But the documentation you're pointing to seems to be the root cause of my problem:

"By default new changes are opened only for new unique commits that have never before been seen by the Gerrit server"

Is this true? There is no association with a branch? I couldn't find a more detailed documentation about what a new change is and how Change-Id, SHA and branches are linked to each other in Gerrit.

 

 

So my question is, how would you change our workmode?

Another possibility would be to create an additional project which would only be used for backups.

Then, your developers can create another remote called "backup" that would push all (or a subset)

of their local branches to that backup project. Each developer could get own branch namespace

into the backup project:

[remote "backup"]

  pushurl = url-of-the-backup-project

  push = refs/heads/*:refs/heads/zivkov/*

to backup my local branches I only need to:

$ git push backup

Yes, this is another way to provide those temporary branch options for the developers. We decided to provide this in the same project to make it easier for the developers because we thought it's possible with the Gerrit rights management.

Matthias

 

--

[Matthias Günther]

Hi Saša,

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

Thanks for the hint to %base. I didn't know this. But actually it's not what I want to achieve here. I'd like to also see the "C" commit as a new open change in Gerrit.

Have you tried that?

If your HEAD points at DE and you do:

git push origin HEAD:refs/for/master%base=B

you should get new open changes for C and DE commits.

This is also what the commit message of 5d8a290d0622ce0469fb0abfdab182b5e100189f tells

https://gerrit-review.googlesource.com/44850

You're right, it creates the 3 new changes I want. The problem is still that I can't enforce this. I can only hope the developers use this feature.

When I tried this new feature I figured out it seems having a bug. After submitting via the Web-UI the 3 changes are still in status "Submitted, Merge Pending", even though I can see the 3 commits on master already. (2.7-rc1)

I'll file a bug for this.

In general I think my initial problem could be solved with an option per project or globally, where I can configure namespaces (like "tmp/*") not to be "monitored" by Gerrit.

At the end I have 2 options

- trust the developers to not bypass the review (use a rebase --force for now (2.6) or the above mentioned %base option later (with 2.7))

- don't allow the tmp/* namespace for backups and rather create separate projects (which is a bit more effort for me to maintain + more effort for the developers to integrate into their local projects)

Thanks for your help

Matthias

[Saša Živkov]

On Fri, Jun 21, 2013 at 10:23 AM, Matthias Günther <mat...@gmail.com> wrote:

Hi Saša,

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

Thanks for the hint to %base. I didn't know this. But actually it's not what I want to achieve here. I'd like to also see the "C" commit as a new open change in Gerrit.

Have you tried that?

If your HEAD points at DE and you do:

git push origin HEAD:refs/for/master%base=B

you should get new open changes for C and DE commits.

This is also what the commit message of 5d8a290d0622ce0469fb0abfdab182b5e100189f tells

https://gerrit-review.googlesource.com/44850

You're right, it creates the 3 new changes I want. The problem is still that I can't enforce this. I can only hope the developers use this feature.

Like every other feature ;-)

You can't even enforce they use refs/for/master  

When I tried this new feature I figured out it seems having a bug. After submitting via the Web-UI the 3 changes are still in status "Submitted, Merge Pending", even though I can see the 3 commits on master already. (2.7-rc1)

I'll file a bug for this.

 

This sounds like a bug.

 

In general I think my initial problem could be solved with an option per project or globally, where I can configure namespaces (like "tmp/*") not to be "monitored" by Gerrit.

At the end I have 2 options

- trust the developers to not bypass the review (use a rebase --force for now (2.6) or the above mentioned %base option later (with 2.7))

- don't allow the tmp/* namespace for backups and rather create separate projects (which is a bit more effort for me to maintain + more effort for the developers to integrate into their local projects)

Thanks for your help

Matthias

[Saša Živkov]

On Fri, Jun 21, 2013 at 11:22 AM, Saša Živkov <ziv...@gmail.com> wrote:

On Fri, Jun 21, 2013 at 10:23 AM, Matthias Günther <mat...@gmail.com> wrote:

Hi Saša,

Let's assume we have the 3 mentioned commits on tmp/mybranch and do a "rebase -i" to squash the latest 2 commits. When you push this to refs/for/master now, the first commit on "tmp/mybranch" is just a fast-forward commit which now slips into master without being reviewed.

A-B        master

   \

    C-D-E  tmp/mybranch

rebase -i origin/master with squashing D+E

A-B-C-'DE'

C doesn't get a review!

 

The official way to achieve what you want is to use the %base argument for the target

branch when pushing for review, see [1].

In this case you would need to specify the B as the base:

$ git push origin HEAD:refs/for/master%base=B

Thanks for the hint to %base. I didn't know this. But actually it's not what I want to achieve here. I'd like to also see the "C" commit as a new open change in Gerrit.

Have you tried that?

If your HEAD points at DE and you do:

git push origin HEAD:refs/for/master%base=B

you should get new open changes for C and DE commits.

This is also what the commit message of 5d8a290d0622ce0469fb0abfdab182b5e100189f tells

https://gerrit-review.googlesource.com/44850

You're right, it creates the 3 new changes I want. The problem is still that I can't enforce this. I can only hope the developers use this feature.

Like every other feature ;-)

You can't even enforce they use refs/for/master  

Sorry, sent too early... and I wanted to write it in a different way:

in general, we cannot enforce that tools are used the right way.

But, I agree, it is difficult to remember to use the %base option if something was pushed to tmp/* before.

 

When I tried this new feature I figured out it seems having a bug. After submitting via the Web-UI the 3 changes are still in status "Submitted, Merge Pending", even though I can see the 3 commits on master already. (2.7-rc1)

I'll file a bug for this.

 

This sounds like a bug.

 

In general I think my initial problem could be solved with an option per project or globally, where I can configure namespaces (like "tmp/*") not to be "monitored" by Gerrit.

At the end I have 2 options

- trust the developers to not bypass the review (use a rebase --force for now (2.6) or the above mentioned %base option later (with 2.7))

- don't allow the tmp/* namespace for backups and rather create separate projects (which is a bit more effort for me to maintain + more effort for the developers to integrate into their local projects)

 

I am not really keen on using this approach but the effort is really one time effort for everyone. 

[Phil Hord]

I disagree. Gerrit _should_ enforce that C does not get merged to
master without review here since the developer pushing the change does
not have "Push" access to "refs/heads/master". That is, if the
developer "forgets" to use %base=B, Gerrit should reject the push as
not authorized.

This is a long-standing bug.

Phil