Skip to content

Conversation

roicos
Copy link
Contributor

@roicos roicos commented Aug 31, 2016

This fix #3022
screenshot from 2016-09-01 00-06-49

@roicos
Copy link
Contributor Author

roicos commented Aug 31, 2016

I have the same problem with old commits again. Only last commit is related to the #3022 and it is in the new branch (iss3022), but the previous are (already merged) and they are in master branch. Please, hint me how to fix this?

@simonbrunel
Copy link
Member

simonbrunel commented Aug 31, 2016

When you create a new branch, you need to start from upstream/master (assuming that the upstream remote refers to chartjs/Chart.js repository). If you don't have a remote for chartjs, add it:

git remote add upstream https://github.com/chartjs/Chart.js.git

Then, in your case, I would simply do a git remote update which will fetch all branches from all remotes (but you can fetch only the remotes/upstream/master branch if you prefer).

Creating a new branch from upstream/master:

git remote update
git checkout --no-track -b iss3022 remotes/upstream/master

Re-syncing with upstream/master (e.g. before submitting a PR):

git remote update
git rebase remotes/upstream/master

I prefer rebasing over merging to re-sync a PR, but it's debatable. If you rebase, don't forget to force push to your fork remote (which might certainly be origin).

git push origin iss3022 --force 

Once a PR is merged, delete the branch, remotely and locally. I think your issue is that you created a new branch from your local master branch which might not be synced with chartjs/master.

Note: I use a GUI on top of Git, so might not be the correct / optimal commands

@roicos
Copy link
Contributor Author

roicos commented Sep 1, 2016

I really create new branch from local master (git checkout -b iss3022), but before sync with remote/master like it is described here. The difference is in "git rebase remotes/upstream/master". I apply it and did "git push origin iss3022 --force" once again, but there are still 11 commits in the PR.

@simonbrunel
Copy link
Member

simonbrunel commented Sep 1, 2016

The extra commits might be because we squashed your PR on merge: they still appear in your local master but not in the chartjs master. I would advice you to delete your local master branch (if you have no change in it), and re-create it from upstream: git checkout -b master remotes/upstream/master. If you want the master branch in your remote fork, then force push to origin.

I don't use the same process described in your link, I never work directly in master (always in different branches) and when I need to re-sync my local master (very rarely), I rebase it on remotes/upstream/master. If you start a new branch for an upstream PR (like this one), I don't think you should create it from your local master, but always from upstream/master.

The easiest way to fix that PR is that since it's a one line change, you can delete your local branch, recreate-it with the command in my previous message, do your change and force push to the same iss3022 remote branch.

@simonbrunel
Copy link
Member

I cloned your fork and did a rebase of your iss3022 branch on top of remotes/upstream/master and it works by skipping the first 10 commits and picking the last one.

@roicos
Copy link
Contributor Author

roicos commented Sep 2, 2016

Ok, thank you for instructions. I prefer to recreate master because I had conflicts during rebase process. Now it's only one commit in PR)

@simonbrunel
Copy link
Member

Glad that helped you :)

The fix LGTM, @etimberg what do you think?

We should make the commit message a bit more explicit when merging.

@etimberg
Copy link
Member

etimberg commented Sep 2, 2016

@simonbrunel I am +1 to merge

@simonbrunel simonbrunel merged commit 6408ba4 into chartjs:master Sep 2, 2016
@roicos roicos deleted the iss3022 branch September 3, 2016 15:40
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug : rotate multilines center labels

3 participants