commit_packed_refs(): report errors rather than dying

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

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))
+	if (commit_packed_refs(refs->packed_ref_store, &err))
-		die_errno("unable to overwrite old ref-pack file");
+		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)) {
+	if (commit_packed_refs(refs->packed_ref_store, err)) {
-		strbuf_addf(err, "unable to commit packed-refs file: %s",
-			    strerror(errno));
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}

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,
+static int write_packed_entry(FILE *fh, const char *refname,
-			       const unsigned char *sha1,
+			      const unsigned char *sha1,
-			       const unsigned char *peeled)
+			      const unsigned char *peeled)
 {
-	fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
+	if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
-	if (peeled)
+	    (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
-		fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
+		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
+ * lock_packed_refs()). Return zero on success. On errors, rollback
- * and return a nonzero value.
+ * 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 ok;
-	int save_errno = 0;
+	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)
+	if (!out) {
-		die_errno("unable to fdopen packed-refs descriptor");
+		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,
+		if (write_packed_entry(out, iter->refname, iter->oid->hash,
-				   peel_error ? NULL : peeled.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)
+	if (ok != ITER_DONE) {
-		die("error while iterating over references");
+		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;
+		strbuf_addf(err, "error overwriting %s: %s",
-		error = -1;
+			    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 ret;
-	return error;
 }
 
 /*
@@ -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);
+	return commit_packed_refs(&refs->base, err);
-	if (ret)
-		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
-			    strerror(errno));
-	return ret;
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)

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);