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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

Jonathan