Re: [PATCH v20 33/48] walker.c: use ref transaction for ref updates
On Tue, Jul 8, 2014 at 6:33 AM, Michael Haggerty 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
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v20 33/48] walker.c: use ref transaction for ref updates
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/ ?
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> 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.
> if (!walker->get_recover)
> for_each_ref(mark_complete, NULL);
>
> for (i = 0; i < targets; i++) {
> if (interpret_target(walker, target[i], &sha1[20 * i])) {
> error("Could not interpret response from server '%s' as
> something to pull", target[i]);
> - goto unlock_and_fail;
> + goto rollback_and_fail;
> }
> if (process(walker, lookup_unknown_object(&sha1[20 * i])))
> - goto unlock_and_fail;
> + goto rollback_and_fail;
> }
>
> if (loop(walker))
> - goto unlock_and_fail;
> + goto rollback_and_fail;
>
> if (write_ref_log_details) {
> msg = xmalloc(strlen(write_ref_log_details) + 12);
> @@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets,
> char **target,
> for (i = 0; i < targets; i++) {
> if (!write_ref || !write_ref[i])
> continue;
> - ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
> (unknown)");
> - lock[i] = NULL;
> - if (ret)
> - goto unlock_and_fail;
> + strbuf_reset(&ref_name);
> + strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
> + if (ref_transaction_update(transaction, ref_name.buf,
> +&sha1[20 * i], NULL, 0, 0,
> +&err)) {
> + error("%s", err.buf);
> + goto rollback_and_fail;
> + }
> + }
> + if (write_ref) {
> + if (ref_transaction_commit(transaction,
> +msg ? msg : "fetch (unknown)",
> +&err)) {
> + error("%s", err.buf);
> + goto rollback_and_fail;
> + }
> + ref_transaction_free(transaction);
> }
> - free(msg);
>
> + free(msg);
> return 0;
>
> -unlock_and_fail:
> - for (i = 0; i < targets; i++)
> - if (lock[i])
> - unlock_ref(lock[i]);
> +rollback_and_fail:
> + ref_transaction_free(transaction);
> + free(msg);
> + strbuf_release(&err);
> + strbuf_release(&ref_name);
>
> return -1;
> }
>
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
[PATCH v20 33/48] walker.c: use ref transaction for ref updates
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
protect against and cause the fetch to fail for to be even more rare.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
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;
}
}
-
if (!walker->get_recover)
for_each_ref(mark_complete, NULL);
for (i = 0; i < targets; i++) {
if (interpret_target(walker, target[i], &sha1[20 * i])) {
error("Could not interpret response from server '%s' as
something to pull", target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
(unknown)");
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ strbuf_reset(&ref_name);
+ strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name.buf,
+ &sha1[20 * i], NULL, 0, 0,
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
+ }
+ }
+ if (write_ref) {
+ if (ref_transaction_commit(transaction,
+ msg ? msg : "fetch (unknown)",
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
+ }
+ ref_transaction_free(transaction);
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i < targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ ref_transaction_free(transaction);
+ free(msg);
+ strbuf_release(&err);
+ strbuf_release(&ref_name);
return -1;
}
--
2.0.0.420.g181e020.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
