add git done #135

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

calebthompson commented Apr 12, 2013

Run this when your Pull Request has been accepted and you're ready to merge
into master.

  • Fetches and rebases your branch (interactively) onto origin/master
  • Pushes up the new changes into your branch so that the Pull Request will be
    closed automatically.
  • Merges the changes into your local master so that you get any upstream
    changes as well as the changes from your branch.
  • Pushes to origin
  • Deletes your branch on origin and master
@calebthompson calebthompson add git done
Run this when your Pull Request has been accepted and you're ready to merge
into master.

* Fetches and rebases your branch (interactively) onto origin/master
* Pushes up the new changes into your branch so that the Pull Request will be
  closed automatically.
* Merges the changes into your local master so that you get any upstream
  changes as well as the changes from your branch.
* Pushes to origin
* Deletes your branch on origin and master
527b362

@croaky croaky commented on the diff Apr 12, 2013

bin/git-done
+# * Merges the changes into your local master so that you get any upstream
+# changes as well as the changes from your branch.
+# * Pushes to origin
+# * Deletes your branch on origin and master
+#
+
+set -e
+
+successfully() {
+ $* || (echo "failed" 1>&2 && exit 1)
+}
+
+branch=`git current`
+
+successfully git fetch
+successfully git rebase -i origin/master
@croaky

croaky Apr 12, 2013

Owner

If this brings in new changes, we should running the specs.

@calebthompson

calebthompson Apr 12, 2013

Contributor

That's a really good point.

Unfortunately, I think it completely defeats the purpose of the script.

  1. This isn't worthwhile if we aren't pulling in the new changes
  2. The time saving is negated by often-hour-long test suites.

I'm going to go ahead and close this.

Owner

croaky commented Apr 12, 2013

We have three helpers already that do this:

https://github.com/thoughtbot/dotfiles/blob/master/gitconfig#L13-L15

If this gets merged, do we want to delete those?

I think I prefer the smaller aliases so we stop and run specs if the rebasing pulls something in, or stop and do a git log origin/master..HEAD after merging to double-check there's not a merge conflict, etc.

Owner

jferris commented Apr 12, 2013

It might be nice to have some of the helpers combined into larger steps. Once a feature is reviewed and ready to merge, you want to:

  1. Get it up to date and re-push your branch
  2. Verify that everything is working (rake)
  3. Merge, push, delete, close pull request

Steps 1 and 3 are currently multiple steps but don't need to be. Thoughts?

jferris reopened this Apr 12, 2013

@gylaz gylaz commented on the diff Apr 12, 2013

bin/git-done
+# * Pushes to origin
+# * Deletes your branch on origin and master
+#
+
+set -e
+
+successfully() {
+ $* || (echo "failed" 1>&2 && exit 1)
+}
+
+branch=`git current`
+
+successfully git fetch
+successfully git rebase -i origin/master
+git diff master --check || successfully git diff master --check
+successfully git push -f
@gylaz

gylaz Apr 12, 2013

Owner

Not a fan of something implicitly doing a force push.

@calebthompson

calebthompson Apr 12, 2013

Contributor

It isn't avoidable if we are going to rebase onto master.

One thing that would be good would be to add a check to see if we have diverged from origin/$branch and exit with an error message if that is the case.

That, in combination with successfully and set -e would make me feel pretty safe about force pushing.

@mcmire

mcmire Jun 16, 2013

Contributor

So the rest of this script actually I'm okay with, except for this little part. In the same way that I don't force push a branch while the PR is open, I don't force push it after rebasing, because it tends to screw up the associated PR and therefore the history associated with it. Do people actually do this?

@calebthompson

calebthompson Jun 16, 2013

Contributor

Many times as part of a pull request process. If people aren't commenting on commits, nothing is lost. Diff comments and pr comments are preserved.

@croaky

croaky Jun 16, 2013

Owner

Can't the push to the branch (force or otherwise) be skipped in the script? I typically don't do that as part of my manual git process. The docs here say that step is to automatically close the pull request but I thought deleting the feature branch covered that.

@calebthompson

calebthompson Jun 20, 2013

Contributor

It is marked as "closed" that way, but not as "merged".

@calebthompson

calebthompson Jun 20, 2013

Contributor

Without updating after a rebase:

2013-06-20 at 2 09 10 pm

With git-done:

2013-06-20 at 2 09 51 pm

@gylaz gylaz commented on the diff Apr 12, 2013

bin/git-done
+# * Fetches and rebases your branch (interactively) onto origin/master
+# * Pushes up the new changes into your branch so that the Pull Request will be
+# closed automatically.
+# * Merges the changes into your local master so that you get any upstream
+# changes as well as the changes from your branch.
+# * Pushes to origin
+# * Deletes your branch on origin and master
+#
+
+set -e
+
+successfully() {
+ $* || (echo "failed" 1>&2 && exit 1)
+}
+
+branch=`git current`
@gylaz

gylaz Apr 12, 2013

Owner

git current doesn't work on my machine. Is it a git command?

@calebthompson

calebthompson Apr 12, 2013

Contributor

Oh, crap. I have an alias for this and didn't bring it over with the script here. Good catch.

@mike-burns

mike-burns Apr 22, 2013

Owner

Instead of adding another alias in this PR, how about inlining the alias?

@gylaz gylaz commented on the diff Apr 12, 2013

bin/git-done
+# changes as well as the changes from your branch.
+# * Pushes to origin
+# * Deletes your branch on origin and master
+#
+
+set -e
+
+successfully() {
+ $* || (echo "failed" 1>&2 && exit 1)
+}
+
+branch=`git current`
+
+successfully git fetch
+successfully git rebase -i origin/master
+git diff master --check || successfully git diff master --check
@gylaz

gylaz Apr 12, 2013

Owner

What is the purpose of the || operator in this case?

@calebthompson

calebthompson Apr 12, 2013

Contributor

successfully eats error output, which git diff --check provides when there are whitespace issues.

I'm not a huge fan of this implementation, but wasn't able to come up with another way to exit gracefully while providing the output I wanted.

To be honest, I didn't put too much effort into that, as my vimrc will strip all of the whitespace errors I run into on write, so I very rarely run into this.

Contributor

mcmire commented May 13, 2013

Going along with Joe, could we just limit git-done to the following?

successfully git checkout master
successfully git merge $branch --ff-only
successfully git push
successfully git push origin --delete $branch
successfully git branch -d $branch

This way it's up to you to rebase your branch against master, run tests, do what you need to do; oftentimes this is a manual process anyway. But, the several steps it takes to merge stuff back is pretty much the same every time and can easily be automated.

@jferris jferris commented on the diff Jun 10, 2013

bin/git-done
@@ -0,0 +1,33 @@
+#!/usr/bin/env zsh
+#
+# Credit: Caleb Thompson
@jferris

jferris Jun 10, 2013

Owner

We don't generally do this.

Although I don't think it's particularly obtrusive here, we can't reasonably have "Credit" for every contribution without having tons of cluttered comments. I've prefer to allow the Git log to do the talking.

@calebthompson

calebthompson Jun 12, 2013

Contributor

I'm happy to remove it, but I was trying to keep symmetry with the other git script, which does have credits. https://github.com/thoughtbot/dotfiles/blob/master/bin/git-churn

Owner

jferris commented Jun 10, 2013

@calebthompson any news on this? I still think we could use something like what @mcmire recommended for the post-review, post-tests workflow.

@mike-burns mike-burns commented on the diff Jun 10, 2013

bin/git-done
+#
+# Credit: Caleb Thompson
+#
+# Run this when your Pull Request has been accepted and you're ready to merge
+# into master.
+#
+# * Fetches and rebases your branch (interactively) onto origin/master
+# * Pushes up the new changes into your branch so that the Pull Request will be
+# closed automatically.
+# * Merges the changes into your local master so that you get any upstream
+# changes as well as the changes from your branch.
+# * Pushes to origin
+# * Deletes your branch on origin and master
+#
+
+set -e
@mike-burns

mike-burns Jun 10, 2013

Owner

How does this work with the successfully function?

@calebthompson

calebthompson Jun 12, 2013

Contributor

I'm not sure what the question is. Both seem to be needed to abort the process when any part of it fails.

@mike-burns

mike-burns Jun 12, 2013

Owner

Aha, you're using this in place of a && between commands. Got it.

Not thrilled with that, but the && would also be ugly. Which is easier for people to maintain?

@calebthompson

calebthompson Jun 12, 2013

Contributor

This idea was pulled from the laptop setup script, for what that's worth. https://github.com/thoughtbot/laptop/blob/master/mac

@pbrisbin

pbrisbin Jun 12, 2013

set -e causes the shell to exit if any command returns non-zero. successfully does the same but adds a "failed" output. set -e is perfect for these sort of scripts, I use it in pretty much all scripts. succesfully feels a bit redundant (IMHO).

@mike-burns mike-burns commented on the diff Jun 10, 2013

bin/git-done
@@ -0,0 +1,33 @@
+#!/usr/bin/env zsh
@mike-burns

mike-burns Jun 10, 2013

Owner

Seems like you could use /bin/sh instead. A performance gain and a portability gain.

Contributor

calebthompson commented Jun 12, 2013

@mcmire I've been using this as-is for a few months now and have never run into an issue with rebases breaking the build. Developers who would benefit fron this process should be rebasing onto master regularly to keep up to date while developing (see best practices), so in most cases this inserts at most one or two changes on master.

If I think the changes might not mesh well, I rebase first without pushing and run the tests. git-done then works fine to push and close, even though it needs to rerebase. This is especially useful when closing a pull that has been in the queue for a while.

It has been really useful for things that I know won't break tests, such as adding rake tasks or CSS changes that don't effect visibility to have this all in one step. I recommend trying it out before we merge.

Contributor

calebthompson commented Jul 5, 2013

Closing for inactivity and general disinterest. If somebody want to take point, feel free to reopen.

teoljungberg deleted the ct-git-done branch Nov 14, 2015

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