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

Remove "its" support as part of rspec-core issue #1083 #1095

Merged
merged 1 commit into from Oct 3, 2013
Merged

Remove "its" support as part of rspec-core issue #1083 #1095

merged 1 commit into from Oct 3, 2013

Conversation

palfvin
Copy link
Contributor

@palfvin palfvin commented Oct 3, 2013

This is step 2 of the 3-step process to extract "its" support to separate gem, per #1083

@coveralls
Copy link

Coverage Status

Coverage increased (+14.51%) when pulling 7d7974d on palfvin:master into 2ecfec8 on rspec:master.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Two commits with the same description?

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Needs a changelog entry but otherwise looks good to me,

@coveralls
Copy link

Coverage Status

Coverage increased (+14.51%) when pulling 0ff583c on palfvin:master into 2ecfec8 on rspec:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+14.51%) when pulling 066fb29 on palfvin:master into 2ecfec8 on rspec:master.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Could you squash this into 1/2 commits? (I'm ok with the changelog being one commit but the others need only be one commit)

@xaviershay
Copy link
Member

I typically include the changelog in the same commit as the actual change. Then if it gets reverted it disappears as well.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

We've never enforced a commit per PR rule, (just a sensible number per) so I'm not going to enforce that here, besides, can always revert the merge commit.

@palfvin
Copy link
Contributor Author

palfvin commented Oct 3, 2013

Guys, I don't know git well enough to do what you're asking without researching it. I can fork a new repository and submit a new pull request with a single commit. Would that be ok?

@xaviershay
Copy link
Member

Here's how:

git rebase -i HEAD~2
# Change second commit from "pick" to "s"
# Save and quit.
# You'll be prompted for a combined git message, edit that to taste.
git log -n 2 # Should see one commit which is yours, and one you based off (i.e. not yours)
git push origin master -f # Force push your master branch

Give it a try! If it doesn't work I can squash them for you.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Run git --rebase interactive on your branch and in your editor (:i if vim) replace all apart from one pick with squash so you end up with:

pick {hash} {message}
squash {hash} {message}
squash {hash} {message}
squash {hash} {message}
squash {hash} {message}
... (etc)

Save and quit whichever editor it uses (:wq if it's vim) then reword the commit message, and run git push origin master -f

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Or what @xaviershay said

@xaviershay
Copy link
Member

haha, race condition, I win :P

Yours is probably a better description though.

@palfvin
Copy link
Contributor Author

palfvin commented Oct 3, 2013

I was never prompted for the combined commit and got this output:

The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Did you follow my instructions or @xaviershay's? You actually do need to specify the commits to squash unforunately because you didn't branch off first. Either use git rebase --interactive HEAD~6 or git rebase --interactive origin/master

@palfvin
Copy link
Contributor Author

palfvin commented Oct 3, 2013

I followed yours. Here's what I saved from the first edit:

pick a93acb8 Remove its support
squash 7d7974d Remove its support
pick 066fb29 Update changelog

# Rebase a9c81cb..066fb29 onto a9c81cb
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
#

Here is the output:

The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'
# Not currently on any branch.
nothing to commit (working directory clean)
Could not apply a93acb8... Remove its support

Then I did git rebase --abort.
Then I did git rebase --interactive origin/master and was thrown into the
editor with the following:

noop

# Rebase 066fb29..066fb29 onto 066fb29
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
#

I don't know what to do next.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Can you pop into #rspec on irc.freenode.org?

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

If not the basic gist is you must have an empty commit or something, you can use git show {hash} to see what a commit contains, and you can use git log to show the hashes of your commits in your master.

@JonRowe
Copy link
Member

JonRowe commented Oct 3, 2013

Thanks a lot, I'll merge when Travis goes green

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) when pulling fa69345 on palfvin:master into 2ecfec8 on rspec:master.

JonRowe added a commit that referenced this pull request Oct 3, 2013
Remove "its" support as part of rspec-core issue #1083
@JonRowe JonRowe merged commit 8882ccc into rspec:master Oct 3, 2013
@palfvin
Copy link
Contributor Author

palfvin commented Oct 3, 2013

@JonRowe Thanks. Any idea why some of the code block markdown in my earlier comments didn't "take" (and some did)?

@JonRowe
Copy link
Member

JonRowe commented Oct 4, 2013

Nope, it's completly valid ( a new comment renders fine)

@palfvin
Copy link
Contributor Author

palfvin commented Oct 17, 2013

@JonRowe FYI, I contacted github support about why my markdown didn't work and they said it's not supported for comments posted via email.

@JonRowe
Copy link
Member

JonRowe commented Oct 17, 2013

Yeah I've noticed this a few times now (/cc @parndt)

@parndt
Copy link

parndt commented Oct 17, 2013

Yeah, it's a real frustration. You can't even edit the comment afterward on the UI! :)

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

5 participants