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

[PATCH v8 24/44] fetch.c: use a single ref transaction for all ref updates

From
Ronnie Sahlberg <sahlberg@google.com>
Date
May 15, 2014, 17:29 UTC
Message-ID
<1400174999-26786-25-git-send-email-sahlberg@google.com>
In-Reply-To
<1400174999-26786-1-git-send-email-sahlberg@google.com>

Change store_updated_refs to use a single ref transaction for all refs that are updated during the fetch. This makes the fetch more atomic when update failures occur.

Since ref update failures will now no longer occur in the code path for updating a single ref in s_update_ref, we no longer have as detailed error message logging the exact reference and the ref log action as in the old cod Instead since we fail the entire transaction we log a much more generic message. But since we commit the transaction using MSG_ON_ERR we will log an error containing the ref name if either locking of writing the ref would so the regression in the log message is minor.

This will also change the order in which errors are checked for and logged which may alter which error will be logged if there are multiple errors occuring during a fetch.

For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
        if (current_branch &&
            !strcmp(ref->name, current_branch->name) &&
            !(update_head_ok || is_bare_repository()) &&
            !is_null_sha1(ref->old_sha1)) {
                /*
                 * If this is the head, and it's not okay to update
                 * the head, and the old value of the head isn't empty...
                 */

In the old code since we would update the refs one ref at a time we would first fail the ENOTDIR and then fail the second update of HEAD as well. But since the first ref failed with ENOTDIR we would eventually fail the who fetch with STORE_REF_ERROR_DF_CONFLICT

In the new code, since we defer committing the transaction until all refs have been processed, we would now detect that the second ref was bad and rollback the transaction before we would even try start writing the update t disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.

I think this new behaviour is more correct, since if there was a problem we would not even try to commit the transaction but need to highlight this change in how/what errors are reported. This change in what error is returned only occurs if there are multiple refs that fail to update and only some, but not all, of them fail due to ENOTDIR.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b41d7b7..5dbd0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 static int shown_url;
+struct ref_transaction *transaction;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
 			struct ref *ref,
 			int check_old)
 {
-	char msg[1024];
-	char *rla = getenv("GIT_REFLOG_ACTION");
-	struct ref_transaction *transaction;
-
 	if (dry_run)
 		return 0;
-	if (!rla)
-		rla = default_rla.buf;
-	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
-	errno = 0;
-	transaction = ref_transaction_begin();
-	if (!transaction ||
-	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old) ||
-	    ref_transaction_commit(transaction, msg, NULL)) {
-		ref_transaction_rollback(transaction);
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	}
-	ref_transaction_free(transaction);
+	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+			       ref->old_sha1, 0, check_old))
+		return STORE_REF_ERROR_OTHER;
+
 	return 0;
 }
 
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		goto abort;
 	}
 
+	errno = 0;
+	transaction = ref_transaction_begin();
+	if (!transaction) {
+		rc = error(_("cannot start ref transaction\n"));
+		goto abort;
+	}
+
 	/*
 	 * We do a pass for each fetch_head_status type in their enum order, so
 	 * merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
+	if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+		rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+		  STORE_REF_ERROR_OTHER;
+	ref_transaction_free(transaction);
 
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
-- 
2.0.0.rc3.477.g0f8edf7
Previous: Ronnie SahlbergNext: Jonathan Nieder
Message 59 of 139 in “Use ref transactions for all ref updates”
  1. 00/44 Use ref transactions for all ref updatesRonnie Sahlberg, May 15, 2014
  2. 01/44 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, May 15, 2014
  3. Jonathan NiederMay 15, 2014
  4. 02/44 refs.c: allow passing NULL to ref_transaction_freeRonnie Sahlberg, May 15, 2014
  5. Jonathan NiederMay 15, 2014
  6. Ronnie SahlbergMay 15, 2014
  7. 03/44 refs.c: add a strbuf argument to ref_transaction_commit for error loggingRonnie Sahlberg, May 15, 2014
  8. 04/44 refs.c: add an err argument to repack_without_refsRonnie Sahlberg, May 15, 2014
  9. Jonathan NiederMay 15, 2014
  10. Ronnie SahlbergMay 15, 2014
  11. 05/44 refs.c: make ref_update_reject_duplicates take a strbuf argument for errorsRonnie Sahlberg, May 15, 2014
  12. 06/44 refs.c: add an err argument ro delete_loose_refRonnie Sahlberg, May 15, 2014
  13. Jonathan NiederMay 15, 2014
  14. Ronnie SahlbergMay 15, 2014
  15. 07/44 refs.c: make update_ref_write update a strbuf on failureRonnie Sahlberg, May 15, 2014
  16. 08/44 update-ref.c: log transaction error from the update_refRonnie Sahlberg, May 15, 2014
  17. Jonathan NiederMay 15, 2014
  18. 09/44 refs.c: remove the onerr argument to ref_transaction_commitRonnie Sahlberg, May 15, 2014
  19. 10/44 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, May 15, 2014
  20. Jonathan NiederMay 15, 2014
  21. Ronnie SahlbergMay 15, 2014
  22. 11/44 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, May 15, 2014
  23. Jonathan NiederMay 15, 2014
  24. Ronnie SahlbergMay 15, 2014
  25. 12/44 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, May 15, 2014
  26. Jonathan NiederMay 15, 2014
  27. Ronnie SahlbergMay 15, 2014
  28. 13/44 tag.c: use ref transactions when doing updatesRonnie Sahlberg, May 15, 2014
  29. Jonathan NiederMay 15, 2014
  30. Ronnie SahlbergMay 15, 2014
  31. 14/44 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, May 15, 2014
  32. Jonathan NiederMay 15, 2014
  33. Ronnie SahlbergMay 15, 2014
  34. 15/44 commit.c: use ref transactions for updatesRonnie Sahlberg, May 15, 2014
  35. Jonathan NiederMay 15, 2014
  36. 16/44 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, May 15, 2014
  37. Jonathan NiederMay 15, 2014
  38. 17/44 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, May 15, 2014
  39. Jonathan NiederMay 15, 2014
  40. Ronnie SahlbergMay 15, 2014
  41. 18/44 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, May 15, 2014
  42. Jonathan NiederMay 15, 2014
  43. 19/44 refs.c: change update_ref to use a transactionRonnie Sahlberg, May 15, 2014
  44. Jonathan NiederMay 15, 2014
  45. 20/44 refs.c: free the transaction before returning when number of updates is 0Ronnie Sahlberg, May 15, 2014
  46. 21/44 refs.c: ref_transaction_commit should not free the transactionRonnie Sahlberg, May 15, 2014
  47. Jonathan NiederMay 16, 2014
  48. Ronnie SahlbergMay 16, 2014
  49. Ronnie SahlbergMay 16, 2014
  50. 22/44 fetch.c: clear errno before calling functions that might set itRonnie Sahlberg, May 15, 2014
  51. Jonathan NiederMay 16, 2014
  52. Ronnie SahlbergMay 16, 2014
  53. Jeff KingMay 16, 2014
  54. 23/44 fetch.c: change s_update_ref to use a ref transactionRonnie Sahlberg, May 15, 2014
  55. Jonathan NiederMay 16, 2014
  56. Ronnie SahlbergMay 16, 2014
  57. Jonathan NiederMay 16, 2014
  58. Ronnie SahlbergMay 19, 2014
  59. 24/44 fetch.c: use a single ref transaction for all ref updatesRonnie Sahlberg, May 15, 2014
  60. Jonathan NiederMay 16, 2014
  61. Ronnie SahlbergMay 19, 2014
  62. 25/44 receive-pack.c: use a reference transaction for updating the refsRonnie Sahlberg, May 15, 2014
  63. Jonathan NiederMay 20, 2014
  64. Ronnie SahlbergMay 20, 2014
  65. Ronnie SahlbergMay 21, 2014
  66. 26/44 fast-import.c: use a ref transaction when dumping tagsRonnie Sahlberg, May 15, 2014
  67. Jonathan NiederMay 20, 2014
  68. Ronnie SahlbergMay 20, 2014
  69. 27/44 walker.c: use ref transaction for ref updatesRonnie Sahlberg, May 15, 2014
  70. Jonathan NiederMay 21, 2014
  71. Ronnie SahlbergMay 21, 2014
  72. 28/44 refs.c: make write_ref_sha1 staticRonnie Sahlberg, May 15, 2014
  73. Jonathan NiederMay 21, 2014
  74. Ronnie SahlbergMay 21, 2014
  75. 29/44 refs.c: make lock_ref_sha1 staticRonnie Sahlberg, May 15, 2014
  76. Jonathan NiederMay 21, 2014
  77. 30/44 refs.c: add transaction.status and track OPEN/CLOSED/ERRORRonnie Sahlberg, May 15, 2014
  78. Jonathan NiederMay 21, 2014
  79. Ronnie SahlbergMay 21, 2014
  80. Jonathan NiederMay 21, 2014
  81. Ronnie SahlbergMay 22, 2014
  82. 31/44 refs.c: remove the update_ref_lock functionRonnie Sahlberg, May 15, 2014
  83. Jonathan NiederMay 21, 2014
  84. 32/44 refs.c: remove the update_ref_write functionRonnie Sahlberg, May 15, 2014
  85. Jonathan NiederMay 21, 2014
  86. Ronnie SahlbergMay 22, 2014
  87. 33/44 refs.c: remove lock_ref_sha1Ronnie Sahlberg, May 15, 2014
  88. Jonathan NiederMay 21, 2014
  89. 34/44 refs.c: make prune_ref use a transaction to delete the refRonnie Sahlberg, May 15, 2014
  90. Jonathan NiederMay 21, 2014
  91. Ronnie SahlbergMay 22, 2014
  92. 35/44 refs.c: make delete_ref use a transactionRonnie Sahlberg, May 15, 2014
  93. Jonathan NiederMay 21, 2014
  94. Ronnie SahlbergMay 22, 2014
  95. Ronnie SahlbergMay 22, 2014
  96. 36/44 refs.c: pass the ref log message to _create/delete/update instead of _commitRonnie Sahlberg, May 15, 2014
  97. Jonathan NiederMay 21, 2014
  98. Ronnie SahlbergMay 22, 2014
  99. 37/44 refs.c: pass NULL as *flags to read_ref_fullRonnie Sahlberg, May 15, 2014
  100. Jonathan NiederMay 21, 2014
  101. 38/44 refs.c: pack all refs before we start to rename a refRonnie Sahlberg, May 15, 2014
  102. Jonathan NiederMay 21, 2014
  103. Ronnie SahlbergMay 22, 2014
  104. Jonathan NiederMay 22, 2014
  105. Ronnie SahlbergMay 22, 2014
  106. 39/44 refs.c: move the check for valid refname to lock_ref_sha1_basicRonnie Sahlberg, May 15, 2014
  107. Jonathan NiederMay 22, 2014
  108. Ronnie SahlbergMay 22, 2014
  109. Jonathan NiederMay 22, 2014
  110. Ronnie SahlbergMay 22, 2014
  111. 40/44 refs.c: call lock_ref_sha1_basic directly from commitRonnie Sahlberg, May 15, 2014
  112. Jonathan NiederMay 22, 2014
  113. 41/44 refs.c: add a new flag for transaction delete for refs we know are packed onlyRonnie Sahlberg, May 15, 2014
  114. Jonathan NiederMay 22, 2014
  115. Ronnie SahlbergMay 22, 2014
  116. Ronnie SahlbergMay 22, 2014
  117. Jonathan NiederMay 22, 2014
  118. Jonathan NiederMay 22, 2014
  119. Ronnie SahlbergMay 23, 2014
  120. Jonathan NiederMay 23, 2014
  121. Michael HaggertyMay 23, 2014
  122. Jonathan NiederMay 23, 2014
  123. Michael HaggertyMay 23, 2014
  124. Junio C HamanoMay 27, 2014
  125. Michael HaggertyMay 28, 2014
  126. Junio C HamanoMay 28, 2014
  127. Michael HaggertyMay 28, 2014
  128. 42/44 refs.c: pass a skip list to name_conflict_fnRonnie Sahlberg, May 15, 2014
  129. Jonathan NiederMay 22, 2014
  130. Ronnie SahlbergMay 27, 2014
  131. 43/44 refs.c: make rename_ref use a transactionRonnie Sahlberg, May 15, 2014
  132. 44/44 refs.c: remove forward declaration of write_ref_sha1Ronnie Sahlberg, May 15, 2014
  133. Jonathan NiederMay 15, 2014
  134. Junio C HamanoMay 15, 2014
  135. Jonathan NiederMay 22, 2014
  136. Jonathan NiederMay 22, 2014
  137. Jonathan NiederMay 22, 2014
  138. Jonathan NiederMay 22, 2014
  139. Ronnie SahlbergMay 27, 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.