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: Hash trait for WatchDescriptor #57

Merged
merged 1 commit into from Jul 26, 2017

Conversation

arsalan86
Copy link
Contributor

I've made a new PR after breaking the previous one due to a clunky rebase/squash.

I've added the Hash to the WatchDescriptor struct, to allow it to be used for HashMap lookup.

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I've added a single comment. Once that is addressed, this is ready to merge.

If you don't have time or don't want to bother right now, let me know. I can make the change myself later.

@@ -33,6 +33,7 @@ use std::os::unix::ffi::OsStrExt;
use std::os::unix::io::RawFd;
use std::path::Path;
use std::slice;
use std::collections::HashMap;
Copy link
Owner

Choose a reason for hiding this comment

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

You should be able to remove this. Deriving traits shouldn't require any imports.

The compiler also warns about this import being unused.

@arsalan86
Copy link
Contributor Author

I've implemented the requested changes, thanks!

@hannobraun
Copy link
Owner

Can you squash the commits into a single one? That way it's easier to read later. If you're unsure about how to do that, something like git rebase -i origin/master should get you on the way.

Again, if you don't have time or can't be bothered, let me know. I can do it later, no problem. Thanks!

@arsalan86
Copy link
Contributor Author

Done!

@hannobraun
Copy link
Owner

Hmm, still seeing two commits. Maybe you forgot to push the branch?

@arsalan86
Copy link
Contributor Author

It's at 4 commits now, I think I'm doing something wrong! I'm sorry!

@hannobraun
Copy link
Owner

No worries! Git can be quite complicated if you're not used to it. That third commit is really weird. It just repeats the change from the third commit, which shouldn't really be possible. Oh well :-)

I'm happy to take care of this for you, but if you want to figure out how to solve this yourself, here's what I would try:

  1. First I would make a second branch to experiment on, so you don't accidentally lose the commits. Like this: git checkout -b backup, where backup can be any valid branch name. That command creates the branch backup and switches over to it.
  2. Then I would get rid of the last two commits, like this: git reset --hard HEAD~2. This resets the branch to your second commit, essentially removing the last two commits from the branch. This can be quite dangerous, that's why we made the backup branch earlier. See [1] for more explanation of HEAD~2.
  3. You should be left with the first two commits now. Run git rebase -i origin/master or git rebase -i HEAD~2. Unless I've made a mistake, both origin/master and HEAD~2 should point to the same commit at this point. This should open a text editor which should show your last two commits.
  4. In the text editor for the interactive rebase, locate the line of your second commit. In that line, replace pick with fixup. Then save and exit the editor. If everything goes right, Git should now merge the second commit into the first one.
  5. You should be left with a single commit. You can push that to your repository now. However, since that push would change an existing commit and delete others, you have to add the -f option to the git push command.

As I said, I'm happy to do it for you. I just figured you might want some pointer on how to solve such problems with Git in the future. Let me know if you want me to do it, or if you have any questions!

[1] HEAD always refers to the last commit of the current branch. HEAD~1 is the commit before that, and HEAD~2 is the commit before that one. So by resetting to HEAD~2, the last two commits are removed from the branch.

@arsalan86
Copy link
Contributor Author

Done!

Thanks for the git help!

I'm quite new to git/github, and still figuring things out. I will take some time out to go through the man pages.

Thank you for your patience.

@hannobraun hannobraun merged commit 8cc6601 into hannobraun:master Jul 26, 2017
@hannobraun
Copy link
Owner

You're welcome, and thanks again for the pull request!

And regarding Git, it's actually quite elegant under the surface. The only problem is that the elegance is hidden under a thick layer of weirdness :-)

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

2 participants