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

Revert "DOC: add vstack, hstack, dstack reference to stack documentation." #7196

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

shoyer
Copy link
Member

@shoyer shoyer commented Feb 5, 2016

Reverts #7191

cc @charris

@charris
Copy link
Member

charris commented Feb 5, 2016

Revert the merge. git revert -m 1 <sha_of_merge>. What you have is separate reversion of all related commits, hence all the tests.

@shoyer
Copy link
Member Author

shoyer commented Feb 5, 2016

I just used the interface on GitHub -- my git foo is weak! If you have a preferred way to do this, it would be great if you could do that yourself :)

@njsmith
Copy link
Member

njsmith commented Feb 5, 2016

@charris: not sure I follow, and want to make sure I understand so I don't mess it up myself at some point :-). This pull request seems to be just a single clean patch reverting the doc changes, which is what we want, right? What's the advantage of reverting the merge commit instead?

@charris
Copy link
Member

charris commented Feb 5, 2016

@njsmith If you just revert the merge the unattached commits are eventually removed. What the revert button does is revert all the commits associated with the merge and each reverted commit gets tested. For instance, if there were ten commits in the PR you would see 11 Travis tests run. It isn't wrong, just ugly ;)

I learned this the same way as @shoyer, by hitting the revert button to see what happened.

@charris
Copy link
Member

charris commented Feb 5, 2016

OK, I'm going to close and reopen to see if that works on top of current master.

@charris charris closed this Feb 5, 2016
@charris charris reopened this Feb 5, 2016
@charris
Copy link
Member

charris commented Feb 5, 2016

I'm going to go ahead an merge this. The merge should get tested later.

charris added a commit that referenced this pull request Feb 5, 2016
Revert "DOC: add vstack, hstack, dstack reference to stack documentation."
@charris charris merged commit 5002e81 into master Feb 5, 2016
@charris
Copy link
Member

charris commented Feb 5, 2016

@shoyer Reverted, thanks.

@njsmith njsmith deleted the revert-7191-stack_documentation branch February 5, 2016 23:28
@njsmith
Copy link
Member

njsmith commented Feb 5, 2016

Okay, so to sum up, I think there are two things that the github "Revert" button does that are somewhat annoying:

  1. If there are N commits in a pull request, then it creates N revert commits, instead of a single rolled-up revert commit.
  2. It puts these commits on a new branch inside the numpy/numpy repository, instead of inside someone's personal fork. This clogs up the travis queue (because travis tests everything committed into the numpy/numpy repository, regardless of which branch it's on), and it clutters up our branch namespace with branches we don't care about. (I just deleted this particular branch, but that's an annoying extra step.)

@charris
Copy link
Member

charris commented Feb 6, 2016

@njsmith Ha, I didn't notice the new branch, wondered why I got a commit notice from github. It may be that the reason the tests doubled was that both the branch and the PR were being tested rather than a lack of rolled up commits.

@njsmith
Copy link
Member

njsmith commented Feb 6, 2016

@charris: it could easily be both working together to create a perfect storm of fail: if there were 10 commits being reverted, then travis sees 10 commits in quick sequence to a new branch, starts testing all 10 of them in parallel, and then sees a pull request posted and starts testing that as well.

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