mauroc / squiddio_pi

squiddio_pi
3 stars 13 forks source link

Question about PR format - JG's PR changes #80

Closed rgleason closed 4 years ago

rgleason commented 4 years ago

I want to get this PR right.

This PR was to include Jon Gough's changes other than the flatpak ones that did not build. However I see that the potential PR to mauro's master shows commits on Oct 12 (leamas-reset35), Oct 19 (Merge branch "mauroc:master") and Oct 22 (Merge branch "mauroc:master"). See https://github.com/mauroc/squiddio_pi/compare/master...rgleason:master

The result looks OK to me and it does build with green checks but correctly fails upload to Jon's cloudsmith because I don't have credentials (its the master branch).

Is this ok to make a PR, or should I from my local repository, git pull upstream master (which is mauroc:master) --force and then push --force that up to rgleason:master and then rewrite the two changes?

Then make the PR?

leamas commented 4 years ago

You should first checkout a feature branch (call it whatever you like)

$ git checkout  -b jon-fixes

The, rebase it against current upstream:

$ git remote add upstream https://github.com/mauroc/squiddio_pi.git
$ git remote update upstream
$ git rebase upstream/master

The last command will most likely generate conflicts which you have to resolve. After doing that, push your branch to your own repo:

$ git push --set-upstream origin jon-fixes

And then, in the github gui, create the pull request. Don't forget to inform Jon so he can review it.

rgleason commented 4 years ago

Thanks very much. That goes in the book. Worked like a charn, no conflicts.

C:\Users\Rick\Documents\GitHub\squiddio_pi>git checkout -b jon-fixes
Switched to a new branch 'jon-fixes'

C:\Users\Rick\Documents\GitHub\squiddio_pi>git remote add upstream https://github.com/mauroc/squiddio_pi.git
fatal: remote upstream already exists.

C:\Users\Rick\Documents\GitHub\squiddio_pi>git remote update upstream
Fetching upstream
From https://github.com/mauroc/squiddio_pi
 * [new branch]      jongough-cmake_changes -> upstream/jongough-cmake_changes
 * [new tag]         v1.0.17                -> v1.0.17

C:\Users\Rick\Documents\GitHub\squiddio_pi>git rebase upstream/master
First, rewinding head to replay your work on top of it...
Applying: remove redundant ignore
Applying: version.h.in add tweak
Applying: change patch version data

C:\Users\Rick\Documents\GitHub\squiddio_pi>git push --set-upstream origin jon-fixes
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 8 threads
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 1.03 KiB | 50.00 KiB/s, done.
Total 10 (delta 6), reused 0 (delta 0)
remote: Resolving deltas: 100% (6/6), completed with 4 local objects.
remote:
remote: Create a pull request for 'jon-fixes' on GitHub by visiting:
remote:      https://github.com/rgleason/squiddio_pi/pull/new/jon-fixes
remote:
To https://github.com/rgleason/squiddio_pi.git
 * [new branch]      jon-fixes -> jon-fixes
Branch 'jon-fixes' set up to track remote branch 'jon-fixes' from 'origin'.

C:\Users\Rick\Documents\GitHub\squiddio_pi>

Now to make the PR.

rgleason commented 4 years ago

The PR is here https://github.com/mauroc/squiddio_pi/pull/81 It looks okay, but my Oct 12 (leamas-reset35), Oct 19 (Merge branch "mauroc:master") and Oct 22 (Merge branch "mauroc:master") are still there.

rgleason commented 4 years ago

NOTES: While making this PR after A. Leamas set of git commands (noted above)

It looks pretty much the same as the one before, and the changes are the same too. https://github.com/mauroc/squiddio_pi/compare/master...rgleason:master

The four travis cloudsmith deploys failed again due to 
Checking raw package upload parameters ... -/|ERROR
Failed to validate upload parameters! (status: 403 - Forbidden)

Detail: You do not have permission to perform this action.
Exited with code 147

However I will make the PR and hope that it corrects itself in Mauro's repository. I learned some new things that will be helpful. Thanks

rgleason commented 4 years ago

Everything passed except travis clang osx which takes longer.