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. 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. There is thus a very short window where changes could be done locally during the fetch which would be overwritten when the fetch completes and commits its transaction. But this window should be reasonably short. Even if this race does trigger, since both the old code and the new code just overwrites the refs to the new values without checking or comparing them with the previous value, this is not too dissimilar to a similar scenario where you first do a ref change locally and then later do a fetch that overwrites the local change. With this in mind I do not see the change in locking semantics to be critical. 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. Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> --- walker.c | 51 ++++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/walker.c b/walker.c index 1dd86b8..6044ccf 100644 --- a/walker.c +++ b/walker.c @@ -251,24 +251,18 @@ 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 *)); + char ref_name[PATH_MAX]; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; 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; - } - } + transaction = ref_transaction_begin(); + if (!transaction) + return -1; if (!walker->get_recover) for_each_ref(mark_complete, NULL); @@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char **target, 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 +288,26 @@ 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; + sprintf(ref_name, "refs/%s", write_ref[i]); + if (ref_transaction_update(transaction, ref_name, + &sha1[20 * i], NULL, + 0, 0)) + goto rollback_and_fail; + } + + if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)", + &err)) { + error("%s", err.buf); + goto rollback_and_fail; } - 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: + free(msg); + strbuf_release(&err); + ref_transaction_free(transaction); return -1; } -- 1.9.1.532.gf8485a6 -- 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