Hi Hannes, On Fri, 10 Jun 2016, Johannes Sixt wrote: > Am 10.06.2016 um 13:11 schrieb Johannes Schindelin: > > On Fri, 10 Jun 2016, Christian Couder wrote: > > > > > I fixed this by moving the "close(fd)" call just after the > > > "apply_patch()" call. > > This bug in v1 was discovered by the test suite and fixed in v2. Maybe it would be a good idea to fix test failures before sending out an 80-strong patch series. Just sayin'. > > and this: > > > > > I will have another look at the 2 other places where there are > > > open()/close() or fopen()/fclose() calls. > > > > but nothing about a careful, systematic investigation of all error code > > paths. As a consequence, I fully expect to encounter test failures as soon > > as I test your patch series again, simply because resources are still in > > use when they should no longer be used. In other words, my expectations > > are now lower than they have been before, my concerns are not at all > > addressed. > > Do you trust the test suite to some degree? Yes. To *some* degree. For comparison, my rebase--helper patches pass the test suite for more than a month now. I still discovered two minor problems and fixed them yesterday. Our test suite (and *any* test suite, really) is in *no way* a substitute for proper review. And the first review needs to be performed by the developer herself. Just compare the two options: to add tests for each and every corner case, especially error code paths, or alternatively just going through the patch manually, inspecting every error code path from the used-resource point of view? I am sure you agree that the latter is a much better use of everybody's time, and also that that review will be most effective when performed by the person most familiar with the patches. > It passes after the above bug was fixed in v2. In addition, haven't > found any problems so far during daily use. I put much more stock into the latter than the former. The former is required, to be sure, but by far not sufficient. TBH I was quite disappointed when I tried to run v1 and found such a simple bug right away, which made me wonder how many more not-so-simple bugs were to be found. All that, while I really wanted this patch series to be good enough to enter core Git's code base. > > > This is the newest iteration: > > > > > > https://github.com/chriscool/git/commits/libify-apply-use-in-am65 > > > > And that cute 65 in the name is the revision. > > Yeah, that number is painful. I would appreciate an unversioned branch > name, too. To be frank, I think that a version number in a branch name is incorrect Git usage. Version numbers are something for tags, not for branches (I would understand partial version numbers in maintenance branches, of course, because then they would not version the *branch* but convey the purpose of the branch, as names should). Ciao, Johannes -- 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