Section 5.5. Reviewing pull requests

christophernhill commented 6 years ago

@edoddridge and @jrscott the description in the 5.5. Reviewing pull requests section doesn't look quite right to me. I think it should be

git clone https://github.com/altMITgcm/MITgcm.git
cd MITgcm
git fetch https://github.com/USERNAME/MITgcm.git BRANCH
git merge FECTH_HEAD
cd verification
./testreport

Note - this is also what anyone else can do i.e. you don't need special privileges. This would be a way for Martin (for example) to test his candidate PR code too.

The current description in 5.5 would test the code in the branch, but it would not show whether it has any issues when merged against the HEAD of master that the PR applies to.

Chris

jklymak commented 6 years ago

I do what the current 5.5 says. I've not tested your way, but can you then go back and checkout your own branches after testing?

jrscott commented 6 years ago

So two comments about 5.5:

originally this was added to address ‘how can someone else help test your new feature’ i.e. help make sure it is battle-tested. That being said, there is nothing much written directly for the code developer about (trying automatic) testing other than a presumption that the person makes sure it works. Ergo need to do something about this.
I’m not crazy that 5.5 offers two different ways to do the same thing. Can we pick one?

Jeff

On Jan 10, 2018, at 4:54 PM, christophernhill notifications@github.com<mailto:notifications@github.com> wrote:

@edoddridgehttps://github.com/edoddridge and @jrscotthttps://github.com/jrscott the description in the 5.5. Reviewing pull requests section doesn't look quite right to me. I think it should be

git clone https://github.com/altMITgcm/MITgcm.git cd MITgcm git fetch https://github.com/USERNAME/MITgcm.git BRANCH git merge FECTH_HEAD cd verification ./testreport

Note - this is also what anyone else can do i.e. you don't need special privileges. This would be a way for Martin (for example) to test his candidate PR code too.

The current description in 5.5 would test the code in the branch, but it would not show whether it has any issues when merged against the HEAD of master that the PR applies to.

Chris

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/altMITgcm/MITgcm/issues/14, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFPiDgpKIUDZrET0lePLvrECjjJKdC58ks5tJTGegaJpZM4RaAIX.

jrscott commented 6 years ago

That being said, there is nothing much written directly for the code developer about (trying automatic) testing other than a presumption that the person makes sure it works. Ergo need to do something about this.

OK, I realize what happened: section 3.6 and 3.7 of the old manual fell into a black hole.

3.7 in particular is about testing (albeit very short) — need to get this content in somewhere, updated.

3.6 needs updating and expanding, but in the short-term simple conversion will do here.

Jeff

christophernhill commented 6 years ago

I think the current recipe in 5.5 misses any incompatible intervening changes in upstream/master. Checking for those is probably a good piece of due diligence to encourage, if we can.

Chris

On Wed, Jan 10, 2018 at 6:35 PM, jrscott notifications@github.com wrote:

That being said, there is nothing much written directly for the code developer about (trying automatic) testing other than a presumption that the person makes sure it works. Ergo need to do something about this.

OK, I realize what happened: section 3.6 and 3.7 of the old manual fell into a black hole.

3.7 in particular is about testing (albeit very short) — need to get this content in somewhere, updated.

3.6 needs updating and expanding, but in the short-term simple conversion will do here.

Jeff

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

jklymak commented 6 years ago

git fetch upstream master is definitely a good idea, particularly before you rebase and send back for a PR.

OTOH, I don't think you need to do anything special if you checkout a remote branch that is against the latest upstream/master. The checkout will include those changes even if your local repo doesn't have them yet.

If the remote branch you are checking out doesn't include changes in upstream/master you won't see them locally when you checkout the branch. But there the onus is on the person who made the remote PR to rebase, particularly if there have been conflicting changes.

edoddridge commented 6 years ago

@christophernhill your proposed method means that the person doing the testing has merged those changes into their master branch, which will be ok if the pull request gets merged quickly. But, if the PR hangs around for a while, or there are multiple PRs open, they will quickly get into a situation where their master branch is incompatible with the master branch of the main repo. That's not a great situation to be in...

Plus, Travis automatically tests the merged code:

$ git clone --depth=50 https://github.com/altMITgcm/MITgcm.git altMITgcm/MITgcm
Cloning into 'altMITgcm/MITgcm'...
remote: Counting objects: 10558, done.
remote: Compressing objects: 100% (7432/7432), done.
remote: Total 10558 (delta 4464), reused 7261 (delta 2963), pack-reused 0
Receiving objects: 100% (10558/10558), 154.01 MiB | 20.88 MiB/s, done.
Resolving deltas: 100% (4464/4464), done.
$ cd altMITgcm/MITgcm
0.76s$ git fetch origin +refs/pull/13/merge:
remote: Counting objects: 54, done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 54 (delta 32), reused 46 (delta 25), pack-reused 0
Unpacking objects: 100% (54/54), done.
From https://github.com/altMITgcm/MITgcm
 * branch            refs/pull/13/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD

Rather than asking developers to mess around with their master branch, I think it would be better to expand the tests on Travis to be as thorough as running ./testreport locally. Are there any barriers to this? Given that we still want to run adjoint tests in house, perhaps we should run the full test suite against all PRs and have a policy of not merging until they have successfully completed.

@jrscott I'm not sure what you mean by

That being said, there is nothing much written directly for the code developer about (trying automatic) testing other than a presumption that the person makes sure it works. Ergo need to do something about this.

The automatic testing happens automatically, so I don't understand why we need to say anything much about it. Perhaps you mean it would be worth saying that developers should run testreport locally?

@jklymak our current workflow doesn't recommend rebasing before submitting a PR. Given that many in the MITgcm community don't have much/any experience with git, we decided it was too dangerous to recommend as the default process. But, we also haven't mentioned that conflicts should be resolved by merging upstream/master into the feature branch. We're working on that at the moment.

jklymak commented 6 years ago

I've never merged - rebasing is the only way I'm able to resolve conflicts. i.e. git rebase upstream/master and then editing any conflicts, git add conflictedfile.F; git rebase --continue etc until I'm done. But maybe my git toolset is limited!

edoddridge commented 6 years ago

It's a very similar workflow: edit the conflicting file, remove the conflict markers (<<<<<, ======, >>>>), and then add the file. Repeat for each conflict, and then commit.

It can be done in a terminal or on GitHub.

If you're more comfortable with rebasing, then do that. We just don't want someone to get into a situation where they have rebased master onto their feature branch...

christophernhill commented 6 years ago

adding more to Travis is good.

I have some exploratory multi-process tests skeleton here

https://github.com/mitgcm/basic-python-examples/blob/master/.travis.yml

maybe we should try working that in soon?

Chris

On Thu, Jan 11, 2018 at 3:10 PM, Ed Doddridge notifications@github.com wrote:

@christophernhill your proposed method means that the person doing the testing has merged those changes into their master branch, which will be ok if the pull request gets merged quickly. But, if the PR hangs around for a while, or there are multiple PRs open, they will quickly get into a situation where their master branch is incompatible with the master branch of the main repo. That's not a great situation to be in...

Plus, Travis automatically tests the merged code:

$ git clone --depth=50 https://github.com/altMITgcm/MITgcm.git altMITgcm/MITgcm Cloning into 'altMITgcm/MITgcm'... remote: Counting objects: 10558, done. remote: Compressing objects: 100% (7432/7432), done. remote: Total 10558 (delta 4464), reused 7261 (delta 2963), pack-reused 0 Receiving objects: 100% (10558/10558), 154.01 MiB | 20.88 MiB/s, done. Resolving deltas: 100% (4464/4464), done. $ cd altMITgcm/MITgcm 0.76s$ git fetch origin +refs/pull/13/merge: remote: Counting objects: 54, done. remote: Compressing objects: 100% (26/26), done. remote: Total 54 (delta 32), reused 46 (delta 25), pack-reused 0 Unpacking objects: 100% (54/54), done. From https://github.com/altMITgcm/MITgcm

branch refs/pull/13/merge -> FETCH_HEAD $ git checkout -qf FETCH_HEAD

Rather than asking developers to mess around with their master branch, I think it would be better to expand the tests on Travis to be as thorough as running ./testreport locally. Are there any barriers to this? Given that we still want to run adjoint tests in house, perhaps we should run the full test suite against all PRs and have a policy of not merging until they have successfully completed.

@jrscott I'm not sure what you mean by

That being said, there is nothing much written directly for the code developer about (trying automatic) testing other than a presumption that the person makes sure it works. Ergo need to do something about this.

The automatic testing happens automatically, so I don't understand why we need to say anything much about it. Perhaps you mean it would be worth saying that developers should run testreport locally?

@jklymak our current workflow doesn't recommend rebasing before submitting a PR. Given that many in the MITgcm community don't have much/any experience with git, we decided it was too dangerous to recommend as the default process. But, we also haven't mentioned that conflicts should be resolved by merging upstream/master into the feature branch. We're working on that at the moment.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jklymak commented 6 years ago

We just don't want someone to get into a situation where they have rebased master onto their feature branch

Not sure what you mean here?

Feel free to ignore me if this discussion is not helpful - I'm not claiming to know what the best git workflow is. But I do routinely deal w/ PRs at Matplotlib, so thought I'd share my experience.

jrscott commented 6 years ago

I’m working on this section now (which might clarify your question Jody), hopefully submit today and all can read/comment.

Jeff

On Jan 12, 2018, at 1:16 PM, Jody Klymak notifications@github.com<mailto:notifications@github.com> wrote:

We just don't want someone to get into a situation where they have rebased master onto their feature branch

Not sure what you mean here?

Feel free to ignore me if this discussion is not helpful - I'm not claiming to know what the best git workflow is. But I do routinely deal w/ PRs at Matplotlib, so thought I'd share my experience.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/altMITgcm/MITgcm/issues/14#issuecomment-357311479, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFPiDu_5RCsN8DVLDrM6oi8YAnAHYtZAks5tJ6FmgaJpZM4RaAIX.

edoddridge commented 6 years ago

It is helpful - thanks.

There's a figure here that shows what I mean - they could move the new commits in master to the end of their feature branch. More generally though, because rebase rewrites history it can cause a lot of trouble if used incorrectly.

We're trying to make sure the instructions and workflow we provide are as beginner friendly as possible. If people are comfortable using rebase then they can do that. Using rebase does lead to a slightly cleaner commit history, but can also cause serious trouble if misused.

jklymak commented 6 years ago

Ah I see.

I agree that rebasing can cause problems. It took me quite a while to understand that I had to squash my own commits first (rebase -i HEAD~X, where X got me to the head of master before I made any changes) before rebasing onto upstream/master unless I wanted to have a mess. My first few PRs also included every Master change since I started my work on the branch, which was embarassing!

altMITgcm / MITgcm

Section 5.5. Reviewing pull requests #14