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

Review v1.1.1 restoration commit before pushing #32

Closed
jdnavarro opened this issue Sep 16, 2015 · 7 comments
Closed

Review v1.1.1 restoration commit before pushing #32

jdnavarro opened this issue Sep 16, 2015 · 7 comments

Comments

@jdnavarro
Copy link
Collaborator

@feuerbach I hand picked the revert commits and squashed them all into a single commit which is directly fast-forwardable to feuerbach/master. You can check the range from where I picked the commits here.

It builds and tests successfully. I think I'm not discarding any non-experimental commit but please have a look before I push it to the main repo.

I'll open a new issue about partial functions and arrows linking to the old commits in case we try to implement them in the future.

After solving this issue, I don't think I'll require that much attention, but this was a tricky revert and wouldn't like to screw it all up with my first commit :)

@UnkindPartition
Copy link
Collaborator

I'd rather you used git rebase and simply removed the experimental commits (after saving them to a branch). That'd leave a clean history and make it much easier to review.

The same applies to pull requests. In your last pull request, you included commits that fix your older commits. No need for that, just rebase them and organize them logically, not historically.

@jdnavarro
Copy link
Collaborator Author

Rebasing a public master branch on a origin repo would break current forks on the next pull. They would have to manually remane their current master and sync it with the remote newly checked out branch. I also could make the default branch for this repo a new branch which then could be rebased safely, but having the name master for a non default branch would be awkward.

For the PR I agree with you, in that case rebasing makes sense to me.

@UnkindPartition
Copy link
Collaborator

Rebasing a public master branch on a origin repo would break current forks on the next pull.

This is a non-issue. If they need to stay on top of master, they need to rebase in either case; and if not, they'll cancel their pull.

@jdnavarro
Copy link
Collaborator Author

Sorry to disagree with you, but I do see this as an issue. Let me be clear, are you asking me to do this?

$ git clone feuerbach/smallcheck
$ cd smallcheck
$ git checkout -b experimental
$ git push origin experimental
$ git checkout master
$ git rebase -i v1.1.1 # Remove experimental commits
$ git push --force

@UnkindPartition
Copy link
Collaborator

Correct.

@jdnavarro
Copy link
Collaborator Author

Sorry, I won't do it. I give a crap about preserving history. I wouldn't do this with any of my repos, so I'm not going start with this one.

@UnkindPartition
Copy link
Collaborator

shrug

History is fun, but keeping the repo clean is more important.

If you rebase the branch, it'd be obvious to me whether you did it right or not. I'd just look at the set of commits in the branch and check that it's the set of commits that I expect to be there.

With your current branch, it isn't obvious to either of us whether you did the right thing. You have a bunch of commits that make the changes that shouldn't have been there in the first place, then another commit hopefully reverting it to the right state. How do you expect me to review that?

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

No branches or pull requests

2 participants