git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 12/13] walker.c: use ref transaction for ref updates

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Apr 21, 2014, 22:53 UTC
Message-ID
<1398120811-20284-13-git-send-email-sahlberg@google.com>
In-Reply-To
<1398120811-20284-1-git-send-email-sahlberg@google.com>

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@google.com>
---
 walker.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..fa02d09 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,16 @@ 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 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 +268,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 +286,22 @@ 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;
+		if (ref_transaction_update(transaction, write_ref[i],
+					   &sha1[20 * i], NULL,
+					   0, 0))
+			goto rollback_and_fail;
 	}
-	free(msg);
 
+	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+				   UPDATE_REFS_QUIET_ON_ERR))
+		goto rollback_and_fail;
+
+	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);
+	ref_transaction_rollback(transaction);
 
 	return -1;
 }

-- 
1.9.1.515.g3b87021
Previous: Ronnie SahlbergNext: Ronnie Sahlberg
Message 16 of 19 in “Use ref transactions from most callers”
  1. 00/13 Use ref transactions from most callersRonnie Sahlberg, Apr 21, 2014
  2. 01/13 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Apr 21, 2014
  3. 02/13 refs.c: use a single exit path from transaction commit and handle onerrRonnie Sahlberg, Apr 21, 2014
  4. Junio C HamanoApr 22, 2014
  5. Ronnie SahlbergApr 22, 2014
  6. Junio C HamanoApr 22, 2014
  7. 03/13 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Apr 21, 2014
  8. 04/13 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Apr 21, 2014
  9. 05/13 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, Apr 21, 2014
  10. 06/13 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Apr 21, 2014
  11. 07/13 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Apr 21, 2014
  12. 08/13 commit.c: use ref transactions for updatesRonnie Sahlberg, Apr 21, 2014
  13. 09/13 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Apr 21, 2014
  14. 10/13 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Apr 21, 2014
  15. 11/13 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Apr 21, 2014
  16. 12/13 walker.c: use ref transaction for ref updatesRonnie Sahlberg, Apr 21, 2014
  17. 13/13 refs.c: change update_ref to use a transactionRonnie Sahlberg, Apr 21, 2014
  18. Junio C HamanoApr 22, 2014
  19. Ronnie SahlbergApr 22, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.