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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]