On Tue, Jul 8, 2014 at 6:33 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 06/20/2014 04:43 PM, 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.
>>
>> 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
>
> s/collissions/collisions/
>
>> protect against and cause the fetch to fail for to be even more rare.
>
> Grammatico: s/to be/are/ ?
Thanks. Fixed.
>
>> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>> ---
>> walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------
>> 1 file changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/walker.c b/walker.c
>> index 1dd86b8..60d9f9e 100644
>> --- a/walker.c
>> +++ b/walker.c
>> @@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
>> int walker_fetch(struct walker *walker, int targets, char **target,
>> const char **write_ref, const char *write_ref_log_details)
>> {
>> - struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
>> + struct strbuf ref_name = STRBUF_INIT;
>> + struct strbuf err = STRBUF_INIT;
>> + struct ref_transaction *transaction = NULL;
>> unsigned char *sha1 = xmalloc(targets * 20);
>> - char *msg;
>> - int ret;
>> + char *msg = NULL;
>> int i;
>>
>> save_commit_buffer = 0;
>>
>> - for (i = 0; i < targets; i++) {
>> - if (!write_ref || !write_ref[i])
>> - continue;
>> -
>> - lock[i] = lock_ref_sha1(write_ref[i], NULL);
>> - if (!lock[i]) {
>> - error("Can't lock ref %s", write_ref[i]);
>> - goto unlock_and_fail;
>> + if (write_ref) {
>> + transaction = ref_transaction_begin(&err);
>> + if (!transaction) {
>> + error("%s", err.buf);
>> + goto rollback_and_fail;
>> }
>> }
>> -
>
> Is there some reason why the transaction cannot be built up during a
> single iteration over targets, thereby also avoiding the need for the
> sha1[20*i] stuff? This seems like exactly the kind of situation where
> transactions should *save* code. But perhaps I've overlooked a
> dependency between the two loops.
I did it this way to keep the changes minimal. But you are right that
with this we can do a larger refactoring and start saving some code.
I can add changes to a later series to do that change but I want to
keep this change as small as possible for now.
regards
ronnie sahlberg
--
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