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

Add definitions of fat, double, and wide pointers #112

Merged
merged 4 commits into from
Apr 15, 2018

Conversation

samWson
Copy link

@samWson samWson commented Apr 8, 2018

Definition of fat pointer referenced from Programming Rust by Jim Blandy & Jason Orendorff, published by O'Reilly. Page 214: References to Slices and Trait Objects.

Definitions of double, and wide pointers refer back to fat pointers.

@samWson samWson mentioned this pull request Apr 8, 2018
27 tasks
Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks!

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2018

Could you please rebase?

@samWson
Copy link
Author

samWson commented Apr 8, 2018

@mark-i-m I haven't done much rebasing before, but I am familiar the concept. Do you want me to rebase my last commit (5cccbd) on top of the last commit before I made my changes (cd053f3)?

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2018

@samWson No worries. You should rebase this entire branch over master as follows:

# first we will update master
git checkout master
git pull origin master

# then we will rebase glossary-fat-pointers onto master

git checkout glossary-fat-pointers
git rebase master

What this does is rewind the changes on glossary-fat-pointers, update glossary-fat-pointers to look like master, and then replay your changes on glossary-fat-pointers. The end result is that master is unchanged, but glossary-fat-pointers looks like master with your changes at the end of the history.

Does that make sense?

Pulling changes from rust-lang-nursery
@samWson
Copy link
Author

samWson commented Apr 8, 2018

@mark-i-m yes I think that makes sense. I've followed your instructions. I think I've complicated things by forking the rust-lang-nursery/rustc-guide repo to my Github account, and using my Github repo as the remote for the copy on my computer.

Is rebasing a feature branch on master the standard practice before making pull requests?

@mark-i-m
Copy link
Member

mark-i-m commented Apr 8, 2018

I think I've complicated things by forking the rust-lang-nursery/rustc-guide repo to my Github account, and using my Github repo as the remote for the copy on my computer.

That's pretty common. I don't know what others do or if this is the easiest way, but I usually add another remote for the original:

git remote add rustc-guide https://github.com/rust-lang-nursery/rustc-guide.git

Then you can

git checkout master
git pull rustc-guide master # EDITED

Is rebasing a feature branch on master the standard practice before making pull requests?

Different projects have different practices. Some projects are ok with merge commits, so just doing a git merge master would be ok. Other projects like to have a "linear" history (i.e. no merges/branches), so rebasing is preferred. Still others prefer a rebase and a squash (combine all commits from the PR into one big commit).

In general, the rust community seems to prefer the rebasing approach (without squashing).

Also, usually, github can do the rebase for you, and you don't have to do it manually, but if there are conflicts (as in this PR), github will ask you to manually resolve them first...

EDIT: sorry, i got the pull command wrong... fixed above

@samWson
Copy link
Author

samWson commented Apr 9, 2018

@mark-i-m thanks. That helps. Is there anything more I need to do at my end for this PR?

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2018

Oh, hmm... I'm not sure what exactly happened, but I think the rebase didn't quite go correctly. It looks like the history has a duplicate of commits and github still is not happy (heh, don't worry, everyone does this on their first rebase 😛 ).

How did you resolve the rebase conflicts?

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2018

So, here is what I did to resolve the conflicts on my local copy:

First, make sure you have a clean repo (meaning there are no uncommitted edits). If there are, you can use git stash to clean up for now and git stash pop to restore them later.

git checkout master
git pull rustc-guide master # this should not require any merge commits
git checkout glossary-fat-pointers
git rebase master
# at this point, you should see a message that their was a conflict on src/appendix-glossary.md.
# open that file with some editor, and you should see the conflicts between >>>>> and <<<<<.
# Edit the file to whichever version you want, save and exit the editor. If you've done a merge with
# conflicts before, this should be familiar.
git add src/appendix-glossary.md
git rebase --continue
# You will probably get one more conflict. Do the same thing again.
# When you do git rebase --continue, this time you may get a warning about "no changes".
# If so, just do 
git rebase --skip
# It should complete now...

After all this, you should be able to do git log and see just your two new commits adding the definitions.

Let me know if you run into any hiccups. git can be tricky until you git the hang of it (pun intended).

@mark-i-m
Copy link
Member

mark-i-m commented Apr 9, 2018

Oh, one other thing. Rebasing rewrites history, so you will need to git push --force to get github to accept it. Make sure that your rebased branch looks correct (git diff master) before you do this, so as not to lose anything.

@mark-i-m mark-i-m added the waiting-on-author This PR is waiting for additional action by the OP label Apr 10, 2018
Samuel Wilson added 3 commits April 15, 2018 13:03
Definition referenced from Programming Rust by Jim Blandy & Jason
Orendorff, published by O'Reilly. Page 214: References to Slices
and Trait Objects.
Definition referenced from Programming Rust by Jim Blandy & Jason
Orendorff, published by O'Reilly. Page 214: References to Slices
and Trait Objects.

Double pointer and wide pointer both refer to fat pointer for
detail.
@samWson
Copy link
Author

samWson commented Apr 15, 2018

@mark-i-m Done. Your instructions were very helpful. Sorry for the delay, life got really busy for me last week. Let me know if there is anything more that needs doing.

@mark-i-m
Copy link
Member

No worries! Thanks for the PR :)

@mark-i-m mark-i-m merged commit a87b323 into rust-lang:master Apr 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author This PR is waiting for additional action by the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants