git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Aug 9, 2016, 11:37 UTC
Message-ID
<20160809113703.57irthzzpg6j3dmv@sigill.intra.peff.net>
In-Reply-To
<3055f063-c9c1-0bf5-99bd-08256c253d33@alum.mit.edu>
On Tue, Aug 09, 2016 at 01:20:05AM +0200, Michael Haggerty wrote:
Show 7 quoted lines
> > 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
Previous: Josh TriplettNext: Junio C Hamano
Message 231 of 262 in “Use merge_recursive() directly in the builtin am”
  1. 0/9 Use merge_recursive() directly in the builtin amJohannes Schindelin, Jun 29, 2016
  2. 1/9 Report bugs consistentlyJohannes Schindelin, Jun 29, 2016
  3. Johannes SchindelinJun 29, 2016
  4. Eric SunshineJun 29, 2016
  5. Johannes SchindelinJun 30, 2016
  6. Junio C HamanoJun 29, 2016
  7. Johannes SchindelinJun 30, 2016
  8. Jeff KingJun 30, 2016
  9. Johannes SchindelinJul 1, 2016
  10. Jeff KingJul 1, 2016
  11. Johannes SixtJun 30, 2016
  12. Johannes SchindelinJun 30, 2016
  13. Duy NguyenJul 2, 2016
  14. Johannes SchindelinJul 2, 2016
  15. Duy NguyenJul 2, 2016
  16. Johannes SchindelinJul 5, 2016
  17. 2/9 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Jun 29, 2016
  18. Junio C HamanoJun 29, 2016
  19. Johannes SchindelinJul 1, 2016
  20. Junio C HamanoJul 1, 2016
  21. Johannes SchindelinJul 2, 2016
  22. Junio C HamanoJul 6, 2016
  23. Johannes SchindelinJul 7, 2016
  24. 4/9 merge_recursive: abort properly upon errorsJohannes Schindelin, Jun 29, 2016
  25. Junio C HamanoJun 29, 2016
  26. Johannes SchindelinJul 1, 2016
  27. Junio C HamanoJul 1, 2016
  28. Johannes SchindelinJul 2, 2016
  29. 3/9 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Jun 29, 2016
  30. Junio C HamanoJun 29, 2016
  31. Johannes SchindelinJul 1, 2016
  32. Junio C HamanoJul 1, 2016
  33. Johannes SchindelinJul 2, 2016
  34. 6/9 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Jun 29, 2016
  35. 7/9 merge-recursive: handle return values indicating errorsJohannes Schindelin, Jun 29, 2016
  36. Junio C HamanoJun 29, 2016
  37. Johannes SchindelinJul 1, 2016
  38. 5/9 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Jun 29, 2016
  39. Junio C HamanoJun 29, 2016
  40. Johannes SchindelinJul 1, 2016
  41. Eric WongJul 1, 2016
  42. 8/9 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Jun 29, 2016
  43. Junio C HamanoJun 29, 2016
  44. Johannes SchindelinJul 1, 2016
  45. 9/9 am: make a direct call to merge_recursiveJohannes Schindelin, Jun 29, 2016
  46. Junio C HamanoJun 29, 2016
  47. Johannes SchindelinJun 30, 2016
  48. Junio C HamanoJul 1, 2016
  49. Junio C HamanoJun 29, 2016
  50. Johannes SchindelinJul 1, 2016
  51. 00/17 Use merge_recursive() directly in the builtin amJohannes Schindelin, Jul 5, 2016
  52. 01/17 Verify that `git pull --rebase` shows the helpful advice when failingJohannes Schindelin, Jul 5, 2016
  53. 02/17 Report bugs consistentlyJohannes Schindelin, Jul 5, 2016
  54. Jakub NarębskiJul 5, 2016
  55. Johannes SchindelinJul 5, 2016
  56. Duy NguyenJul 6, 2016
  57. Johannes SchindelinJul 7, 2016
  58. 04/17 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Jul 5, 2016
  59. 03/17 Avoid translating bug messagesJohannes Schindelin, Jul 5, 2016
  60. 06/17 merge_recursive: abort properly upon errorsJohannes Schindelin, Jul 5, 2016
  61. 07/17 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Jul 5, 2016
  62. 05/17 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Jul 5, 2016
  63. 09/17 merge-recursive: handle return values indicating errorsJohannes Schindelin, Jul 5, 2016
  64. 08/17 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Jul 5, 2016
  65. 10/17 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Jul 5, 2016
  66. 12/17 am -3: use merge_recursive() directly againJohannes Schindelin, Jul 5, 2016
  67. 11/17 am: counteract gender biasJohannes Schindelin, Jul 5, 2016
  68. Junio C HamanoJul 6, 2016
  69. Johannes SchindelinJul 7, 2016
  70. Junio C HamanoJul 7, 2016
  71. Johannes SchindelinJul 7, 2016
  72. Junio C HamanoJul 7, 2016
  73. 13/17 merge-recursive: flush output buffer before printing error messagesJohannes Schindelin, Jul 5, 2016
  74. 14/17 merge-recursive: write the commit title in one goJohannes Schindelin, Jul 5, 2016
  75. 16/17 Ensure that the output buffer is released after calling merge_trees()Johannes Schindelin, Jul 5, 2016
  76. 15/17 merge-recursive: offer an option to retain the output in 'obuf'Johannes Schindelin, Jul 5, 2016
  77. 17/17 merge-recursive: flush output buffer even when erroring outJohannes Schindelin, Jul 5, 2016
  78. Junio C HamanoJul 6, 2016
  79. Johannes SchindelinJul 7, 2016
  80. 00/16 Use merge_recursive() directly in the builtin amJohannes Schindelin, Jul 7, 2016
  81. 02/16 Report bugs consistentlyJohannes Schindelin, Jul 7, 2016
  82. 03/16 Avoid translating bug messagesJohannes Schindelin, Jul 7, 2016
  83. 05/16 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Jul 7, 2016
  84. 06/16 merge_recursive: abort properly upon errorsJohannes Schindelin, Jul 7, 2016
  85. 04/16 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Jul 7, 2016
  86. 01/16 Verify that `git pull --rebase` shows the helpful advice when failingJohannes Schindelin, Jul 7, 2016
  87. 07/16 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Jul 7, 2016
  88. 08/16 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Jul 7, 2016
  89. 09/16 merge-recursive: handle return values indicating errorsJohannes Schindelin, Jul 7, 2016
  90. 11/16 am -3: use merge_recursive() directly againJohannes Schindelin, Jul 7, 2016
  91. 10/16 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Jul 7, 2016
  92. 12/16 merge-recursive: flush output buffer before printing error messagesJohannes Schindelin, Jul 7, 2016
  93. 14/16 merge-recursive: offer an option to retain the output in 'obuf'Johannes Schindelin, Jul 7, 2016
  94. 13/16 merge-recursive: write the commit title in one goJohannes Schindelin, Jul 7, 2016
  95. 15/16 Ensure that the output buffer is released after calling merge_trees()Johannes Schindelin, Jul 7, 2016
  96. 16/16 merge-recursive: flush output buffer even when erroring outJohannes Schindelin, Jul 7, 2016
  97. Junio C HamanoJul 12, 2016
  98. Johannes SchindelinJul 14, 2016
  99. Junio C HamanoJul 14, 2016
  100. Junio C HamanoJul 19, 2016
  101. Johannes SchindelinJul 19, 2016
  102. Johannes SchindelinJul 19, 2016
  103. Junio C HamanoJul 19, 2016
  104. 00/16 Use merge_recursive() directly in the builtin amJohannes Schindelin, Jul 22, 2016
  105. 01/16 Verify that `git pull --rebase` shows the helpful advice when failingJohannes Schindelin, Jul 22, 2016
  106. Junio C HamanoJul 25, 2016
  107. Johannes SchindelinJul 26, 2016
  108. 02/16 Report bugs consistentlyJohannes Schindelin, Jul 22, 2016
  109. Junio C HamanoJul 25, 2016
  110. Jeff KingJul 25, 2016
  111. Junio C HamanoJul 25, 2016
  112. Johannes SchindelinJul 26, 2016
  113. 03/16 Avoid translating bug messagesJohannes Schindelin, Jul 22, 2016
  114. 04/16 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Jul 22, 2016
  115. 05/16 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Jul 22, 2016
  116. 06/16 merge_recursive: abort properly upon errorsJohannes Schindelin, Jul 22, 2016
  117. Junio C HamanoJul 25, 2016
  118. Johannes SchindelinJul 26, 2016
  119. 07/16 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Jul 22, 2016
  120. 08/16 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Jul 22, 2016
  121. 09/16 merge-recursive: handle return values indicating errorsJohannes Schindelin, Jul 22, 2016
  122. 10/16 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Jul 22, 2016
  123. 11/16 am -3: use merge_recursive() directly againJohannes Schindelin, Jul 22, 2016
  124. Junio C HamanoJul 25, 2016
  125. Johannes SchindelinJul 26, 2016
  126. Junio C HamanoJul 26, 2016
  127. 12/16 merge-recursive: flush output buffer before printing error messagesJohannes Schindelin, Jul 22, 2016
  128. 14/16 merge-recursive: offer an option to retain the output in 'obuf'Johannes Schindelin, Jul 22, 2016
  129. 15/16 Ensure that the output buffer is released after calling merge_trees()Johannes Schindelin, Jul 22, 2016
  130. 13/16 merge-recursive: write the commit title in one goJohannes Schindelin, Jul 22, 2016
  131. 16/16 merge-recursive: flush output buffer even when erroring outJohannes Schindelin, Jul 22, 2016
  132. 00/16 Use merge_recursive() directly in the builtin amJohannes Schindelin, Jul 26, 2016
  133. 01/16 t5520: verify that `pull --rebase` shows the helpful advice when failingJohannes Schindelin, Jul 26, 2016
  134. 02/16 Report bugs consistentlyJohannes Schindelin, Jul 26, 2016
  135. 03/16 Avoid translating bug messagesJohannes Schindelin, Jul 26, 2016
  136. 04/16 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Jul 26, 2016
  137. 05/16 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Jul 26, 2016
  138. 06/16 merge_recursive: abort properly upon errorsJohannes Schindelin, Jul 26, 2016
  139. 07/16 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Jul 26, 2016
  140. 08/16 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Jul 26, 2016
  141. 09/16 merge-recursive: handle return values indicating errorsJohannes Schindelin, Jul 26, 2016
  142. 10/16 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Jul 26, 2016
  143. 13/16 merge-recursive: write the commit title in one goJohannes Schindelin, Jul 26, 2016
  144. Junio C HamanoJul 27, 2016
  145. Johannes SchindelinAug 1, 2016
  146. 11/16 am -3: use merge_recursive() directly againJohannes Schindelin, Jul 26, 2016
  147. 12/16 merge-recursive: flush output buffer before printing error messagesJohannes Schindelin, Jul 26, 2016
  148. Junio C HamanoJul 27, 2016
  149. Junio C HamanoJul 27, 2016
  150. Johannes SchindelinAug 1, 2016
  151. 14/16 merge-recursive: offer an option to retain the output in 'obuf'Johannes Schindelin, Jul 26, 2016
  152. Junio C HamanoJul 27, 2016
  153. Junio C HamanoJul 28, 2016
  154. Johannes SchindelinAug 1, 2016
  155. Junio C HamanoAug 1, 2016
  156. Johannes SchindelinAug 2, 2016
  157. Junio C HamanoAug 2, 2016
  158. Johannes SchindelinAug 1, 2016
  159. 15/16 Ensure that the output buffer is released after calling merge_trees()Johannes Schindelin, Jul 26, 2016
  160. Junio C HamanoJul 27, 2016
  161. Johannes SchindelinAug 1, 2016
  162. 16/16 merge-recursive: flush output buffer even when erroring outJohannes Schindelin, Jul 26, 2016
  163. Junio C HamanoJul 27, 2016
  164. Johannes SchindelinAug 1, 2016
  165. Junio C HamanoAug 1, 2016
  166. 00/16 Use merge_recursive() directly in the builtin amJohannes Schindelin, Aug 1, 2016
  167. 01/16 t5520: verify that `pull --rebase` shows the helpful advice when failingJohannes Schindelin, Aug 1, 2016
  168. 02/16 Report bugs consistentlyJohannes Schindelin, Aug 1, 2016
  169. 05/16 Prepare the builtins for a libified merge_recursive()Johannes Schindelin, Aug 1, 2016
  170. Junio C HamanoAug 1, 2016
  171. Johannes SchindelinAug 2, 2016
  172. 04/16 merge-recursive: clarify code in was_tracked()Johannes Schindelin, Aug 1, 2016
  173. 03/16 Avoid translating bug messagesJohannes Schindelin, Aug 1, 2016
  174. 11/16 am -3: use merge_recursive() directly againJohannes Schindelin, Aug 1, 2016
  175. 09/16 merge-recursive: handle return values indicating errorsJohannes Schindelin, Aug 1, 2016
  176. 14/16 merge-recursive: offer an option to retain the output in 'obuf'Johannes Schindelin, Aug 1, 2016
  177. 10/16 merge-recursive: switch to returning errors instead of dyingJohannes Schindelin, Aug 1, 2016
  178. 12/16 merge-recursive: flush output buffer before printing error messagesJohannes Schindelin, Aug 1, 2016
  179. 07/16 merge-recursive: avoid returning a wholesale structJohannes Schindelin, Aug 1, 2016
  180. Junio C HamanoAug 4, 2016
  181. 15/16 Ensure that the output buffer is released after calling merge_trees()Johannes Schindelin, Aug 1, 2016
  182. Junio C HamanoAug 4, 2016
  183. 08/16 merge-recursive: allow write_tree_from_memory() to error outJohannes Schindelin, Aug 1, 2016
  184. Junio C HamanoAug 4, 2016
  185. 06/16 merge_recursive: abort properly upon errorsJohannes Schindelin, Aug 1, 2016
  186. Junio C HamanoAug 1, 2016
  187. Johannes SchindelinAug 2, 2016
  188. Junio C HamanoAug 2, 2016
  189. patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errorsJohannes Schindelin, Aug 3, 2016
  190. Junio C HamanoAug 3, 2016
  191. Jeff KingAug 3, 2016
  192. Junio C HamanoAug 3, 2016
  193. Jeff KingAug 3, 2016
  194. Johannes SchindelinAug 4, 2016
  195. Jeff KingAug 4, 2016
  196. Junio C HamanoAug 4, 2016
  197. Jeff KingAug 5, 2016
  198. Johannes SchindelinAug 5, 2016
  199. Johannes SchindelinAug 3, 2016
  200. Stefan BellerAug 3, 2016
  201. Johannes SchindelinAug 4, 2016
  202. Stefan BellerAug 4, 2016
  203. Eric WongAug 4, 2016
  204. Johannes SchindelinAug 5, 2016
  205. Eric WongAug 5, 2016
  206. Johannes SchindelinAug 5, 2016
  207. Stefan BellerAug 5, 2016
  208. Josh TriplettAug 5, 2016
  209. Eric WongAug 5, 2016
  210. Johannes SchindelinAug 6, 2016
  211. Junio C HamanoAug 6, 2016
  212. Eric WongAug 6, 2016
  213. Johannes SchindelinAug 7, 2016
  214. Junio C HamanoAug 8, 2016
  215. Johannes SchindelinAug 9, 2016
  216. Lars SchneiderAug 7, 2016
  217. Junio C HamanoAug 8, 2016
  218. Johannes SchindelinAug 9, 2016
  219. Junio C HamanoAug 9, 2016
  220. Eric WongAug 5, 2016
  221. Johannes SchindelinAug 6, 2016
  222. Richard IpsumAug 5, 2016
  223. Johannes SchindelinAug 5, 2016
  224. Richard IpsumAug 6, 2016
  225. Michael HaggertyAug 8, 2016
  226. Junio C HamanoAug 8, 2016
  227. Michael HaggertyAug 8, 2016
  228. Michael J GruberAug 9, 2016
  229. Jeff KingAug 9, 2016
  230. Josh TriplettAug 10, 2016
  231. Jeff KingAug 9, 2016
  232. Junio C HamanoAug 9, 2016
  233. Jeff KingAug 9, 2016
  234. Junio C HamanoAug 9, 2016
  235. Duy NguyenAug 9, 2016
  236. Stefan BellerAug 9, 2016
  237. Jeff KingAug 9, 2016
  238. Jeff KingAug 9, 2016
  239. Duy NguyenAug 9, 2016
  240. Duy NguyenAug 9, 2016
  241. Duy NguyenAug 9, 2016
  242. Richard IpsumAug 9, 2016
  243. Jeff KingAug 9, 2016
  244. Michael HaggertyAug 9, 2016
  245. Johannes SchindelinAug 9, 2016
  246. Eric WongAug 9, 2016
  247. Josh TriplettAug 10, 2016
  248. Eric WongAug 10, 2016
  249. Jakub NarębskiAug 10, 2016
  250. Josh TriplettAug 10, 2016
  251. Junio C HamanoAug 10, 2016
  252. Duy NguyenAug 5, 2016
  253. Johannes SchindelinAug 5, 2016
  254. Philip OakleyAug 5, 2016
  255. Johannes SchindelinAug 6, 2016
  256. Philip OakleyAug 6, 2016
  257. Junio C HamanoAug 2, 2016
  258. 13/16 merge-recursive: write the commit title in one goJohannes Schindelin, Aug 1, 2016
  259. 16/16 merge-recursive: flush output buffer even when erroring outJohannes Schindelin, Aug 1, 2016
  260. Junio C HamanoAug 4, 2016
  261. Johannes SchindelinAug 5, 2016
  262. Junio C HamanoAug 6, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.