Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer  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/".  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin  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 ( 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/".  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer  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 ( 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:

--- 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 ( 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)

Cheers,
William

-- 
https://jb55.com
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin  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 ( 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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

William Casarin  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Hey Kyle,

Kyle Meyer  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[William Casarin]

Kyle Meyer  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: Washing GitHub emails to include inline patch?

[Kyle Meyer]

Tomi Ollila  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

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 ?
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Washing GitHub emails to include inline patch?

[William Casarin]

Hey team,

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

--- Begin Message ---
You can view, comment on, or merge this pull request online at:
MIME-Version: 1.0
Content-Type: text/plain

  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
--- End Message ---


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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch