From: Ronnie Sahlberg Date: Wed, 28 May 2014 20:56:33 GMT Subject: Re: [PATCH v11 26/41] walker.c: use ref transaction for ref updates Message-ID: In-Reply-To: <20140528195645.GY12314@google.com> On Wed, May 28, 2014 at 12:56 PM, Jonathan Nieder wrote: > 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. > > Sounds good. > >> This changes the locking slightly for walker_fetch. Previously the code would >> lock all refs before writing them but now we do not lock the refs until the >> commit stage. > > I don't think this actually changes locking in any significant way. > (Timing changes, but that's to be expected whenever code changes.) > > The old contract was: > > You run git http-fetch with refnames passed with "-w". http-fetch > will overwrite the refs with their new values. > > The new contract is: > > You run git http-fetch with refnames passed with "-w". http-fetch > will overwrite the refs with their new values. > > What changed? Timing for the locks basically. Which means the outcome for certain races could change slightly. But not important. > > [...] >> Note that this function is only called when fetching from a remote HTTP >> repository onto the local (most of the time single-user) repository which >> likely means that the type of collissions that the previous locking would >> protect against and cause the fetch to fail for to be even more rare. > > More to the point, while this function is used by 'git remote-http' (and > hence by "git fetch https://foo/bar/baz"), it is only used to fetch > objects by that code path. The remote helper machinery handles the ref > updates on its own (and passes NULL as the write_ref argument to the > dumb walker). > > [...] >> Signed-off-by: Ronnie Sahlberg >> --- >> walker.c | 56 +++++++++++++++++++++++++++++++++----------------------- >> 1 file changed, 33 insertions(+), 23 deletions(-) > > The code change looks good from a quick glance. > > gcc 4.6.3 doesn't seem to be smart enough to notice that 'transaction' > is always initialized before it is used, so it (optionally) could be > worth initializing 'transition = NULL' to work around that. > > So I think this just needs a simpler commit message and then it would be > good to go. > Done, thanks.