Having trouble wrapping my head around "An effective RBTools workflow for Git"

[dave.a...@shipwire.com]

Referring to this article: http://blog.beanbaginc.com/2015/01/26/an-effective-rbtools-workflow-for-git/

What I need is to be able to land changes not in master but in a remote branch off of master. Our release process merges these branches into master via pull requests at the behest of DevOps, not the individual developers. Landing in master is not an option, because there is a qualification process that needs to take place prior to merge to master.

I'm pretty sure there is something I'm not understanding about Git and/or rb, I'm posting here in case someone can help me spot the issue.

When I look in the BitBucket webUI, oddly I only see the mb-1 commits on the "commits" page(none of the mb-2 commits) and I see the mb-2 branch on the branches pagem but not the mb-1 branch. The mb-1 commits are in the mb-2 branch.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (master)
$ git checkout -b mb-1
Switched to a new branch 'mb-1'

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ git push --set-upstream origin mb-1
Total 0 (delta 0), reused 0 (delta 0)
remote:
remote: Create pull request for mb-1:
remote:   https://bitbucket.org/.../rb-test/pull-requests/new?source=mb-1&t=1
remote:
To git@bitbucket.org:shipwire/rb-test.git
 * [new branch]      mb-1 -> mb-1
Branch mb-1 set up to track remote branch mb-1 from origin.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ vi foo.py

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ vi bar.py

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ git commit -a
[mb-1 216b719] Adding via mb-1
 2 files changed, 4 insertions(+)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ git checkout -b mb-2
Switched to a new branch 'mb-2'

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git status
On branch mb-2
Untracked files:
  (use "git add <file>..." to include in what will be committed)

        find_gitpush_done
        ../conf/file.txt
        ../copy_merchant_output.txt
        ../copy_merchant_output2.txt
        ../output.txt
        ../output2.txt

nothing added to commit but untracked files present (use "git add" to track)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git push --set-upstream mb-1 mb-2
fatal: 'mb-1' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git push --set-upstream origin/mb-1 mb-2
fatal: 'origin/mb-1' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git push --set-upstream origin mb-2
Counting objects: 5, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 451 bytes | 0 bytes/s, done.
Total 5 (delta 4), reused 0 (delta 0)
remote:
remote: Create pull request for mb-2:
remote:   https://bitbucket.org/.../rb-test/pull-requests/new?source=mb-2&t=1
remote:
To git@bitbucket.org:shipwire/rb-test.git
 * [new branch]      mb-2 -> mb-2
Branch mb-2 set up to track remote branch mb-2 from origin.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ gitk

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ vi foo.py

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git commit -a
[mb-2 ec57060] Add via mb-2
 1 file changed, 2 insertions(+)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git checkout mb-1
Switched to branch 'mb-1'
Your branch is ahead of 'origin/mb-1' by 1 commit.
  (use "git push" to publish your local commits)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ rbt post
Review request #71 posted.

http://rb.tools.aws.....com/r/71/
http://rb.tools.aws.....com/r/71/diff/

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ git checkout mb-2
Switched to branch 'mb-2'
Your branch is ahead of 'origin/mb-2' by 1 commit.
  (use "git push" to publish your local commits)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ rbt post
Review request #72 posted.

http://rb.tools.aws.....com/r/72/
http://rb.tools.aws.....com/r/72/diff/

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ git checkout mb-1
Switched to branch 'mb-1'
Your branch is ahead of 'origin/mb-1' by 1 commit.
  (use "git push" to publish your local commits)

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ rbt land --dest=mb-1 --push mb-1
ERROR: The local branch cannot be merged onto itself. Try a different local branch or destination branch.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-1)
$ rbt land --dest=origin/mb-1 --push mb-1
Merging branch "mb-1" into "origin/mb-1"
Deleting merged branch "mb-1"
Pushing branch "origin/mb-1" upstream
ERROR: Could not pull changes from upstream.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin ((6efeb87...))
$ rbt land --dest=mb-1 --push origin/mb
Failed to execute command: ['git', 'rev-parse', 'origin/mb']
origin/mb
fatal: ambiguous argument 'origin/mb': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'


Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin ((6efeb87...))
(arg: 2) rbt land --dest=mb-1 --push origin/mb

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin ((6efeb87...))
$ rbt land --dest=mb-2 --push origin/mb-2
Merging branch "origin/mb-2" into "mb-2"
Failed to execute command: ['git', 'commit', '-m', u'Adding via mb-1\n\nReviewed at http://rb.tools.aws.....com/r/7
1/', u'--author="Dave Anderson <David....com>"']
On branch mb-2
Your branch is ahead of 'origin/mb-2' by 1 commit.
  (use "git push" to publish your local commits)
Untracked files:
        bin/find_gitpush_done
        conf/file.txt
        copy_merchant_output.txt
        copy_merchant_output2.txt
        output.txt
        output2.txt

nothing added to commit but untracked files present


Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin (mb-2)
$ rbt land --dest=origin/mb-2 --push mb-2
Merging branch "mb-2" into "origin/mb-2"
Deleting merged branch "mb-2"
Pushing branch "origin/mb-2" upstream
ERROR: Could not pull changes from upstream.

Dave-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test/bin ((ca80b47...))
$

[Christian Hammond]

Hi Dave,

You don't have to merge to master. You can set LAND_DEST_BRANCH in .reviewboardrc to point to any branch you want.

Any commits you're working on must be done on a feature branch that comes off the branch you want to land on. You wouldn't be committing and landing to the same location. For instance, this is how your branches might look:

o [master] [origin/master]

|

| o my-feature-branch

| |

| o [my-land-dest] [origin/my-land-dest]

|/

|

You'd set LAND_DEST_BRANCH to "my-land-dest", and what you'd be pushing upstream. Code in development that would go up for review would be committed to "my-feature-branch" or equivalent. It would only end up on "my-land-dest" when it's been reviewed and ready to land (rbt land would take care of this). That's the key thing. The upstream branch would be treated similarly to the way you're treating "master" today. You'd let `rbt land` manage commits going on that branch.

Since you're not working off of master itself, you're also going to want to set TRACKING_BRANCH to "origin/my-land-dest". This is used to tell RBTools what the nearest tracking branch would be for the purpose of generating diffs and parent diffs.

So your .reviewboardrc would be:

REVIEWBOARD_URL = "https://yourserver.example.com"

REPOSITORY = "<Your Repository Name in RB>"

LAND_DEST_BRANCH = "my-land-dest"

TRACKING_BRANCH = "origin/my-land-dest"

Christian

-- 
Christian Hammond

President/CEO of Beanbag

Makers of Review Board

--
Supercharge your Review Board with Power Pack: https://www.reviewboard.org/powerpack/
Want us to host Review Board for you? Check out RBCommons: https://rbcommons.com/
Happy user? Let us know! https://www.reviewboard.org/users/
---
You received this message because you are subscribed to the Google Groups "reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[dave.a...@shipwire.com]

Thanks Christian, I have been on PTO since Thursday and am catching up today. I think I understand what you said, though I think we change things up too often to rely on LAND_DEST_BRANCH. Will give it a whirl later today/tomorrow.

D

To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard...@googlegroups.com.

[dave.a...@shipwire.com]

I've spent quite a bit of time playing with this and while I think I am getting closer to a usable approach I still have some concerns.

As an aside, the following example from the workflow blog post creates a merge conflict:

$ git checkout master
$ rbt land --dest=master --push my-branch-1
$ rbt land --dest=master --push my-branch-2

The two land commands should be performed in the reverse order, as far as I can tell.

Anyway, on to the workflow that I am trying to mold and shape from my end, as I try to understand how to mold and shape RB to fit. I am starting to think this is a square peg/round hole problem but since I really want the file-level reviewers functionality(to ultimately extend to required reviewers) I persist. :)

Your comment "You wouldn't be committing and landing to the same location." seems to advise against using land at all. Here is the current workflow for the most common use case:

1. git checkout master
2. git fetch && git pull
3. git checkout -b working-branch
4. make some changes
5. git commit
6. git push --set-upstream origin working-branch (or otherwise push to remote branch, usually this branch has same name as the JIRA tracking the issue/task)
7. Make any changes to the draft pull request description/reviewers, then click "Create pull request" button.
8. Address any review comments iteratively with edit/commit/push
9. Once approvals are in place, merge to master.

Alternatively, a branch off of master may be created as a remote "feature branch", and devs will create their own branches off of that and we simply add another level of the above steps, similar to what you described.

So, what I am looking to do is insert RB around steps 6..8, and find a way to automagically add reviewers based on what files have been changed. My thinking was that we could create a special user in Bitbucket (e.g. "ReviewBoard") and preventing merge to master unless that reviewer has approved. This approval would be triggered by all required reviewers registering "Ship it" in RB. I'm pretty sure I know how to write the needed BB plugin and RB extensions for those integration details but certain aspects of integrating RB into our workflow are giving me trouble.

We have explored building a wrapper script that detects when rbt should have the -s and -u flags applied and I had posted a while back about getting the stamp in place on the first push. I solved that by doing a push with --set-upstream immediately after creating the branch prior to any commits.

The challenge for me at this point seems to be coming up with an RB workflow that supports our push to a remote branch that has the same name as the local branch and is NOT master. I think that the fundamental requirement of "land" needing to land to a different branch precludes leveraging that.

Does RBTools have any automation around determining what the branch heirarchy is, short of using "land"? Is there a simple patch I could apply to land.py to allow landing on the same branch?

It's beginning to look like using RB in our workflow will be difficult to automate; I was hoping to make it transparent to users but that's seeming less and less likely. Any help you can offer to help simplify this transition would be appreciated.

On Thursday, May 11, 2017 at 2:28:46 PM UTC-7, Christian Hammond wrote:

To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard...@googlegroups.com.

[Christian Hammond]

Hi Dave,

So I think there may be some confusion as to what `rbt land` is and what it ultimately does, and I want to go into that to see if it clears anything up.

rbt land just does the following:

1) Makes sure the commit has a review request that has been approved

2) Updates the commit message with the latest from the review request, and stamping the review request (basically calling `rbt stamp`)

3) Merges the feature branch onto the destination branch (optionally squashing)

4) Optionally deleting the original branch, and optionally pushing the destination branch

If you want to commit and land to the same branch, then `rbt land` isn't for you. The purpose of the command is to take a change that's not on the destination, verify that it's landable, and then get it to the destination. The reason it enforces using a separate feature branch off the land destination branch is to ensure that you don't accidentally push changes that haven't been approved for review.

Doing development on the branch that you'd then land to means the "Ship It" checking won't be as effective. If a person accidentally git pushes the branch, they've just put themselves and everyone else in a state where there's code now in the repository that's either unreviewed or has issues that still need to be addressed.

Given that, I don't think it makes sense to add an option to rbt land for this use case. You'd probably want something that ties together the `rbt stamp` and `git push`. You can do this easily by defining an alias in the repository's `.reviewboardrc` that hooks those operations together.

RBTools has no automation around branch hierarchy. `rbt land` doesn't care about hierarchy. It's going to land the branch you specify at the target you choose. RBTools 0.8 will have some stuff for auto-determining the nearest tracking branch, but that's it.

Christian

-- 
Christian Hammond

President/CEO of Beanbag

Makers of Review Board

To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscribe@googlegroups.com.

[dave.a...@shipwire.com]

Thanks Christian, that helps a lot, especially drawing my attention to aliases. I think that between git and rb aliases I might be able to come up with something elegant and simple.

A couple of things:

Is there a way to get rbt to remove "Ship it" to address the unreviewed changes scenario that you describe? I am not unduly worried about that -- IMO that falls under "bad behavior" which we don't have much trouble with at our small size. But, an ounce of prevention...

Having said that, I should point out that setting up required reviewers per file/dir is more of a check for unintended consequences than a check for bad behavior. :)

Also, I have found that I get errors when passing '-u' to rbt when there is nothing to update. It would simplify automation if there were a way to get rbt to ignore use of '-u' when it's a no-op instead of erroring out. I would not be surprised to hear that it needs to be that way for some good reason, but I thought I would toss it out there since it seems like an easy thing for people to overlook in their workflows. I may be able to alias my way out of that though...

Thanks again, I'm off to play with aliases. :)

Dave

[Christian Hammond]

Hey Dave,

Glad that helped :)

On Wed, May 17, 2017 at 3:34 PM, <dave.a...@shipwire.com> wrote:

Thanks Christian, that helps a lot, especially drawing my attention to aliases. I think that between git and rb aliases I might be able to come up with something elegant and simple.

A couple of things:

Is there a way to get rbt to remove "Ship it" to address the unreviewed changes scenario that you describe? I am not unduly worried about that -- IMO that falls under "bad behavior" which we don't have much trouble with at our small size. But, an ounce of prevention...

I wasn't sure what you meant here by removing Ship It. Can you go into that more?

 

Having said that, I should point out that setting up required reviewers per file/dir is more of a check for unintended consequences than a check for bad behavior. :)

Also, I have found that I get errors when passing '-u' to rbt when there is nothing to update. It would simplify automation if there were a way to get rbt to ignore use of '-u' when it's a no-op instead of erroring out. I would not be surprised to hear that it needs to be that way for some good reason, but I thought I would toss it out there since it seems like an easy thing for people to overlook in their workflows. I may be able to alias my way out of that though...

Hmm, errors aren't good. What are you seeing?

-u has to do some guesswork to find a matching review request. What it does is check the user's pending review requests, trying to perform matches against the summary and the description. If there's a review request with an exact match, then it can feel comfortable choosing that one, but if not, it falls back to fuzzy matching with confirmation. Unfortunately, with Git, there isn't a really good, reliable, fool-proof way of storing metadata for associating things like a review request with a branch.

Anyway, because it can't directly look up a matching review request through associated metadata, it has to scan and match, and that means it can't really assume that the user would want a brand new review request (there might be another you want to match that has a different summary and description from what you have locally).

 

Thanks again, I'm off to play with aliases. :)

This might help a bit:

http://blog.beanbaginc.com/2015/01/19/composing-workflows-using-aliases-in-rbtools/

To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscribe@googlegroups.com.

[dave.a...@shipwire.com]

Inline...

On Wednesday, May 17, 2017 at 3:40:28 PM UTC-7, Christian Hammond wrote:

Hey Dave,

Glad that helped :)

On Wed, May 17, 2017 at 3:34 PM, <dave.a...@shipwire.com> wrote:

Thanks Christian, that helps a lot, especially drawing my attention to aliases. I think that between git and rb aliases I might be able to come up with something elegant and simple.

A couple of things:

Is there a way to get rbt to remove "Ship it" to address the unreviewed changes scenario that you describe? I am not unduly worried about that -- IMO that falls under "bad behavior" which we don't have much trouble with at our small size. But, an ounce of prevention...

I wasn't sure what you meant here by removing Ship It. Can you go into that more?

I was referring to the scenario where "land" is not part of the workflow, and a review request is in the "Ship It" state, then another change is pushed. Would there be some notification that it needs to be reviewed again? Or could the "Ship it"s be removed and the original "New Review Request" notification be emailed again?

 

Having said that, I should point out that setting up required reviewers per file/dir is more of a check for unintended consequences than a check for bad behavior. :)

Also, I have found that I get errors when passing '-u' to rbt when there is nothing to update. It would simplify automation if there were a way to get rbt to ignore use of '-u' when it's a no-op instead of erroring out. I would not be surprised to hear that it needs to be that way for some good reason, but I thought I would toss it out there since it seems like an easy thing for people to overlook in their workflows. I may be able to alias my way out of that though...

Hmm, errors aren't good. What are you seeing?

I guess it's reasonable:

ERROR: Could not determine the existing review request to update.

I thought that it gave me something different either on an earlier rev of rbtools or when I was just starting and it couldn't find anything to guess against.
 

[dave.a...@shipwire.com]

Having a bit of difficulty with aliases... Here is the alias entry:

    'commit1': '!git commit $* &&'
               '!export BRANCH=$(git rev-parse --abbrev-ref HEAD);rbt post -s --branch $BRANCH',
    'commit':  '!git commit $* &&'
               '!export BRANCH=$(git rev-parse --abbrev-ref HEAD) &&'
               'rbt post -u --branch $BRANCH',

These outputs are from having the export line without and with the preceding bang, respectively:

Dave Anderson@DaveAnderson-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test (rc-branchadd)
$ rbt commit
[rc-branchadd 1f8a05b] nother attempt at shell behavior
 1 file changed, 2 insertions(+), 1 deletion(-)
'export' is not recognized as an internal or external command,
operable program or batch file.

Dave Anderson@DaveAnderson-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test (rc-branchadd)
$ git add .reviewboardrc

Dave Anderson@DaveAnderson-PC MINGW64 ~/Vagrant/sws-vagrant/src/rb-test (rc-branchadd)
$ rbt commit
[rc-branchadd 2d037ce] Adding bang
 1 file changed, 2 insertions(+), 2 deletions(-)
'!export' is not recognized as an internal or external command,
operable program or batch file.

I was getting similar behavior when I had the export and rbt post on the same line as shown in the commit1 alias.

Both of the below variants on the rbt post line in the alias gave me a "CRITICAL: Too many revisions specified" error.

               'rbt post -u --branch `git rev-parse --abbrev-ref HEAD`',
               'rbt post -u --branch $(git rev-parse --abbrev-ref HEAD)',

Not sure where I'm going wrong here... I suspect it's related to shell expansion that also prevents me from running e.g. 'rbt commit -a'.

sigh. Off to get some coffee, maybe that will help.

Note, I know this info could go in the reviewboardrc file but we are changing branches all the time... we need it automated.

[Christian Hammond]

Hi,

Sorry I didn't get to this until now.

The "!" should be only on the beginning of the string. It's just a special indicator to RBTools to execute the rest of the string as a shell command. Try removing the "!" before the export and see if that works for you.

Did you figure out the Too Many Revisions error? That usually means too many positional arguments were being passed. You can try putting an echo before the command and seeing what it's all expanding to.

Christian

-- 
Christian Hammond

President/CEO of Beanbag

Makers of Review Board

To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscribe@googlegroups.com.

[dave.a...@shipwire.com]

To be clear, I realized fairly early on that I only needed one '!' and that the error message wasn't really relevant to troubleshooting the issue... as you say it's just an indication that things didn't go as planned. :)

I tried many different variations, here are some that I remember offhand; with/without export, using backticks, etc... no joy. Can you show a simple example of getting/using an environment variable that is NOT $[0-9]?

    'commit':  '!git commit $* &&'

               'export BRANCH=$(git rev-parse --abbrev-ref HEAD) &&'

               'rbt post -u --branch $BRANCH',

    'commit1': '!git commit $* &&'

               '!export BRANCH=$(git rev-parse --abbrev-ref HEAD) &&'
               'rbt post -s --branch $BRANCH',
    'commita':  '!git commit $* &&'
               'BRANCH=$(git rev-parse --abbrev-ref HEAD) &&'

               'rbt post -u --branch $BRANCH',

    'commit1a': '!git commit $* &&'
               '!BRANCH=$(git rev-parse --abbrev-ref HEAD) &&'
               'rbt post -s --branch $BRANCH',
    'commitb':  '!git commit $* &&'
               'BRANCH=$(git rev-parse --abbrev-ref HEAD);rbt post -u --branch $BRANCH',
    'commit1b': '!git commit $* &&'
                'rbt post -s --branch `git rev-parse --abbrev-ref HEAD`',

[dave.a...@shipwire.com]

I am taking the approach of creating a wrapper script to be called by my aliases. In addition to the problem of picking up and setting $BRANCH, the alias approach seems a long way from being able to support commit messages, e.g. 'git commit -m"<message>"'. I'll post in a new thread once I have a solution so others may have an easier path.

On Friday, May 19, 2017 at 12:44:24 AM UTC-7, Christian Hammond wrote:

[dave.a...@shipwire.com]

I've posted my initial solution to this here. Will update as it evolves.

[dave.a...@shipwire.com]

I meant to reiterate: