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]

 



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@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.

Jonathan
--
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]