Re: [PATCH v11 26/41] walker.c: use ref transaction for ref updates

> 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?

>
> [...]
>> 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@google.com>
>> ---
>>  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.
>