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

[PATCH v3 21/30] commit_packed_refs(): report errors rather than dying

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jul 1, 2017, 18:30 UTC
Message-ID
<6adc870c99c80bffcadf34637b98126b698e27e4.1498933362.git.mhagger@alum.mit.edu>
In-Reply-To
<cover.1498933362.git.mhagger@alum.mit.edu>

Report errors via a `struct strbuf *err` rather than by calling `die()`. To enable this goal, change `write_packed_entry()` to report errors via a return value and `errno` rather than dying.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  | 10 +++---
 refs/packed-backend.c | 85 +++++++++++++++++++++++++++++++++------------------
 refs/packed-backend.h |  2 +-
 3 files changed, 61 insertions(+), 36 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 60f4fa5e7a..2810785efc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1094,6 +1094,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	struct ref_iterator *iter;
 	int ok;
 	struct ref_to_prune *refs_to_prune = NULL;
+	struct strbuf err = STRBUF_INIT;
 
 	lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
@@ -1128,10 +1129,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	if (ok != ITER_DONE)
 		die("error while iterating over references");
 
-	if (commit_packed_refs(refs->packed_ref_store))
-		die_errno("unable to overwrite old ref-pack file");
+	if (commit_packed_refs(refs->packed_ref_store, &err))
+		die("unable to overwrite old ref-pack file: %s", err.buf);
 
 	prune_refs(refs, refs_to_prune);
+	strbuf_release(&err);
 	return 0;
 }
 
@@ -2693,9 +2695,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 				       &update->new_oid);
 	}
 
-	if (commit_packed_refs(refs->packed_ref_store)) {
-		strbuf_addf(err, "unable to commit packed-refs file: %s",
-			    strerror(errno));
+	if (commit_packed_refs(refs->packed_ref_store, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4676dc3959..18ce47fcb7 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -493,15 +493,19 @@ static struct ref_iterator *packed_ref_iterator_begin(
 
 /*
  * Write an entry to the packed-refs file for the specified refname.
- * If peeled is non-NULL, write it as the entry's peeled value.
+ * If peeled is non-NULL, write it as the entry's peeled value. On
+ * error, return a nonzero value and leave errno set at the value left
+ * by the failing call to `fprintf()`.
  */
-static void write_packed_entry(FILE *fh, const char *refname,
-			       const unsigned char *sha1,
-			       const unsigned char *peeled)
+static int write_packed_entry(FILE *fh, const char *refname,
+			      const unsigned char *sha1,
+			      const unsigned char *peeled)
 {
-	fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
-	if (peeled)
-		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
+	if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
+	    (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
+		return -1;
+
+	return 0;
 }
 
 int lock_packed_refs(struct ref_store *ref_store, int flags)
@@ -550,49 +554,74 @@ static const char PACKED_REFS_HEADER[] =
 /*
  * Write the current version of the packed refs cache from memory to
  * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, set errno
- * and return a nonzero value.
+ * lock_packed_refs()). Return zero on success. On errors, rollback
+ * the lockfile, write an error message to `err`, and return a nonzero
+ * value.
  */
-int commit_packed_refs(struct ref_store *ref_store)
+int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 {
 	struct packed_ref_store *refs =
 		packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
 				"commit_packed_refs");
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
-	int ok, error = 0;
-	int save_errno = 0;
+	int ok;
+	int ret = -1;
 	FILE *out;
 	struct ref_iterator *iter;
 
 	if (!is_lock_file_locked(&refs->lock))
-		die("BUG: packed-refs not locked");
+		die("BUG: commit_packed_refs() called when unlocked");
 
 	out = fdopen_lock_file(&refs->lock, "w");
-	if (!out)
-		die_errno("unable to fdopen packed-refs descriptor");
+	if (!out) {
+		strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
+			    strerror(errno));
+		goto error;
+	}
 
-	fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
+	if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
+		strbuf_addf(err, "error writing to %s: %s",
+			    get_lock_file_path(&refs->lock), strerror(errno));
+		goto error;
+	}
 
 	iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		struct object_id peeled;
 		int peel_error = ref_iterator_peel(iter, &peeled);
 
-		write_packed_entry(out, iter->refname, iter->oid->hash,
-				   peel_error ? NULL : peeled.hash);
+		if (write_packed_entry(out, iter->refname, iter->oid->hash,
+				       peel_error ? NULL : peeled.hash)) {
+			strbuf_addf(err, "error writing to %s: %s",
+				    get_lock_file_path(&refs->lock),
+				    strerror(errno));
+			ref_iterator_abort(iter);
+			goto error;
+		}
 	}
 
-	if (ok != ITER_DONE)
-		die("error while iterating over references");
+	if (ok != ITER_DONE) {
+		strbuf_addf(err, "unable to write packed-refs file: "
+			    "error iterating over old contents");
+		goto error;
+	}
 
 	if (commit_lock_file(&refs->lock)) {
-		save_errno = errno;
-		error = -1;
+		strbuf_addf(err, "error overwriting %s: %s",
+			    refs->path, strerror(errno));
+		goto out;
 	}
+
+	ret = 0;
+	goto out;
+
+error:
+	rollback_lock_file(&refs->lock);
+
+out:
 	release_packed_ref_cache(packed_ref_cache);
-	errno = save_errno;
-	return error;
+	return ret;
 }
 
 /*
@@ -628,7 +657,7 @@ int repack_without_refs(struct ref_store *ref_store,
 				"repack_without_refs");
 	struct ref_dir *packed;
 	struct string_list_item *refname;
-	int ret, needs_repacking = 0, removed = 0;
+	int needs_repacking = 0, removed = 0;
 
 	packed_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
@@ -665,11 +694,7 @@ int repack_without_refs(struct ref_store *ref_store,
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs(&refs->base);
-	if (ret)
-		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
-			    strerror(errno));
-	return ret;
+	return commit_packed_refs(&refs->base, err);
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index beea9c14b5..3d4057b65b 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -14,7 +14,7 @@ int lock_packed_refs(struct ref_store *ref_store, int flags);
 void add_packed_ref(struct ref_store *ref_store,
 		    const char *refname, const struct object_id *oid);
 
-int commit_packed_refs(struct ref_store *ref_store);
+int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err);
 
 int repack_without_refs(struct ref_store *ref_store,
 			struct string_list *refnames, struct strbuf *err);

-- 
2.11.0
Previous: Michael HaggertyNext: Michael Haggerty
Message 29 of 52 in “Create a reference backend for packed refs”
  1. 00/30 Create a reference backend for packed refsMichael Haggerty, Jul 1, 2017
  2. 01/30 t1408: add a test of stale packed refs covered by loose refsMichael Haggerty, Jul 1, 2017
  3. 03/30 packed_ref_store: new structMichael Haggerty, Jul 1, 2017
  4. 04/30 packed_ref_store: move `packed_refs_path` hereMichael Haggerty, Jul 1, 2017
  5. 02/30 add_packed_ref(): teach function to overwrite existing refsMichael Haggerty, Jul 1, 2017
  6. 06/30 clear_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  7. 05/30 packed_ref_store: move `packed_refs_lock` member hereMichael Haggerty, Jul 1, 2017
  8. 09/30 get_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  9. 07/30 validate_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  10. 12/30 commit_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  11. 08/30 get_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  12. 10/30 add_packed_ref(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  13. 11/30 lock_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  14. 16/30 packed_peel_ref(): new function, extracted from `files_peel_ref()`Michael Haggerty, Jul 1, 2017
  15. 15/30 repack_without_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  16. 14/30 get_packed_ref(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  17. 17/30 packed_ref_store: support iterationMichael Haggerty, Jul 1, 2017
  18. 19/30 packed-backend: new module for handling packed referencesMichael Haggerty, Jul 1, 2017
  19. 18/30 packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`Michael Haggerty, Jul 1, 2017
  20. 22/30 commit_packed_refs(): use a staging file separate from the lockfileMichael Haggerty, Jul 1, 2017
  21. 20/30 packed_ref_store: make class into a subclass of `ref_store`Michael Haggerty, Jul 1, 2017
  22. 13/30 rollback_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jul 1, 2017
  23. 23/30 packed_refs_lock(): function renamed from lock_packed_refs()Michael Haggerty, Jul 1, 2017
  24. 28/30 repack_without_refs(): don't lock or unlock the packed refsMichael Haggerty, Jul 1, 2017
  25. 29/30 t3210: add some tests of bogus packed-refs file contentsMichael Haggerty, Jul 1, 2017
  26. 26/30 clear_packed_ref_cache(): don't protest if the lock is heldMichael Haggerty, Jul 1, 2017
  27. 30/30 read_packed_refs(): die if `packed-refs` contains bogus dataMichael Haggerty, Jul 1, 2017
  28. 27/30 commit_packed_refs(): remove call to `packed_refs_unlock()`Michael Haggerty, Jul 1, 2017
  29. 21/30 commit_packed_refs(): report errors rather than dyingMichael Haggerty, Jul 1, 2017
  30. 25/30 packed_refs_unlock(), packed_refs_is_locked(): new functionsMichael Haggerty, Jul 1, 2017
  31. 24/30 packed_refs_lock(): report errors via a `struct strbuf *err`Michael Haggerty, Jul 1, 2017
  32. Jeff KingJul 5, 2017
  33. Stefan BellerJul 20, 2017
  34. Jonathan NiederJul 20, 2017
  35. packed_ref_store: handle a packed-refs file that is a symlinkMichael Haggerty, Jul 26, 2017
  36. Stefan BellerJul 27, 2017
  37. Jonathan NiederJul 27, 2017
  38. Michael HaggertyJul 27, 2017
  39. Junio C HamanoJul 27, 2017
  40. Jeff KingJul 27, 2017
  41. Junio C HamanoJul 27, 2017
  42. Michael HaggertyJul 28, 2017
  43. Ævar Arnfjörð BjarmasonMay 31, 2021
  44. Jeff KingJun 3, 2021
  45. t: use portable wrapper for readlink(1)Jeff King, Jun 3, 2021
  46. Ævar Arnfjörð BjarmasonJun 4, 2021
  47. Felipe ContrerasJun 3, 2021
  48. Jeff KingJun 3, 2021
  49. Felipe ContrerasJun 3, 2021
  50. Ævar Arnfjörð BjarmasonJun 4, 2021
  51. Felipe ContrerasJun 5, 2021
  52. Ævar Arnfjörð BjarmasonJun 4, 2021

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.