Washing GitHub emails to include inline patch?

Washing GitHub emails to include inline patch?

[William Casarin]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]


Hey team,

Most of the patches I review these days comes in as GitHub emails that
look like this:


[-- Attachment #2: Type: message/rfc822, Size: 660 bytes --]


  https://github.com/monstercat/iris/pull/52

-- Commit Summary --

  * Fix merge conflict putting location search into non-clean-ui iris

-- File Changes --

    M bin/js/main.js (66)
    M bin/js/users.js (71)
    M html/head.html (2)
    M templates/users.html (68)

-- Patch Links --

https://github.com/monstercat/iris/pull/52.patch
https://github.com/monstercat/iris/pull/52.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/monstercat/iris/pull/52

[-- Attachment #3: Type: text/plain, Size: 278 bytes --]



I wonder if it would be be possible to wash this email by downloading
the patch and present it inline like git-send-email. This would allow me
to review patches without having to click around the GitHub interface.
Has anyone done this?

Cheers,
William
 
-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin writes:

> Most of the patches I review these days comes in as GitHub emails that
> look like this:
>
>
> You can view, comment on, or merge this pull request online at:

[...]

> -- Patch Links --
>
> https://github.com/monstercat/iris/pull/52.patch
> https://github.com/monstercat/iris/pull/52.diff

[...]

> I wonder if it would be be possible to wash this email by downloading
> the patch and present it inline like git-send-email. This would allow me
> to review patches without having to click around the GitHub interface.
> Has anyone done this?

I have a command in my Emacs configuration that I think gets close to
what you want.

--8<---------------cut here---------------start------------->8---
(defun km/open-github-patch (buffer)
  "Find GitHub patch link in BUFFER and show it in a new buffer."
  (let ((url
         (with-current-buffer buffer
           (save-excursion
             (goto-char (point-min))
             (if (re-search-forward "https://github.com/.*\\.patch" nil t)
                 (match-string-no-properties 0)
               (user-error "No patch found"))))))
    (with-current-buffer (get-buffer-create
                          (generate-new-buffer-name "*mail-github-patch*"))
      (url-insert-file-contents url)
      (diff-mode)
      (view-mode 1)
      (pop-to-buffer (current-buffer)))))

(defun km/notmuch-show-open-github-patch ()
  "Open patch from GitHub email."
  (interactive)
  (with-current-notmuch-show-message
   (km/open-github-patch (current-buffer))))
--8<---------------cut here---------------end--------------->8---

The km/open-github-patch helper function exists because I made a slow
transition from gnus to notmuch and have a gnus variant that also calls
km/open-github-patch.  If km/notmuch-show-open-github-patch is the only
caller, there's not much point in having a separate helper function.

-- 
Kyle

Re: Washing GitHub emails to include inline patch?

[Tomi Ollila]

On Thu, Sep 21 2017, Kyle Meyer wrote:

>
> --8<---------------cut here---------------start------------->8---
> (defun km/open-github-patch (buffer)
>   "Find GitHub patch link in BUFFER and show it in a new buffer."
>   (let ((url
>          (with-current-buffer buffer
>            (save-excursion
>              (goto-char (point-min))
>              (if (re-search-forward "https://github.com/.*\\.patch" nil t)

Just a read-through thought -- would "https://github[.]com/.*[.]patch" work
above ?

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Thu, Sep 21 2017, Kyle Meyer wrote:
>
>>
>> --8<---------------cut here---------------start------------->8---
>> (defun km/open-github-patch (buffer)
>>   "Find GitHub patch link in BUFFER and show it in a new buffer."
>>   (let ((url
>>          (with-current-buffer buffer
>>            (save-excursion
>>              (goto-char (point-min))
>>              (if (re-search-forward "https://github.com/.*\\.patch" nil t)
>
> Just a read-through thought -- would "https://github[.]com/.*[.]patch" work
> above ?

oops, good catch.  I didn't mean to match any single character with the
first period.

Either your suggestion or "https://github\\.com/.*\\.patch" should work.

-- 
Kyle

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer <kyle@kyleam.com> writes:

> I have a command in my Emacs configuration that I think gets close to
> what you want.

This is great! Now if there was only a way to make it work for private
repositories... hmm...

-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Hey Kyle,

Kyle Meyer <kyle@kyleam.com> writes:
>> I wonder if it would be be possible to wash this email by downloading
>> the patch and present it inline like git-send-email. This would allow me
>> to review patches without having to click around the GitHub interface.
>> Has anyone done this?
>
> I have a command in my Emacs configuration that I think gets close to
> what you want.

I was wondering if you had any insight into what I'm thinking next. I
would love to view these patches via the way magit handles hunks. I
wonder if there was a way to get magit-style hunk browsing when viewing
a patch file with a series of commits.

Things like:

  * Jump to the next/previous hunk/commit in a patch series
  * Collapse hunks/commits with tab

You get this for free with mailed patches + notmuch, but dealing with
large patch series from GitHub is a bit annoying as it's one big buffer
with no way to jump between commits.

Do you know of any mode that would make this a reality or does it not
exist yet?

Cheers,
William

-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin <jb55@jb55.com> writes:

> I was wondering if you had any insight into what I'm thinking next. I
> would love to view these patches via the way magit handles hunks. I
> wonder if there was a way to get magit-style hunk browsing when viewing
> a patch file with a series of commits.
>
> Things like:
>
>   * Jump to the next/previous hunk/commit in a patch series

Diff mode has navigation commands to go to the next hunk/file, but not
the next commit.

>   * Collapse hunks/commits with tab

Hmm, yeah, I don't think Diff mode has commands for collapsing sections.
And I haven't checked, but I'd guess that Diff mode isn't really
recognizing the structure of the patch series; it's probably just
considering the next commit's header/message as context lines.

Magit doesn't have a mode for displaying a patch series.  Creating such
a mode shouldn't be too painful, at least if the command maps the patch
series to a local repository.

However, I personally haven't felt the need for such a command.  I
pretty frequently use the command I posted earlier in this thread to
take a quick look at PRs, but, for anything aside from the simplest
changes, I want to apply the commits locally to review/test.  If I
regularly review PRs for a GitHub repo, I have

        fetch = +refs/pull/*/head:refs/pull/origin/*

in the GitHub remote's configuration section of ".git/config".  (GitLab
has an analogous merge request namespace.)  Then, after fetching from
the GitHub remote, I can view the PR in Magit like I would any other
ref.

> You get this for free with mailed patches + notmuch, but dealing with
> large patch series from GitHub is a bit annoying as it's one big buffer
> with no way to jump between commits.

I share your preference for mailed patches, but using the process above,
I don't find *viewing* GitHub PRs annoying.  I do find *reviewing*
GitHub PRs annoying and tedious compared to reviewing patches on a
mailing list.  At the moment, I typically do the review/commenting
locally and at the end open a browser and add my inline comments.

Jonas recently got a Kickstarter funded [1], and one of his goals is to
support code review from within Magit [2].  Not sure how that will turn
out, but it seems more promising than my current strategy of hoping
everyone will start sharing my preference for mail-based collaboration :)


[1] https://www.kickstarter.com/projects/1681258897/its-magit-the-magical-git-client
[2] https://github.com/magit/magit/issues/2972

-- 
Kyle

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer <kyle@kyleam.com> writes:

> However, I personally haven't felt the need for such a command.  I
> pretty frequently use the command I posted earlier in this thread to
> take a quick look at PRs, but, for anything aside from the simplest
> changes, I want to apply the commits locally to review/test.  If I
> regularly review PRs for a GitHub repo, I have
>
>         fetch = +refs/pull/*/head:refs/pull/origin/*
>
> in the GitHub remote's configuration section of ".git/config".  (GitLab
> has an analogous merge request namespace.)  Then, after fetching from
> the GitHub remote, I can view the PR in Magit like I would any other
> ref.

Nice, I've been using magit-gh-pulls for this. It's a bit clunky though
so I might try this approach instead.

I now mainly review GitHub PRs via the patch fetching snippet you sent,
but there's a tedious disconnect between notmuch and magit. In the above
scenario, I would have to:

  1. cd to the project
  2. open magit
  2. fetch
  3. checkout the branch
  4. start reviewing

If I want to start a review on GitHub:

  5. go back to the mail buffer
  6. open the GitHub link
  7. try to find lines where I had a comment
  8. make comment, start review

Steps 1-4 are way too long for the number of code reviews I do at work
and on public projects. 

Perhaps I could write a script that quickly jump from steps 1 to 4. I
think this would still be too slow compared to simply fetching the patch
and having a nicer view, at least until we have full magit code reviews.
I guess I could just wait until that's finished.

> Jonas recently got a Kickstarter funded [1], and one of his goals is
> to support code review from within Magit [2]. Not sure how that will
> turn out, but it seems more promising than my current strategy of
> hoping everyone will start sharing my preference for mail-based
> collaboration :)

This is the dream. I have been following this as well. I hope Jonas can
pull it off.

Thanks for the tips!

Cheers,
William


-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin <jb55@jb55.com> writes:

> I now mainly review GitHub PRs via the patch fetching snippet you sent,
> but there's a tedious disconnect between notmuch and magit. In the above
> scenario, I would have to:
>
>   1. cd to the project
>   2. open magit
>   2. fetch
>   3. checkout the branch
>   4. start reviewing
>
> If I want to start a review on GitHub:
>
>   5. go back to the mail buffer
>   6. open the GitHub link
>   7. try to find lines where I had a comment
>   8. make comment, start review
>
> Steps 1-4 are way too long for the number of code reviews I do at work
> and on public projects.

Fair enough.  5-8 are my pain points.

> Perhaps I could write a script that quickly jump from steps 1 to 4. I
> think this would still be too slow compared to simply fetching the patch
> and having a nicer view, at least until we have full magit code reviews.
> I guess I could just wait until that's finished.

I think you could get 1-4 nearly as quick as calling the patch fetching
command.  How about something like this?

--8<---------------cut here---------------start------------->8---

(defun km/notmuch-github-pr-number ()
  "Return the PR number for this message."
  (let (pr)
    (with-current-notmuch-show-message
     (goto-char (point-min))
     (if (re-search-forward "https://github\\.com/.*/pull/\\([0-9]+\\)" nil t)
         (setq pr (match-string-no-properties 1))
       (user-error "Could not find PR number")))
    pr))

;; This function could be anything that figures out the project based
;; on the current notmuch message.  Or, if you use projectile and
;; don't mind getting queried each time, it could just read a project
;; from `projectile-relevant-known-projects'.
(defun km/notmuch-repo-from-message ()
  "Return the repository that this message is associated with."
  (let ((fname (notmuch-show-get-filename)))
    (or (and fname
             (string-match-p "magit-list" fname)
             "~/src/emacs/magit")
        (user-error "Could not determine repo"))))

(defun km/notmuch-visit-pr-in-magit (&optional dont-fetch)
  "Show the Magit log for this message's PR.
If DONT-FETCH is non-nil, do not fetch first."
  (interactive "P")
  (let* ((pr (km/notmuch-github-pr-number))
         (repo (km/notmuch-repo-from-message))
         (default-directory repo))
    ;; "origin" is hard-coded below, but it could of course be
    ;; anything.  You could also have an alist that maps repo ->
    ;; remote.
    ;;
    ;; This assumes that you've added
    ;;
    ;;    fetch = +refs/pull/*/head:refs/pull/origin/*
    ;;
    ;; to origin's in ".git/config".  You could drop that assumption
    ;; passing a more explicit refspec to the fetch call.
    (unless dont-fetch
      (magit-call-git "fetch" "origin"))
    (magit-log (list (concat "master..refs/pull/origin/" pr)))))
--8<---------------cut here---------------end--------------->8---

Call M-x km/notmuch-visit-pr-in-magit in a notmuch-show buffer for a
GitHub PR, modifying km/notmuch-repo-from-message so that it returns the
full path to the local repository.

-- 
Kyle

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer <kyle@kyleam.com> writes:

> Call M-x km/notmuch-visit-pr-in-magit in a notmuch-show buffer for a
> GitHub PR, modifying km/notmuch-repo-from-message so that it returns
> the full path to the local repository.

Are you a wizard? That worked like magit. I changed
km/notmuch-visit-pr-in-magit to parse the subject via
notmuch-show-get-subject and then went from there.

Was pretty quick too.

Going to try out this workflow more tomorrow.

Thanks!
William

-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[William Casarin]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

Kyle Meyer <kyle@kyleam.com> writes:

> --8<---------------cut here---------------start------------->8---
>
> (defun km/notmuch-github-pr-number ()
>   "Return the PR number for this message."
>   (let (pr)
>     (with-current-notmuch-show-message
>      (goto-char (point-min))
>      (if (re-search-forward "https://github\\.com/.*/pull/\\([0-9]+\\)" nil t)
>          (setq pr (match-string-no-properties 1))
>        (user-error "Could not find PR number")))
>     pr))
>
> ;; This function could be anything that figures out the project based
> ;; on the current notmuch message.  Or, if you use projectile and
> ;; don't mind getting queried each time, it could just read a project
> ;; from `projectile-relevant-known-projects'.
> (defun km/notmuch-repo-from-message ()
>   "Return the repository that this message is associated with."
>   (let ((fname (notmuch-show-get-filename)))
>     (or (and fname
>              (string-match-p "magit-list" fname)
>              "~/src/emacs/magit")
>         (user-error "Could not determine repo"))))
>
> (defun km/notmuch-visit-pr-in-magit (&optional dont-fetch)
>   "Show the Magit log for this message's PR.
> If DONT-FETCH is non-nil, do not fetch first."
>   (interactive "P")
>   (let* ((pr (km/notmuch-github-pr-number))
>          (repo (km/notmuch-repo-from-message))
>          (default-directory repo))
>     ;; "origin" is hard-coded below, but it could of course be
>     ;; anything.  You could also have an alist that maps repo ->
>     ;; remote.
>     ;;
>     ;; This assumes that you've added
>     ;;
>     ;;    fetch = +refs/pull/*/head:refs/pull/origin/*
>     ;;
>     ;; to origin's in ".git/config".  You could drop that assumption
>     ;; passing a more explicit refspec to the fetch call.
>     (unless dont-fetch
>       (magit-call-git "fetch" "origin"))
>     (magit-log (list (concat "master..refs/pull/origin/" pr)))))
> --8<---------------cut here---------------end--------------->8---

This worked pretty well, I've modified it a bit to support a generic
github repo location which is parsed from the subject, and origin/master
instead of master:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github.diff --]
[-- Type: text/x-patch, Size: 1095 bytes --]

--- kyle1.el	2017-10-12 12:22:01.977663605 -0700
+++ kyle2.el	2017-10-12 12:24:49.644673189 -0700
@@ -14,10 +14,12 @@
 ;; from `projectile-relevant-known-projects'.
 (defun km/notmuch-repo-from-message ()
   "Return the repository that this message is associated with."
-  (let ((fname (notmuch-show-get-filename)))
-    (or (and fname
-             (string-match-p "magit-list" fname)
-             "~/src/emacs/magit")
+  (let ((subject (notmuch-show-get-subject))
+        (repo))
+    (or (and subject
+             (or (and (string-match "\\([^\]]+\\)" subject 1)
+                      (setq repo (match-string-no-properties 1 subject))
+                      (concat "~/dev/github/" repo))))
         (user-error "Could not determine repo"))))
 
 (defun km/notmuch-visit-pr-in-magit (&optional dont-fetch)
@@ -39,4 +41,4 @@
     ;; passing a more explicit refspec to the fetch call.
     (unless dont-fetch
       (magit-call-git "fetch" "origin"))
-    (magit-log (list (concat "master..refs/pull/origin/" pr)))))
+    (magit-log (list (concat "origin/master..refs/pull/origin/" pr)))))

[-- Attachment #3: Type: text/plain, Size: 39 bytes --]


Cheers,
William

-- 
https://jb55.com

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin <jb55@jb55.com> writes:

> This worked pretty well, I've modified it a bit to support a generic
> github repo location which is parsed from the subject,

Great, glad you were able to tweak that test function into something
that works well with your setup.

> and origin/master instead of master:

Yep, that's better.

>  (defun km/notmuch-visit-pr-in-magit (&optional dont-fetch)
> @@ -39,4 +41,4 @@
>      ;; passing a more explicit refspec to the fetch call.
>      (unless dont-fetch
>        (magit-call-git "fetch" "origin"))

Looking at what I wrote again, I'd change DONT-FETCH to FORCE-FETCH and
then do something like

    (when (or force-fetch
              (not (magit-ref-exists-p local-ref)))
      (magit-call-git "fetch" "origin"))

where local-ref is bound to "refs/pull/origin/<pr>".  That way, "git
fetch" is only called if the ref doesn't already exist locally or when a
prefix argument is given, which would be useful for forced updates.

> -    (magit-log (list (concat "master..refs/pull/origin/" pr)))))
> +    (magit-log (list (concat "origin/master..refs/pull/origin/" pr)))))

Anyway, it's nice to see that you've been able to modify this into
something that might be useful to you.

-- 
Kyle

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer <kyle@kyleam.com> writes:

> Looking at what I wrote again, I'd change DONT-FETCH to FORCE-FETCH and
> then do something like
>
>     (when (or force-fetch
>               (not (magit-ref-exists-p local-ref)))
>       (magit-call-git "fetch" "origin"))
>
> where local-ref is bound to "refs/pull/origin/<pr>".  That way, "git
> fetch" is only called if the ref doesn't already exist locally or when a
> prefix argument is given, which would be useful for forced updates.

Oh, good call.

>> -    (magit-log (list (concat "master..refs/pull/origin/" pr)))))
>> +    (magit-log (list (concat "origin/master..refs/pull/origin/" pr)))))
>
> Anyway, it's nice to see that you've been able to modify this into
> something that might be useful to you.

Indeed, thanks again.

The last piece of the puzzle is the origin/master branch isn't always
the base branch it's merging into. I believe the proper way to do this
is like so. First we add another set of refs to fetch in our .git/config:

    [remote "origin"]
      fetch = +refs/pull/*/merge:refs/merge/origin/*

Now we can use this to get the base branch for the PR:

    refs/merge/origin/100^..refs/pull/origin/100

Which returns the proper set of commits.


Cheers,


-- 
https://jb55.com