Email: Add option to show commit diffs in the emails #275
Conversation
|
I'm not totally opposed to this but we need to handle cases like extremely large diffs, timeouts, etc. |
| @@ -131,6 +131,24 @@ def commit_text(commit) | |||
|
|
|||
| EOH | |||
|
|
|||
| if data['show_diff'] | |||
| patch_resp = http_get commit['url'] + ".diff" # Why doesn't this use accept headers? | |||
There was a problem hiding this comment.
Accept header should work as well here. Either Accept: text/plain+diff or using the .diff extension should work equally well. We don't only use Accept because there's no way to link to a content negotiated resource.
There was a problem hiding this comment.
Hmmm. I can't get it to work:
curl -H 'Accept: text/plain+diff' -D- https://github.com/mojombo/grit/commit/06f63b43050935962f84fe54473a7c5de7977325 | grep Status
gives me:
Status: 406 Not Acceptable
There was a problem hiding this comment.
Sorry, I checked and it looks like the content negotiation was removed recently. The explicit .diff and .patch URLs are the only way to access this content.
There was a problem hiding this comment.
I am getting an error "Status 406" using the github e-mail service hook on my repositories with embedded diffs. Did something here get pushed that shouldn't have? Or is it unluckily unrelated?
There was a problem hiding this comment.
On 4/24/12 2:55 PM, Lucas Madar wrote:
@@ -131,6 +131,24 @@ def commit_text(commit)
EOH
- if data['show_diff']
patch_resp = http_get commit['url'] + ".diff" # Why doesn't this use accept headers?I am getting an error "Status 406" using the github e-mail service hook on my repositories with embedded diffs. Did something here get pushed that shouldn't have? Or is it unluckily unrelated?
I might be related but I'm not sure. I originally tested with curl and
noticed that github didn't honor the "Accept" header and, in fact,
returned "406 Not Acceptable" (which is what you are getting) when I
passed in anything other than "text/html". But in the end I didn't set
the "Accept" header, so I don't understand why you would be seeing that.
There's a little bit of discussion of this section of the code in the
pull request here:
#275 (comment)
For an example, check out this:
https://github.com/github/github-services/pull/275/commits
vs.
https://github.com/github/github-services/pull/275/commits.json
-David
|
Good point. Do you think just limiting it to a certain size would be reasonable? 100K? Do you mean timeouts on the fetch of the diff from github? I was assuming that stuff was built into the Faraday lib code. Right now it raises if something goes wrong. Do you think it should instead just skip it and send the email without diffs? |
It's possible. We should verify that's the case though. Or possibly dial down the timeout to something like < 5s.
Yeah exactly. If the diff can't be fetched for any reason it should fallback to sending the commit information and noting that the diff could not be obtained. |
|
There are 2 more commits here. I added a timeout on the HTTP requests and made it so that if any exceptions get raised there's an error message put in the email in place of the diff (the rest of the email prints normally). |
|
We're using net/http in production, so that's the timeout code that's being used as well. |
Email: Add option to show commit diffs in the emails
|
@caldwell this is radical. I didn't see a diff option at Service Hooks > Email on our organization's paid GitHub-hosted repo. Do you know when I could expect this to hit github.com or where I can track that status? We've been waiting for this for a while! |
|
On 5/18/12 4:30 PM, Rob Barreca wrote:
I'm glad you like the idea. I saw the feature enabled on github a couple -David Caldwell david@porkrind.org |
|
We removed it because it didn't work on private repositories and caused a lot of confusion. |
|
I need this feature badly! |
|
@technoweenie can the issue with private repo's be resolved? I also have been wanting this feature in the email hook for a long time. |
|
We just migrated from a self-hosted Git repo to Github, and this is the only feature we're really missing. |
|
+1 |
|
+1 We just moved from an svn host that had this feature to github and this is a feature we sorely miss. It might be missed enough to move back :/ |
|
+1 -- especially for private repos |
|
We're not adding this at this time. This approach in the pull request is problematic. |
For our use case, I only care about sending commit messages upon receiving Are there any third-party services that provide such commit-hook email Ideally, I'd like to integrate it with something like cyrus shared imap jrf |
|
Not that I know of. Special cases and workflows are exactly why we have open APIs and hooks. If you have more feedback on this, send it to support@github.com. Thanks. |
|
It's been a year and this is still not fixed. There are already external services which do just this (gitdub). Stackoverflow is full of questions about how to solve this. Thank you, github team, for ignoring your users and customers. |
|
Being a long-term kind-of-happy bitbucket user i wanted to give github a more serious try because so many people are raving about it. About the first thing i did after convert+import was looking for enabling email diffs on push. Quite irritating that such a basic service is not there out-of-the-box. |
|
We switched entirely to gitolite on our own servers -- motivated largely by this issue. |
|
Note, that the diffs are there, github does generate them and stores permanently. To get a fine email for this commit, you can simply go to The current email hook contains a link to the commit, so all the hook needs to be doing is actually giving people the diff instead of the link. It's infuriating how Github can be so deaf to this. |
|
After many user requests and even user contributions, 2 years went by and Github is still not offering email with diff. While the Atlassian folks are on top of that: From a once enthusiastic but now unhappy paying Github user. Very sad.... |
These patches add an option that puts the diff for each commit right into the email. It gets the diffs from github.com by adding ".diff" to the commit URL.
Rationale: I love getting email when new stuff is pushed to projects I'm involved with. I especially love when I can get diffs sent for every commit. This allows me to quickly review and stay on top of what's going on in the project. Currently I've been using github's RSS feeds but they require a lot of clicking around to get to the meat of the commits (the patch itself). Also it's not good for offline reviews (on plane flights, etc.). Email, despite being old-school, is remarkably adept at offline viewing and quick perusal.
-David