On Mon, Jun 27, 2016 at 7:36 AM, Jeff King <peff@xxxxxxxx> wrote: > On Mon, Jun 27, 2016 at 06:02:12AM -0700, Junio C Hamano wrote: > >> Jeff King <peff@xxxxxxxx> 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.) > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html