Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

Michael Haggerty [off-list ref] writes:

Is it *really* so insane to consider moving collaboration on the Git
project to GitHub or some other similar platform?

I only know how "pull requests" and "issues" discussion in GitHub
Web interface _currently_ looks like, so if you have even more
wonderful thing in the works, I _might_ be swayed otherwise, but I
do not think it is sane to expect that the same quality and quantity
of reviews as we do here can be maintained with that thing.

On 08/09/2016 12:36 AM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

Is it *really* so insane to consider moving collaboration on the Git
project to GitHub or some other similar platform?

I only know how "pull requests" and "issues" discussion in GitHub
Web interface _currently_ looks like, so if you have even more
wonderful thing in the works, I _might_ be swayed otherwise,

If I did I couldn't say anyway, so let's assume that current GitHub is
what's on the table [1].

There are a couple of recent code-review improvements that you might
have missed:

* You can now get email updates about your own activity [2]. (Previously
you would only get emails about the activity of other people, which
would leave holes in the email record of the conversation.)

* There is also now better visibility of code review comments regarding
lines that are no longer part of a PR [3].

but I
do not think it is sane to expect that the same quality and quantity
of reviews as we do here can be maintained with that thing.

Could you elaborate why you would expect quality and/or quantity of
reviews to suffer? I'm really curious, and I'd be happy to pass your
feedback along to my colleagues.

Here are some factors that I think will *improve* reviews:

* While you are reviewing patches, you can "zoom out" to see code beyond
the usual diff context. Currently a reviewer who wants more context has
to transition from reading the diff in email to applying the patch and
viewing it in another tool. Then the reviewer has to go back to email to
leave the comment.

* If you want to compile/run/edit/profile the code, you just need to
"git fetch" rather than messing around with "git am". For more involved
suggestions, it is possible to propose a PR against the original PR.

* It is easy to summon somebody else into the review conversation by
@-mentioning them. That person immediately can see the whole history of
the PR. (This is an improvement on the status quo, where a new entrant
to a conversation might have to dig through the email trash or an email
archive to see emails that they overlooked before they were added to the
CC list.)

* It is easy to subscribe/unsubscribe from particular discussions [4].
This makes it easier to follow the discussions you are interested in
without getting swamped with emails about other discussions. You can
unsubscribe from a discussion permanently, or in such a way that a new
@-mention brings you back in.

* It is easy to mention other PRs/commits/issues in a discussion, and
those mentions become clickable links (no jumping back and forth between
email client and web browser). Of course you can also link to arbitrary
URLs (e.g., mailing list archives).

* It is possible to search old issues and PRs for additional context.
(Granted, the history of the project from its ML era would have to be
searched separately.)

Given that I work for GitHub, I'm uncomfortable doing any more advocacy
here. If people have concrete questions, I'd be happy to answer them, on
the list or in private.

Michael

[1] In general, GitHub *does* get better over time, and we would benefit
from any future improvements.
[2] https://github.com/blog/2203-email-updates-about-your-own-activity
[3] https://github.com/blog/2123-more-code-review-tools
[4] https://github.com/blog/2183-improvements-to-notification-emails

Michael Haggerty venit, vidit, dixit 09.08.2016 01:20:

Given that I work for GitHub, I'm uncomfortable doing any more advocacy
here. If people have concrete questions, I'd be happy to answer them, on
the list or in private.

You're doing a great job differentiating between your roles as a member
of the Git devel community and as a GitHub employee, so please keep the
discussion here.

Maybe two more points of input for the discussion:

off-line capabilities
=====================

The current workflow here seems to work best when you are subscribed to
the git-ml and have your own (local, maybe selective) copy of git-ml in
your (text-based) MUA (mutt, (al)pine, emacs, ...) that can jump right
into git-am and such directly. I'm not sure how important the "off-line"
aspect of that is for some of us, and how that could be replicated on
GitHub - while PRs and such may be Git based behind the scenes there
seems to be no way to clone that info and work from a local clone.
(Don't know if GitLab is more open.)

My own setup
============

My usual MUA is Thunderbird because of its integration with calendars
and address books. I usually read and post to mailing lists via
nntp/gmane, that works best for me for several reasons (e.g. archive
available when I need it).

For git-ml, I had to learn early on to answer by e-mail to git-ml rather
than by nntp-reply because proper nntp-replies somehow didn't meet the
expectations of the e-mail users (double copies because of the cc-policy
or such, I don't remember).

I use git sendemail even for small throw-in patches because the git-ml
does not allow attachments but wants patches (files) as in-line text,
and Thunderbird possibly reformats text (because it's text, you know).

When I want to try out a proposed patch I have to "save message" and run
git-am because patches don't come as file attachments on the git-ml
(can't use "save/open attachment"+ git-apply) nor a PR (can't use git
fetch nor view in browser). If it's a series, I have to do that for each
invididual patch, which usually means I simply don't (or rely on Junio
doing it and fetch his xy/topic branch).

And more often than not, patches from series do not appear in sequence,
not threaded on top of the cover letter (in the gmane nntp version of
git-ml), and it usually happens for the same people again and again,
which tells me it's a git sendemail config issue and not gmane.

So really, everytime I interact with the git-ml I think about switching
to mutt or such just for git-ml, even though over time I have gotten
used to the number of hoops that I have to jump through if I want to
interact with git-ml.

Suggestion
==========

Maybe the current gmane woes are a good reason to try out something
different for a month or two, with copies to the git-ml, and with the
default being to revert back to git-ml after that and discuss what we've
learned. As a result we may improve our workflow here, get GitHub to
improve, and maybe switch or not. Either way we could profit from that.

Michael

On Tue, Aug 09, 2016 at 10:11:30AM +0200, Michael J Gruber wrote:

Maybe two more points of input for the discussion:

off-line capabilities
=====================

The current workflow here seems to work best when you are subscribed to
the git-ml and have your own (local, maybe selective) copy of git-ml in
your (text-based) MUA (mutt, (al)pine, emacs, ...) that can jump right
into git-am and such directly. I'm not sure how important the "off-line"
aspect of that is for some of us, and how that could be replicated on
GitHub - while PRs and such may be Git based behind the scenes there
seems to be no way to clone that info and work from a local clone.
(Don't know if GitLab is more open.)

You can pull it all out via GitHub's HTTP API, but the question is what
format you would use to store it locally (and which tools you would then
use to play with it).

I haven't really tried this lately, though, so I don't know if there is
information that the API would be missing.

I do have a dream of writing a tool that sucks in GitHub PRs to a fake
email thread, lets me make my responses inline in an editor, and then
pushes it back up as PR comments (finding the right positions based on
the quoted context).

For git-ml, I had to learn early on to answer by e-mail to git-ml rather
than by nntp-reply because proper nntp-replies somehow didn't meet the
expectations of the e-mail users (double copies because of the cc-policy
or such, I don't remember).

At least some people's workflows seem to send two copies to the list.
For instance, Jakub's [off-list ref]
got delivered to me via the list twice. Once directly from gmail with:

  To: Oleg Taranenko [off-list ref],
      Junio C Hamano [off-list ref]
  Cc: git@vger.kernel.org

and once via gmane with:

  To: git@vger.kernel.org
  Cc: git@vger.kernel.org

It's like this with all of his messages (sorry I can't point to the
duplicates in an archive; they have the same message-id, so public-inbox
treats them as a single unit).

Replying to the second one breaks the usual "cc-everybody" rule. Sending
duplicates means everybody sees it twice (3 times if they're on the cc
list!), and the second copy still has the bogus headers (so people
replying need to pick the right one).

I use git sendemail even for small throw-in patches because the git-ml
does not allow attachments but wants patches (files) as in-line text,
and Thunderbird possibly reformats text (because it's text, you know).

I wonder if this is something we could change. I do not personally have
any problem with attached patches. "git am" knows how to apply them, and
mutt is smart enough to show text/* by default, and to include it in
quoted text on reply. So the output of "git format-patch --attach" works
fine for me. But it may not be as nice in other MUAs, and we have to
care about all of the other reviewers.

When I want to try out a proposed patch I have to "save message" and run
git-am because patches don't come as file attachments on the git-ml
(can't use "save/open attachment"+ git-apply) nor a PR (can't use git
fetch nor view in browser). If it's a series, I have to do that for each
invididual patch, which usually means I simply don't (or rely on Junio
doing it and fetch his xy/topic branch).

So you would like the opposite of my dream tool, I think: something that
takes mailing list conversations and turns them into PRs.

(My real dream is actually to have a bidirectional version of the tool,
so that everybody can use whatever interface they like, and nobody has
to care about somebody else's preferences).

And more often than not, patches from series do not appear in sequence,
not threaded on top of the cover letter (in the gmane nntp version of
git-ml), and it usually happens for the same people again and again,
which tells me it's a git sendemail config issue and not gmane.

Just a guess, but I suspect this is caused by people who use "rebase -i"
to rearrange patches. When format-patch writes out the patches, it uses
the author date as the "Date" field, which means it may be out of order.
I think send-email will always write out a new, monotonically increasing
date. But I suspect other workflows (e.g., imap-send and then mailing
from a MUA) blindly re-use that date.

Suggestion
==========

Maybe the current gmane woes are a good reason to try out something
different for a month or two, with copies to the git-ml, and with the
default being to revert back to git-ml after that and discuss what we've
learned. As a result we may improve our workflow here, get GitHub to
improve, and maybe switch or not. Either way we could profit from that.

I think public-inbox is a nice step forward on the reading side (it's a
lot easier to get raw patches out of it, for example). But it doesn't
help much with sending (and sending is a tricky subject; anytime you
promise to send mail on behalf of somebody, you're going to attract
spammers).

-Peff

On Tue, Aug 09, 2016 at 01:20:05AM +0200, Michael Haggerty wrote:

quoted

but I
do not think it is sane to expect that the same quality and quantity
of reviews as we do here can be maintained with that thing.

Could you elaborate why you would expect quality and/or quantity of
reviews to suffer? I'm really curious, and I'd be happy to pass your
feedback along to my colleagues.

Having done a lot of review here on the mailing list, as well as in
GitHub PRs, I vastly prefer the mailing list.

Here's a random list of things that I think I would miss:

 - I really like the flow of having the commit message and diff dumped
   in my editor. I'm very efficient at slicing and dicing text, omitting
   uninteresting quoted bits, etc.

   Web text boxes feel like a straitjacket. I do have a browser plugin
   that opens them in vim. That helps, but it breaks the flow (I make a
   comment, save the file, click "comment", then read to the next place,
   click "+", then start a new vim instance for that comment).  Besides
   the tedium of clicking around, it loses the "unit" size of a single
   email, where I may make many comments, go back and revise earlier
   comments after reading more of the patch, etc.

 - I really like the flow of having conversations next to patches. I can
   look at the index of the mailing list folder and see what people are
   talking about, how big the threads are, etc, at a glance. Moving
   between messages and threads involve single keystrokes.

   Similarly, having local storage is _fast_. I think GitHub is fine for
   a web app. But when I'm reading a high-volume mailing list, I really
   want to flip around quickly. If there's even 500ms to get to the next
   message or thread, it feels clunky and slow to me. Obviously I spend
   more than 500ms _reading_ most messages (though for some I see the
   first paragraph and immediately jump away). It's just the latency
   when I've decided I'm done with one thing and want to move to the
   next.

 - For that matter, GitHub doesn't really have a good tool for random
   conversations. There are issues, which you can vaguely use like a
   thread, but it doesn't quite feel the same.

   I think part of it is that I can view the mailing list both as a
   series of threads _and_ as a stream of messages. So sometimes I mark
   a thread as "read", and then see the next day that there are a ton of
   new messages on it. Maybe those are uninteresting (and it's a single
   keystroke to mark the thread again), but maybe that's a hint that
   there's interesting discussion going on.

   The threading in GitHub comments and pull requests is also not great.
   Each PR or issue is its own thread, but it's totally flat inside.
   There are no sub-threads to organize discussion, and it's sometimes
   hard to see what people are replying to.

 - When I move between a discussion and patches on the list and my local
   git checkout, it's important to do so with minimal fuss. Which means
   I want to use _context_ in my workflow. If I'm reading a thread, I
   want there to be a keystroke for "jump to this thread in my
   checkout". That's (relatively) easy for me to script via mutt (grab
   these patches, apply them). It's a bit harder in the browser (the
   best I've got is to copy-paste the URL to a script that pulls out the
   PR number, then fetches and checks it out).

 - A sort-of feature: the mailing list is actually fairly decentralized,
   because of the "reply-to-all" convention. I don't know if anybody
   else noticed, but vger seemed to be down Friday evening and Saturday
   morning (at least my messages to the list got 400 SMTP codes, and no
   new messages were delivered to me). But I still had some
   conversations going with people, because our messages were mailed
   directly (and the list eventually caught up).

   Now that probably doesn't matter for GitHub, which seems to have
   fairly reasonable uptime. It would matter if we picked a centralized
   tool that didn't.

There are probably more, but I've run out of ranting steam for now. :)

Here are some factors that I think will *improve* reviews:

I was going to respond point-by-point to a few of these, but I think I
covered most of it above. In short, I agree with many of the benefits
you list. In most cases, I've already reaped those benefits for my own
workflow (e.g., my "git am" workflow is pretty efficient now). But not
everybody has done so, and it's a lot to ask of casual contributors.

Given that I work for GitHub, I'm uncomfortable doing any more advocacy
here. If people have concrete questions, I'd be happy to answer them, on
the list or in private.

Hopefully I provided some counterpoint. ;)

-Peff

On Tue, Aug 9, 2016 at 1:20 AM, Michael Haggerty [off-list ref] wrote:

Could you elaborate why you would expect quality and/or quantity of
reviews to suffer? I'm really curious, and I'd be happy to pass your
feedback along to my colleagues.

Since I have been using github at work for a couple months, I do have
a few complaints if it will ever become the way of reviewing things in
git. Some of these may be covered by other people already (I haven't
read all new mails in this thread yet)

 - Github PRs seem to encourage seeing changes as a whole, not a
separate commits. Or at least it's not so convenient to view separate
commits (e.g. not easy to go to the next commit)

 - The ability to show all outdated comments, in case I want to search
through them.

 - I have a feeling that commits in PRs are sorted by authordate, not
in topological order. The order of commits being committed is
important sometimes.

 - Not showing leading spaces mixing with TABs, or trailing spaces

 - I would love to have all patches numbered so can refer to them as
1/7, 2/5... instead of just short sha1 (and I think you have the
ability to refer to "1/7 of iteration 2", see next bullet point)

 -I guess you can manage multiple iterations of a topic with one
iteration per PR, then linking them together. It would be nicer to
somehow bake the iteration concept directly to a PR so we can switch
between them, or do interdiff. I know, this is more of a improvement
request than complaint because ML is not really better.

 - Offline support would be very nice. I'm only most of the time, but
sometimes I do work on git stuff offline.

 - We lose the integration with ML, I think. Sometimes the user
reports a bug here, then we reply back with a patch. With github, I
guess we reply back with a PR number, then further discussion may go
there, some discussion may still be on ML.

* It is easy to summon somebody else into the review conversation by
@-mentioning them. That person immediately can see the whole history of
the PR. (This is an improvement on the status quo, where a new entrant
to a conversation might have to dig through the email trash or an email
archive to see emails that they overlooked before they were added to the
CC list.)

On the other hand, we can't just CC anybody anymore because we don't
know if they have a github account (or the account name for that
matter). Or does github allow @-ing email addresses too? We record
people's email address, not github account names.

* It is possible to search old issues and PRs for additional context.
(Granted, the history of the project from its ML era would have to be
searched separately.)

To me searching in email is still better. Maybe I haven't fully
explored github search capabilities
-- 
Duy

On Tue, Aug 9, 2016 at 8:36 PM, Duy Nguyen [off-list ref] wrote:

On Tue, Aug 9, 2016 at 1:20 AM, Michael Haggerty [off-list ref] wrote:

quoted

Could you elaborate why you would expect quality and/or quantity of
reviews to suffer? I'm really curious, and I'd be happy to pass your
feedback along to my colleagues.

Since I have been using github at work for a couple months, I do have
a few complaints if it will ever become the way of reviewing things in
git. Some of these may be covered by other people already (I haven't
read all new mails in this thread yet)

Another super nit thing: use monospace font for commit messages, or at
least have an option for that.
-- 
Duy

On Tue, Aug 9, 2016 at 1:37 PM, Jeff King [off-list ref] wrote:

   That's (relatively) easy for me to script via mutt (grab
   these patches, apply them).

Could you share your mutt set up pleaaase? I've been wanting this for
a long time, but never used mutt long enough to bother with a proper
setup like this (I blame gmail).
-- 
Duy

On Tue, Aug 9, 2016 at 11:43 AM, Duy Nguyen [off-list ref] wrote:

On Tue, Aug 9, 2016 at 1:37 PM, Jeff King [off-list ref] wrote:

quoted

   That's (relatively) easy for me to script via mutt (grab
   these patches, apply them).

Could you share your mutt set up pleaaase? I've been wanting this for
a long time, but never used mutt long enough to bother with a proper
setup like this (I blame gmail).

That is my complaint^H^H^H^H position, too.
I always wanted to switch to a more powerful
setup than git-send-email for sending /gmail for reading,
but I could not convince myself the steep learning/setup curve
is worth it eventually as it is "not broken enough" to do the change
right now.

My experiments with mutts, have left these lines in my
~/.muttrc

# use shift + A to apply a patch in the working dir
# macro index A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"
# macro pager A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"

macro index A ":set folder='.'\n:copy-message\n"

(IIRC they were broken for many patches, but I got applying
one patch to work. Which sucks for long email series.)

On Tue, Aug 09, 2016 at 08:43:59PM +0200, Duy Nguyen wrote:

On Tue, Aug 9, 2016 at 1:37 PM, Jeff King [off-list ref] wrote:

quoted

   That's (relatively) easy for me to script via mutt (grab
   these patches, apply them).

Could you share your mutt set up pleaaase? I've been wanting this for
a long time, but never used mutt long enough to bother with a proper
setup like this (I blame gmail).

It's actually pretty simple. The relevant config from my .muttrc is:

   macro pager,index D '<shell-escape>rm -f $HOME/patch<enter>'
   macro pager,index A '<copy-message>~/patch<enter><enter>'

I use "~/patch" as a rendezvous point, and then "git am ~/patch" from my
other terminal. That avoids mutt having to know which repo to apply to,
and keeps the "am" process in its own terminal (which is handy if it
runs into conflicts, for example).

So generally I would "D" to clear out the contents of ~/patch, and then
"A" whichever patches I want to apply. I often use mutt's aggregate
selection for that. My bindings are:

  bind index \; tag-pattern
  bind index a tag-prefix

which I think come from pine (which I used for many years before
switching to mutt probably 15 years ago). I don't recall the default
keybindings.

Anyway, you can either tag using a pattern (with ";"), or tag mails
individually (using "t", the default), and then "a-A" to apply the "A"
to all of them (if you are in the habit of tagging all of them and then
doing "A" in one swoop, you could also get rid of the separate "D"
command and just make "A" imply it).

-Peff

On Tue, Aug 09, 2016 at 11:50:51AM -0700, Stefan Beller wrote:

quoted

Could you share your mutt set up pleaaase? I've been wanting this for
a long time, but never used mutt long enough to bother with a proper
setup like this (I blame gmail).

That is my complaint^H^H^H^H position, too.
I always wanted to switch to a more powerful
setup than git-send-email for sending /gmail for reading,
but I could not convince myself the steep learning/setup curve
is worth it eventually as it is "not broken enough" to do the change
right now.

I think I may have shared it before, but here is the script I use to
send emails. It dumps you in mutt, and then I have:

  macro index,pager b ":set edit_headers=yes<enter><resend-message>:set edit_headers=no<enter>"

to send the message ("b" is for "bounce", which I think may be another
Pine-ism).

-- >8 --
#!/bin/sh

upstream_branch() {
  current=`git symbolic-ref HEAD`
  upstream=`git for-each-ref --format='%(upstream)' "$current"`
  if test -n "$upstream"; then
    echo $upstream
  else
    echo origin
  fi
}

get_reply_headers() {
  perl -ne '
    if (defined $opt) {
      if (/^\s+(.*)/) {
        $val .= " $1";
        next;
      }
      print "--$opt=", quotemeta($val), " ";
      $opt = $val = undef;
    }
    if (/^(cc|to):\s*(.*)/i) {
      $opt = lc($1);
      $val = $2;
    }
    elsif (/^message-id:\s*(.*)/i) {
      $opt = "in-reply-to";
      $val = $1;
    }
    elsif (/^subject:\s*\[PATCH v(\d+)/i) {
      print "-v$1 ";
    }
    elsif (/^$/) {
      last;
    }
  '
}

has_nonoption=
for i in "$@"; do
  case "$i" in
    -[0-9]) has_nonoption=yes ;;
    -*) ;;
     *) has_nonoption=yes
  esac
done

git rev-parse || exit 1

: ${REPLY:=$HOME/patch}
test -e "$REPLY" && eval "set -- `get_reply_headers <\"$REPLY\"` \"\$@\""
test "$has_nonoption" = "yes" || set -- "$@" `upstream_branch`

git format-patch -s --stdout --from "$@" >.mbox
if test -t 1; then
  mutt -e 'set sort=mailbox-order' -f .mbox
else
  perl -lne '
    if (/^Subject: (.*)/) {
      $subject = $1;
    }
    elsif ($subject && /^\s+(.*)/) {
      $subject .= " $1";
    }
    elsif ($subject) {
      print $subject;
      $subject = undef;
    }
  ' .mbox |
  sed -e 's/\[PATCH /[/' \
      -e 's/]/]:/' \
      -e 's/^/  /'
fi
rm -f .mbox

On Tue, Aug 09, 2016 at 06:57:05AM -0400, Jeff King wrote:

On Tue, Aug 09, 2016 at 10:11:30AM +0200, Michael J Gruber wrote:

quoted

Maybe two more points of input for the discussion:

off-line capabilities
=====================

The current workflow here seems to work best when you are subscribed to
the git-ml and have your own (local, maybe selective) copy of git-ml in
your (text-based) MUA (mutt, (al)pine, emacs, ...) that can jump right
into git-am and such directly. I'm not sure how important the "off-line"
aspect of that is for some of us, and how that could be replicated on
GitHub - while PRs and such may be Git based behind the scenes there
seems to be no way to clone that info and work from a local clone.
(Don't know if GitLab is more open.)

You can pull it all out via GitHub's HTTP API, but the question is what
format you would use to store it locally (and which tools you would then
use to play with it).

I haven't really tried this lately, though, so I don't know if there is
information that the API would be missing.

I do have a dream of writing a tool that sucks in GitHub PRs to a fake
email thread, lets me make my responses inline in an editor, and then
pushes it back up as PR comments (finding the right positions based on
the quoted context).

You might try https://github.com/joeyh/github-backup