Re: [PATCH 11/11] walker.c: use ref transaction for ref updates
I have updated the commit message with some text why I do not think this change is critical for this case. I will resend v2 of the patch series in a little while. On Sat, Apr 19, 2014 at 12:48 PM, Michael Haggerty wrote: > On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: >> Switch to using ref transactions in walker_fetch(). As part of the >> refactoring >> to use ref transactions we also fix a potential memory leak where in the >> original code if write_ref_sha1() would fail we would end up returning from >> the function without free()ing the msg string. > > I don't have time to review this last patch this evening, but one thing > struck me. It seems like the old code went to extra effort to lock all > the write_ref references early in the function, whereas your modified > version doesn't lock them until later. Have you verified that you are > not opening a possible race condition? If so, your commit message > should justify that it isn't a problem. In other words, what does the > code do between the old time of locking and the new time of locking and > why doesn't it care whether the references are locked? > > Aside from my other comments, patches 01-10 in the series looked fine. > Thanks! > > Michael > > -- > Michael Haggerty > mhag...@alum.mit.edu > http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/11] walker.c: use ref transaction for ref updates
Michael Haggerty writes: > On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: >> Switch to using ref transactions in walker_fetch(). As part of the >> refactoring >> to use ref transactions we also fix a potential memory leak where in the >> original code if write_ref_sha1() would fail we would end up returning from >> the function without free()ing the msg string. > ... > Aside from my other comments, patches 01-10 in the series looked fine. > Thanks! Thanks, both. When this is queued on 'pu', it appears that t5550 breaks. I had to futz with conflicts with the lockfile topic, so a botched conflict resolution might be a cause for the breakage, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/11] walker.c: use ref transaction for ref updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Switch to using ref transactions in walker_fetch(). As part of the refactoring > to use ref transactions we also fix a potential memory leak where in the > original code if write_ref_sha1() would fail we would end up returning from > the function without free()ing the msg string. I don't have time to review this last patch this evening, but one thing struck me. It seems like the old code went to extra effort to lock all the write_ref references early in the function, whereas your modified version doesn't lock them until later. Have you verified that you are not opening a possible race condition? If so, your commit message should justify that it isn't a problem. In other words, what does the code do between the old time of locking and the new time of locking and why doesn't it care whether the references are locked? Aside from my other comments, patches 01-10 in the series looked fine. Thanks! Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
