On Wed, May 28, 2014 at 12:56 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 <sahlberg@xxxxxxxxxx> >> --- >> 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. -- 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