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

Added squash commits prompt to to-master #136

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added squash commits prompt to to-master #136

wants to merge 5 commits into from

Conversation

pgoodrich
Copy link

Updating to-master command to prompt user to squash commits before continuing with to-master. Hopefully this will reduce the number of cases where users forget to squash.

Note: Default editor used by interactive rebase may not launch properly on Windows machines.

@jdigger
Copy link
Owner

jdigger commented Apr 21, 2014

This could make sense if it detected that there's more than a small number of commits (likely 1, but being configurable to would be good). This is most useful if you usually rebase but just forgot and this acts as that backstop. Otherwise if it's too noisy everyone will simply turn it off.

@jdigger
Copy link
Owner

jdigger commented Apr 21, 2014

Test cases? What happens if the user cancels the rebase, there's an error, or a Ctl-C?

@pgoodrich
Copy link
Author

@jdigger I agree, I will update to check for the number of commits before prompting. Right now it's off by default anyway.

I will add test cases after I figure out why the editor is not launching. Wanted to ask you today why you thought that may be the case :)

@pgoodrich
Copy link
Author

@jdigger Added check for >1 commit before prompting. Seems to not work with VIM on Windows but it's working with NP++

If the user cancels the rebase, currently the to-master will continue. Do you think it should prompt again? An error will cancel the entire to-master process and a Ctrl-C will print an error regarding the interrupt on the next attempted command, but that's the same as interrupting the current to-master I believe.

I added test cases but I'm having trouble running them (about 95% of the test cases are failing on master when I run them)

@jdigger
Copy link
Owner

jdigger commented Apr 25, 2014

The test cases use Rugged, which requires at least Ruby 1.9.

@pgoodrich
Copy link
Author

@jdigger Using Ruby 1.9.3. Could it be a Windows issue?

@jdigger
Copy link
Owner

jdigger commented Apr 25, 2014

I guess? What's it saying? The last "master" build on Travis built fine, except for Ruby 1.8.

@jdigger
Copy link
Owner

jdigger commented Apr 25, 2014

You should sync with the latest on jdigger/git-process:master I just updated the .travis.yml file to not even try to build the tests with 1.8, so everything should be fully green.



def commits_since_master
Integer(gitlib.rev_list('origin/master','HEAD', :count => []))
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep the formatting consistent (2 spaces).

@pgoodrich
Copy link
Author

@jdigger @jharrell Finally was able to get back to this and fix the test cases.

@pgoodrich
Copy link
Author

@jdigger Do you have any further feedback?



def commits_since_master
Integer(gitlib.rev_list('origin/master','HEAD', :count => []))
Copy link
Owner

Choose a reason for hiding this comment

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

Why [] instead of simply true?

Copy link
Author

Choose a reason for hiding this comment

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

Passing no argument to the option, but I can change this to true.

@pgoodrich
Copy link
Author

@jdigger Updated

@ghowell15
Copy link

+1 Testing. One note, when it hits the interactive re-base if I don't set my default editor it seems to just hang but if I set it to an editor it works just fine. Believe you updated the documentation though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants