Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update path.c #587

Closed
wants to merge 1 commit into from
Closed

Update path.c #587

wants to merge 1 commit into from

Conversation

crides
Copy link

@crides crides commented Jul 20, 2018

Might be an error? path is obviously not a struct and should not use the -> operator.

@crides
Copy link
Author

crides commented Jul 23, 2018

Seems like the failed ci is related to one of the tests not working. Otherwise since Travis doesn't seem to have a Mingw integration, so manual testing is needed

@bjorn
Copy link
Contributor

bjorn commented Jul 24, 2018

The test has been fixed in the meantime (so you could rebase to get a green tick), but indeed a MinGW CI is missing, which could be one reason this code breakage wasn't noticed earlier. A MinGW CI could be set up on AppVeyor.

@crides
Copy link
Author

crides commented Jul 25, 2018

Oh, great. But rebasing on git is new for me, and after some research I haven't figured out the correct way to do it. Can you give a few examples for this case? Thanks!

Here is a list of what I've tried:

git rebase --interactive master
git rebase --interactive munificent:master
git rebase --interactive e47f175  # which is the lastest commit on munificent:master
git rebase --interactive HEAD~1  # followed by some unsuccessful `pick` commands in the editor

@bjorn
Copy link
Contributor

bjorn commented Aug 1, 2018

I don't know how you set up your repository, so giving commands is always a little dangerous. Assuming you have a remote called "munificent" pointing to the upstream repository, and you are currently on your own master branch, you would do:

git fetch munificent
git rebase munificent/master

And assuming you've got your own repo pointed to by the remote origin, you update this PR afterwards by doing:

git push -f origin master

@crides
Copy link
Author

crides commented Aug 2, 2018

Strange. Only the 32-bit Mac CIs failed with the test/core/bool/equality.wren timed out.

@bjorn
Copy link
Contributor

bjorn commented Aug 2, 2018

@crides Where did the merge commit come from? Did you forget the -f when pushing your local master? It seems like what happened was something like this:

git fetch munificent
git rebase munificent/master
git pull                                        # Should not have done a pull!
git push

When you pulled your own master branch, you created a merge commit that merged your non-rebased version of the patch to path.c into the rebased version along with the changes made in the main repository in the meantime. But we really only want the one rebased patch to path.c in this pull request.

So now we need to somehow clean up the mess. You can start by throwing away the merge commit:

git reset --hard HEAD^

Then your local master should be a clean version of munificent's master, plus your one patch to path.c. Verify this by looking at it with git log or gitk.

If everything looks alright, force-push to the master repo in your fork:

git push -f origin master

@crides
Copy link
Author

crides commented Aug 2, 2018

Sorry for that... I am not really familiar with merging branches and rebasing. Also, my bad for not looking into git log. I just scrambled all the things together and thought it was done. Now it should be fine :-).

@munificent
Copy link
Member

Got to this one first: #657

(Sorry, I should have merged yours in since it was older, but I didn't notice it.)

@munificent munificent closed this Feb 20, 2019
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.

None yet

3 participants