From: William Casarin <jb55@jb55.com>
To: Kyle Meyer <kyle@kyleam.com>
Cc: notmuch@notmuchmail.org
Subject: Re: Washing GitHub emails to include inline patch?
Date: Tue, 10 Oct 2017 18:45:14 -0700 [thread overview]
Message-ID: <87h8v6foet.fsf@jb55.com> (raw)
In-Reply-To: <87o9pebwyb.fsf@kyleam.com>
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
next prev parent reply other threads:[~2017-10-11 1:45 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-22 1:49 Washing GitHub emails to include inline patch? Kyle Meyer
2017-09-22 14:17 ` Tomi Ollila
2017-09-22 14:57 ` Kyle Meyer
2017-09-22 21:38 ` William Casarin
2017-10-10 17:35 ` William Casarin
2017-10-10 19:54 ` Kyle Meyer
2017-10-11 1:45 ` William Casarin [this message]
2017-10-11 3:51 ` Kyle Meyer
2017-10-11 4:38 ` William Casarin
2017-10-12 19:28 ` William Casarin
2017-10-13 5:24 ` Kyle Meyer
2017-10-13 6:03 ` William Casarin
-- strict thread matches above, loose matches on Subject: below --
2017-09-21 16:34 William Casarin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87h8v6foet.fsf@jb55.com \
--to=jb55@jb55.com \
--cc=kyle@kyleam.com \
--cc=notmuch@notmuchmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).