Gerrit workflow problems.

[Lazo Apostolovski]

Hi there.

We try to use Gerrit with code review but its a nightmare for us. I read a lot on this mail list for basic, standard, save workflow with gerrit but I still have problems. Something that I still cant catch.

Case 1:

like Ted say in: https://groups.google.com/forum/#!searchin/repo-discuss/Gerrit$20workflow/repo-discuss/h3AKGwHDuZ8/_Q-XLRLmT8cJ

- lets assume we have a feature that's medium size, lets assume that the feature has been decomposed into 10 smaller pieces of work.

- A developer then does the 10 pieces of work, and results in 10 separate commits.

- Lets assume on commit #1, I find some small issues, like a spelling mistake, and maybe some one used System.err.println instead of a logger. Lets assume generally speaking everything else is fine, and everything is fine with commits #2-10.

Possible solution:

Based on what I've read, the developer can call git rebase -i and to select edit on #1 commit. After changes are done, git commit --amend command can be used on that commit. Also for commit message we place copied Change ID: from gerrit site. After that git rebase --continue is executed, and rebase continues so on.

The problem here is that commits #2-10 have new Change id, and when i call git push origin refs/for/master i going to have duplicate 10 commits for code review.

What I do wrong here? Do I need manual abandon all old commits for review? (What about we have 10 developers with 10 commits each day?)

Case 2:

- A developer have 3 commits for code review. 

- before that commits is reviewed, another patch is verified and is merged in commit

- when i try to merge #1 change from first developer, i get message that there is conflicts and merge need to be done manual

- developer type git rebase -i refs/changes/origin/master. Resolve conflicts and continue rebasing with git rebase --continue

after that he push his changes on gerrit for review. In this case also we receive duplicated commits in gerrit code review.

Case 3:

Assume the following history exists:

                     A---B---C master

                    /

               D---E---F---G origin/master

From this point, i just run git rebase -i refs/remotes/origin/master and end with this.

                                A'--B'--C' master

                               /

               D---E---F---G origin/master

So if i have A, B, C on gerrit for code review, and rebase it and push it again I going to have A, B, C, A', B', C' for code review on gerrit.

I have no others idea what to do.

Please describe to me, any possible, safe, clean way how any gerrit workflow can function.

Thank you.

[Martin Fick]

On Friday, January 13, 2012 07:26:02 am Lazo Apostolovski
wrote:

> Possible solution:
> Based on what I've read, the developer can call git
> rebase -i and to select edit on #1 commit. After changes
> are done, git commit --amend command can be used on that
> commit. Also for commit message we place copied Change
> ID: from gerrit site. After that git rebase --continue
> is executed, and rebase continues so on.
>
> The problem here is that commits #2-10 have new Change
> id, and when i call git push origin refs/for/master i
> going to have duplicate 10 commits for code review.
> What I do wrong here? Do I need manual abandon all old
> commits for review? (What about we have 10 developers
> with 10 commits each day?)

If you install and use the change ID hook, you should not
need to copy the changeIds from Gerrit since your first
upload will have dictated what the changeIds will be. When
you edit your change #1 during your rebase, all the changes
will keep their original changeIds, so when you repush them,
they will be added as new patchsets instead of as new
changes.

You can enforce this better workflow be modifying the
project to require always changeIds, thus preventing
accidental uploads without them,

-Martin

[Lazo Apostolovski]

Thank you Martin.

I didn't know that there is commit-msg hook. Now I get the hook and problem with duplicated patch seems to be solved.

I have another question now. When I commit #1 commit, patch set for commits #2-10 is also uploaded. This patch are not reviewed and people need to review them again. Is there any solution about this?

For now we use only one master branch. Can someone give me suggestion to use gerrit with different branches? For each feature I need different branch or for each developer? Also how that branches get merged in master?

Thank you.

[Edwin Kempin]

2012/1/17 Lazo Apostolovski <lapost...@tricode.nl>

It is best practice to locally develop each feature in an own branch. This ensures that you don't get any unwanted coupling between the changes. When a feature is ready the developer can simply push it from the local feature branch to the central master branch for code review.

Of course you can have also central branches to develop certain features. But I would say that this only makes sense if it's a bigger feature and several developer should collaborate to develop it. To merge a central branch into the central master branch, simply create merge commit locally and then push this merge commit for code review to the central master branch.

Developers may want to have own central branches for different purposes (for backup of unfinished festures, to collaborate with other developers etc.). You may want to use the sandbox feature [1] to allow developers to create and manage their own central branches in a sandbox area.

Often additional central Gerrit branches are used when you prepare a new release for the project (as an example you can see that for the Gerrit development we have a 'stable' branch which is used to prepare the next release).

[1] https://gerrit-review.googlesource.com/Documentation/access-control.html#_project_access_control_lists
 

Thank you.

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

[Swindells, Thomas]

My initial reaction to this is whether you have the right process with how you are splitting up your commits.

Normally (but not always) if a task is broken up into smaller chunks then I would expect those chunks to be pushed

for review when they are complete. It sounds like this isn’t the case and you are only pushing the changes after all of

them have been done? If this is your standard method have you considered whether it is better just to get the developers

to squash the commits into a single change. It is much easier to update a single change rather than a chain of changes,

sometimes it is also easier for the reviewers to see the entire picture and review all the files in one go, other times it does

make reviewing much harder, depending on what the change is and how your project is structured.

 

In terms of controlling what you push the instructions to use “git push origin HEAD:refs/for/master” is a simplification and

not the complete picture, the format is really “git push origin <sha of last commit>:refs/for/<branch>”. HEAD is a convenient

shortcut to refer to the last commit on your current working branch – commit #10 in this case. However you can also specify the

actual SHA  so if the SHA of #3 is abcde… you can use the command “git push origin abcde:refs/for/master” and it will only push

commits up to #3 and not push #4-#10.

 

Thomas

 

--

To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---

**************************************************************************************
This message is confidential and intended only for the addressee. If you have received this message in error, please immediately notify the postm...@nds.com and delete it from your system as well as any copies. The content of e-mails as well as traffic data may be monitored by NDS for employment and security purposes. To protect the environment please do not print this e-mail unless necessary.

NDS Limited. Registered Office: One London Road, Staines, Middlesex, TW18 4EX, United Kingdom. A company registered in England and Wales. Registered no. 3080780. VAT no. GB 603 8808 40-00
**************************************************************************************

[Jason Axelson]

Hi Lazo,

I think you may be helped by the trivial rebase detector hook.

Here's where you can download the hook:
https://www.codeaurora.org/xwiki/bin/QAEP/Gerrit

I've also made an example patchset-created script you can use when you
install the hook, you'll just need to input your gerrit site path that
is passed into --private-key.

https://gist.github.com/1628673

Jason

[Magnus Bäck]

On Tuesday, January 17, 2012 at 08:45 CET,
Lazo Apostolovski <lapost...@tricode.nl> wrote:

> I didn't know that there is commit-msg hook. Now I get the hook and
> problem with duplicated patch seems to be solved.
>
> I have another question now. When I commit #1 commit, patch set for
> commits #2-10 is also uploaded. This patch are not reviewed and people
> need to review them again. Is there any solution about this?

Not upload 10 commits at a time? I'm serious. If you find yourself doing
that often, I think you have greater problems than Gerrit not working
perfectly. What if someone has serious objections in one of the first
commits, affecting everything you do in the remaining commits? Do your
commits have the right scope?

[...]

--
Magnus Bäck Opinions are my own and do not necessarily
SW Configuration Manager represent the ones of my employer, etc.
Sony Ericsson

[Lazo Apostolovski]

I going to give a try to this hook.

[Lazo Apostolovski]

On Wednesday, January 18, 2012 8:42:55 AM UTC+1, Magnus Bäck wrote:

On Tuesday, January 17, 2012 at 08:45 CET,
     Lazo Apostolovski <lapost...@tricode.nl> wrote:

> I didn't know that there is commit-msg hook. Now I get the hook and
> problem with duplicated patch seems to be solved.
>
> I have another question now. When I commit #1 commit, patch set for
> commits #2-10 is also uploaded. This patch are not reviewed and people
> need to review them again. Is there any solution about this?

Not upload 10 commits at a time? I'm serious. If you find yourself doing
that often, I think you have greater problems than Gerrit not working
perfectly. What if someone has serious objections in one of the first
commits, affecting everything you do in the remaining commits? Do your
commits have the right scope?

What scope commits need to have? 

And what about bigger functionality? The developer should commit everything at once?

How can more developers work on same functionality?

Thank you.

[Edwin Kempin]

2012/1/18 Lazo Apostolovski <lapost...@tricode.nl>

On Wednesday, January 18, 2012 8:42:55 AM UTC+1, Magnus Bäck wrote:

On Tuesday, January 17, 2012 at 08:45 CET,

     Lazo Apostolovski <lapost...@tricode.nl> wrote:

> I didn't know that there is commit-msg hook. Now I get the hook and
> problem with duplicated patch seems to be solved.
>
> I have another question now. When I commit #1 commit, patch set for
> commits #2-10 is also uploaded. This patch are not reviewed and people
> need to review them again. Is there any solution about this?

Not upload 10 commits at a time? I'm serious. If you find yourself doing
that often, I think you have greater problems than Gerrit not working
perfectly. What if someone has serious objections in one of the first
commits, affecting everything you do in the remaining commits? Do your
commits have the right scope?

What scope commits need to have? 

I would say, a commit should implement a piece of functionality that is useful on its own. Personally I think it's very important that all code changes in the commit are really necessary for this functionality. Code changes that are not related should go into own independent commits. Often it makes also sense to seperate refactoring from adding the functionality. In general it is always good to keep the reviewer in mind and to try making the life of the reviewer easy. Reviewers like small commits that implement a clearly defined functionality that looks useful to them and where they can immediately see that the code changes are needed for this functionality. In addition reviewers want to understand the motivation for the change from the commit message.
 

And what about bigger functionality? The developer should commit everything at once?

Try to split bigger functionality into smaller pieces, but in a way that each piece adds some piece of functionality that is already usable. I know that this is difficult, but if it's not possible I think one big commit with the big feature is better than having an chain of commits that only make sense together.
 

How can more developers work on same functionality?

Thank you.

 

[...]

--
Magnus Bäck                   Opinions are my own and do not necessarily
SW Configuration Manager      represent the ones of my employer, etc.
Sony Ericsson

--

[Lazo Apostolovski]

I try this script out and I do something wrong. I give chmod +x on patchset-created hook and on trivial_rebase.py. That is executed but i receive this in logs:

Permission denied (publickey).

return code is 255

stdout and stderr is

None

Traceback (most recent call last):

  File "./trivial_rebase.py", line 218, in <module>

    Main()

  File "./trivial_rebase.py", line 163, in Main

    prev_revision = FindPrevRev(options.changeId, options.patchset, server)

  File "./trivial_rebase.py", line 85, in FindPrevRev

    revisions = GsqlQuery(sql_query, server)

  File "./trivial_rebase.py", line 70, in GsqlQuery

    (gsql_out, gsql_stderr) = CheckCall(gsql_cmd)

  File "./trivial_rebase.py", line 62, in CheckCall

    raise CheckCallError(command, cwd, process.returncode, std_out, std_err)

__main__.CheckCallError: (['ssh', '-p', '29418', 'localhost', 'gerrit', 'gsql', '--format', 'JSON', '-c', '"SELECT revision FROM patch_sets,changes WHERE patch_sets.change_id = changes.change_id AND patch_sets.patch_set_id = 7 AND changes.change_key = \'Ifd98591d837c4309f849bb14b4eb57b347417e2c\'"'], None, 255, '', None)

I think that I do something wrong on this line!

$DIR/trivial_rebase.py --change=${2} --project=${6} --commit=${12} --patchset=${14} --private-key=/home/administrator/gerrit/mySite/etc/ssh_host_key 1> error.log >> output.log

Thanks.

[Lazo Apostolovski]

solved :-)

solution->

https://www.codeaurora.org/2010/07/06/spotting-trivial-rebases-in-gerrit/

[Magnus Bäck]

On Wednesday, January 18, 2012 at 08:47 CET,
Lazo Apostolovski <lapost...@tricode.nl> wrote:

> What scope commits need to have?

To paraphrase Albert Einstein, make them as small as possible, but not
smaller.

> And what about bigger functionality? The developer should commit
> everything at once?

Absolutely not, big commits that change large chunks of code become
a real pain to review. Commits that are painful to review tend to get
a sloppy and/or late review, thus both hurting developer morale and
productivity and lowering the quality of the code.

Why not just upload the commits in smaller batches? Putting together
ten commits takes a while, so while you're working on commits 4-10
your peers could review commits 1-3.

> How can more developers work on same functionality?

You mean how more than one developer can collaborate on a larger
feature? If the feature can't be iteratively developed, the team
can share a branch where they can implement the new functionality
in isolation from the trunk. When everything's ready, the branch
can be merged to the trunk in one go.

[Lazo Apostolovski]

this script is for Gerrit version 2.1.2.2

we use Gerrit version 2.2.1 and something does not work here :-(

Does this script not compatible? And does this ging to be implemented in some Gerrit release?

[JT Olds]

I've updated the script to work with 2.2.2.1

All that I found that needed to change was the SQL in the GetApprovals
call to:

  sql_query = ("\"SELECT value,account_id,category_id FROM
patch_set_approvals "
               "WHERE patch_set_id = %s AND change_id = (SELECT
change_id FROM "
               "changes WHERE change_key = \'%s\') AND value <> 0\""
               % ((patchset - 1), changeId))

The full script updated for 2.2.2.1 can be found here: https://gist.github.com/1896929

On Jan 18, 9:58 am, Lazo Apostolovski <lapostolov...@tricode.nl>
wrote:

[Nasser Grainawi]

If you wouldn't mind, could you update the copy that's in gerrit's contrib/ dir?

thanks,
Nasser

> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[Luciano Carvalho]

That brings me to a question I have in my mind now.

I've been out for some time, but using Gerrit since 2.0.17

What are all those Gerrit stuff on github?  This is the second one I see and I'm wondering what's going on.

Thanks,

Luciano

[JT Olds]

Uh, I couldn't seem to figure out how to push to Gerrit's Gerrit instance. Either I'm doing something wrong or there's a bug.

First (not part of the bug, just confusing), the contributor page says I need to mail in a signed Grant document to Google. Is that in addition to signing the contributor agreement online, or do I really not need to mail in anything? The individual agreement says both that you can sign it online and that you need to mail in something when you read it.

Secondly, after filling out the agreement (which I was prompted to do by running git push https://gerrit-review.googlesource.com/p/gerrit), I tried pushing again with the same command and got the following error:

Compressing objects: 100% (4/4), done.

Writing objects: 100% (4/4), 479 bytes, done.

Total 4 (delta 3), reused 0 (delta 0)

send-pack: protocol error: bad band #69

fatal: The remote end hung up unexpectedly

fatal: The remote end hung up unexpectedly

Since then, no subsequent pushes have worked, and have failed with this error:

Individual contributor agreement requires current contact information.

Please review your contact information:

  https://gerrit-review.googlesource.com/#/settings/contact

My contact information is definitely up to date, and the Gerrit UI has a recent timestamp on it.

Attached is the patch if you'd be so kind as to submit it, or inform me of what I'm screwing up. :)

-JT

[JT Olds]

Just to clarify, I did in fact try both

git push https://gerrit-review.googlesource.com/p/gerrit HEAD:refs/for/master

and just

git push https://gerrit-review.googlesource.com/p/gerrit

[Bob Foerster]

Did you set an HTTP password?

Per the original "Gerrit's own gerrit thread":

= Uploading Changes =

A contributor must generate a password by visiting Settings > HTTP
Password, or directly jumping to the password generator URL in a web
browser:

  https://gerrit-review.googlesource.com/new-password

Once a password has been saved to ~/.netrc, changes can be pushed for
review over HTTP:

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

[JT Olds]

I did, yes.

I got it to push now, but a few more thoughts about what happened.

I run Debian Squeeze, so I was running Git 1.7.2.5. I tried again in a Wheezy schroot (git 1.7.9) and actually got so far as for Gerrit to reject my change because my email didn't match. I registered my preferred email address with Gerrit and tried again, and I again got the bad band #69 error. I then tried and amended my commit to match my gmail address and pushed again. That failed and wanted me to update my contact info. I updated my contact info. It still thought my contact info needed updating. I then noticed that it appears Gerrit only supports one registered email at a time, so I tried to reregister my gmail address. Again, same problem. So I gave up.

I just came back to it after closing out my browser and waiting some time, and it pushed.

https://gerrit-review.googlesource.com/#/c/33175/