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

Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK

From
Stefan Beller <sbeller@google.com>
Date
Jun 27, 2016, 16:49 UTC
Message-ID
<CAGZ79kZ94PaOfq3GimWiHULbTE7ihMzL9S=Y+npQ4F5gGwFrsA@mail.gmail.com>
In-Reply-To
<20160627143648.GA2618@sigill.intra.peff.net>
On Mon, Jun 27, 2016 at 7:36 AM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> On Mon, Jun 27, 2016 at 06:02:12AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > I also wondered how we managed to miss such an obvious point in review
>> > of the original patch. Sadly, we _did_ notice it[1] but it looks like we
>> > never fixed the problem. That is even more disturbing.
>>
>> Yes indeed.
>>
>> I try to pay attention to "this is broken because..."  comments in
>> discussions to make a note in my copy of "What's cooking" report for
>> a problematic topic, as that is where I work from when merging
>> topics down, but apparently that procedure failed work in this case.
>> There needs a stronger mechanism to stop a known-buggy patch from
>> going thru, but I am not sure offhand what that should be.
>
> I was the one who saw the bug. I could have followed the series more
> closely to make sure my concern was addressed. Or possibly pointed out
> the bug more prominently than an in a "PS" as part of the discussion.

I thought I would have fixed that bug, but apparently I did not. (I agreed on the bug being there at the time of discussion [1], so I guess I can be blamed most for this failure)

[1] http://thread.gmane.org/gmane.comp.version-control.git/282514/focus=282694
>
> I think part of the problem was that this particular series was
> large-ish and involved a lot of re-rolls, and I got sick of looking at
> it. I dunno.

I haven't send patches to git for quite a while now, but writing smaller patches is the way to go for me. (I am currently looking at the repo tool, that has no test suite, so there too I try to make very small patches.)

Show 5 quoted lines
>
> It's also true that our error rate will never be 0%. So some bugs will
> always slip through, some review comments will be forgotten, etc. Eric
> did find and fix the bug just now, so the "many eyes" theory did work
> here eventually.
Eric, thanks for catching and fixing the bug!

Quite a while ago, when I started doing code reviews professionally, I wondered if the code review procedure can be semi-automated, as automation helps keeping the error rate low. By that I mean having a check list which I can check off each point for each patch. That seems to be very good in theory, but when trying it I was finding myself doing a lot of unneeded work as some points of such a check list just do not apply for a specific patch. So I did not follow through with that.

Thanks, Stefan

Previous: Jeff KingNext: Jeff King
Message 6 of 17 in “wrapper: xread/xwrite fixes for non-blocking FDs”
  1. 0/2 wrapper: xread/xwrite fixes for non-blocking FDsEric Wong, Jun 26, 2016
  2. 1/2 xread: retry after poll on EAGAIN/EWOULDBLOCKEric Wong, Jun 26, 2016
  3. Jeff KingJun 26, 2016
  4. Junio C HamanoJun 27, 2016
  5. Jeff KingJun 27, 2016
  6. Stefan BellerJun 27, 2016
  7. Jeff KingJun 27, 2016
  8. Stefan BellerJun 27, 2016
  9. Jeff KingJun 27, 2016
  10. Eric WongJun 27, 2016
  11. Jeff KingJun 27, 2016
  12. Jeff KingJun 27, 2016
  13. Eric WongJun 27, 2016
  14. Jeff KingJun 26, 2016
  15. 1/2 xread: retry after poll on EAGAIN/EWOULDBLOCKEric Wong, Jun 27, 2016
  16. 2/2 xwrite: poll on non-blocking FDsEric Wong, Jun 26, 2016
  17. Jeff KingJun 26, 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.