[PATCH 0/5] ref-transactions-req-strbuf-err

[PATCH 0/5] ref-transactions-req-strbuf-err

[Ronnie Sahlberg]

List,

This is the next patch series in the ref transaction work.
This patch series is called ref-transactions-req-strbuf-err and builds ontop
of the series called ref-transactions-req-packed-refs which is origin/pu


This patch series mainly adds some nice strbuf arguments to some functions to
pass errors back to callers.
The only thing noteworthy is that we finally get to remove
-enum action_on_err {
-       UPDATE_REFS_MSG_ON_ERR,
-       UPDATE_REFS_DIE_ON_ERR,
-       UPDATE_REFS_QUIET_ON_ERR
-};

aside from that there is little/nothing much interesting in there.


Ronnie Sahlberg (5):
  refs.c: replace the onerr argument in update_ref with a strbuf err
  refs.c: make add_packed_ref return an error instead of calling die
  refs.c: make lock_packed_refs take an err argument
  refs.c: add an err argument to commit_packed_refs
  refs.c: add an err argument to pack_refs

 builtin/checkout.c   |   7 ++-
 builtin/clone.c      |  23 +++++---
 builtin/merge.c      |  20 ++++---
 builtin/notes.c      |  24 +++++----
 builtin/pack-refs.c  |   8 ++-
 builtin/reset.c      |  12 +++--
 builtin/update-ref.c |   7 ++-
 notes-cache.c        |   2 +-
 notes-utils.c        |   5 +-
 refs.c               | 148 +++++++++++++++++++++++++++++----------------------
 refs.h               |  13 ++---
 transport-helper.c   |   7 ++-
 transport.c          |   9 ++--
 13 files changed, 170 insertions(+), 115 deletions(-)

-- 
2.0.1.523.g70700c9

[PATCH 1/5] refs.c: replace the onerr argument in update_ref with a strbuf err

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/checkout.c   |  7 +++++--
 builtin/clone.c      | 23 +++++++++++++++--------
 builtin/merge.c      | 20 +++++++++++++-------
 builtin/notes.c      | 24 ++++++++++++++----------
 builtin/reset.c      | 12 ++++++++----
 builtin/update-ref.c |  7 +++++--
 notes-cache.c        |  2 +-
 notes-utils.c        |  5 +++--
 refs.c               | 14 +++-----------
 refs.h               | 10 ++--------
 transport-helper.c   |  7 ++++++-
 transport.c          |  9 ++++++---
 12 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 808c58f..a9ec5be 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -580,6 +580,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 {
 	struct strbuf msg = STRBUF_INIT;
 	const char *old_desc, *reflog_msg;
+	struct strbuf err = STRBUF_INIT;
+
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
@@ -617,8 +619,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
 		/* Nothing to do. */
 	} else if (opts->force_detach || !new->path) {	/* No longer on any branch. */
-		update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
-			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
+			       REF_NODEREF, &err))
+			die("%s", err.buf);
 		if (!opts->quiet) {
 			if (old->path && advice_detached_head)
 				detach_advice(new->name);
diff --git a/builtin/clone.c b/builtin/clone.c
index 7737e12..99e23cf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -521,6 +521,7 @@ static void write_remote_refs(const struct ref *local_refs)
 static void write_followtags(const struct ref *refs, const char *msg)
 {
 	const struct ref *ref;
+	struct strbuf err = STRBUF_INIT;
 	for (ref = refs; ref; ref = ref->next) {
 		if (!starts_with(ref->name, "refs/tags/"))
 			continue;
@@ -528,8 +529,9 @@ static void write_followtags(const struct ref *refs, const char *msg)
 			continue;
 		if (!has_sha1_file(ref->old_sha1))
 			continue;
-		update_ref(msg, ref->name, ref->old_sha1,
-			   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref(msg, ref->name, ref->old_sha1,
+			       NULL, 0, &err))
+			die("%s", err.buf);
 	}
 }
 
@@ -592,28 +594,33 @@ static void update_remote_refs(const struct ref *refs,
 static void update_head(const struct ref *our, const struct ref *remote,
 			const char *msg)
 {
+	struct strbuf err = STRBUF_INIT;
+
 	if (our && starts_with(our->name, "refs/heads/")) {
 		/* Local default branch link */
 		create_symref("HEAD", our->name, NULL);
 		if (!option_bare) {
 			const char *head = skip_prefix(our->name, "refs/heads/");
-			update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
-				   UPDATE_REFS_DIE_ON_ERR);
+			if (update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
+				       &err))
+				die("%s", err.buf);
 			install_branch_config(0, head, option_origin, our->name);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(our->old_sha1);
 		/* --branch specifies a non-branch (i.e. tags), detach HEAD */
-		update_ref(msg, "HEAD", c->object.sha1,
-			   NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref(msg, "HEAD", c->object.sha1,
+			       NULL, REF_NODEREF, &err))
+			die("%s", err.buf);
 	} else if (remote) {
 		/*
 		 * We know remote HEAD points to a non-branch, or
 		 * HEAD points to a branch but we don't know which one.
 		 * Detach HEAD in all these cases.
 		 */
-		update_ref(msg, "HEAD", remote->old_sha1,
-			   NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+	  if (update_ref(msg, "HEAD", remote->old_sha1,
+			 NULL, REF_NODEREF, &err))
+		die("%s", err.buf);
 	}
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..17dda64 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -396,9 +396,11 @@ static void finish(struct commit *head_commit,
 			printf(_("No merge message -- not updating HEAD\n"));
 		else {
 			const char *argv_gc_auto[] = { "gc", "--auto", NULL };
-			update_ref(reflog_message.buf, "HEAD",
-				new_head, head, 0,
-				UPDATE_REFS_DIE_ON_ERR);
+			struct strbuf err = STRBUF_INIT;
+			if (update_ref(reflog_message.buf, "HEAD",
+				       new_head, head, 0,
+				       &err))
+				die("%s", err.buf);
 			/*
 			 * We ignore errors in 'gc --auto', since the
 			 * user should see them.
@@ -1093,6 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	unsigned char head_sha1[20];
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	const char *head_arg;
 	int flag, i, ret = 0, head_subsumed;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
@@ -1221,8 +1224,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (!remote_head)
 			die(_("%s - not something we can merge"), argv[0]);
 		read_empty(remote_head->object.sha1, 0);
-		update_ref("initial pull", "HEAD", remote_head->object.sha1,
-			   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref("initial pull", "HEAD", remote_head->object.sha1,
+			       NULL, 0, &err))
+			die("%s", err.buf);
 		goto done;
 	} else {
 		struct strbuf merge_names = STRBUF_INIT;
@@ -1338,8 +1342,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		free(list);
 	}
 
-	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
-		   NULL, 0, UPDATE_REFS_DIE_ON_ERR);
+	if (update_ref("updating ORIG_HEAD", "ORIG_HEAD",
+		       head_commit->object.sha1,
+		       NULL, 0, &err))
+		die("%s", err.buf);
 
 	if (remoteheads && !common)
 		; /* No common ancestors found. We need a real merge. */
diff --git a/builtin/notes.c b/builtin/notes.c
index 820c341..f508fbd 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -675,6 +675,7 @@ static int merge_abort(struct notes_merge_options *o)
 static int merge_commit(struct notes_merge_options *o)
 {
 	struct strbuf msg = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	unsigned char sha1[20], parent_sha1[20];
 	struct notes_tree *t;
 	struct commit *partial;
@@ -715,10 +716,10 @@ static int merge_commit(struct notes_merge_options *o)
 	format_commit_message(partial, "%s", &msg, &pretty_ctx);
 	strbuf_trim(&msg);
 	strbuf_insert(&msg, 0, "notes: ", 7);
-	update_ref(msg.buf, o->local_ref, sha1,
-		   is_null_sha1(parent_sha1) ? NULL : parent_sha1,
-		   0, UPDATE_REFS_DIE_ON_ERR);
-
+	if (update_ref(msg.buf, o->local_ref, sha1,
+		       is_null_sha1(parent_sha1) ? NULL : parent_sha1,
+		       0, &err))
+		die("%s", err.buf);
 	free_notes(t);
 	strbuf_release(&msg);
 	ret = merge_abort(o);
@@ -729,6 +730,7 @@ static int merge_commit(struct notes_merge_options *o)
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	unsigned char result_sha1[20];
 	struct notes_tree *t;
 	struct notes_merge_options o;
@@ -809,14 +811,16 @@ static int merge(int argc, const char **argv, const char *prefix)
 
 	result = notes_merge(&o, t, result_sha1);
 
-	if (result >= 0) /* Merge resulted (trivially) in result_sha1 */
+	if (result >= 0) {/* Merge resulted (trivially) in result_sha1 */
 		/* Update default notes ref with new commit */
-		update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
-			   0, UPDATE_REFS_DIE_ON_ERR);
-	else { /* Merge has unresolved conflicts */
+		if (update_ref(msg.buf, default_notes_ref(), result_sha1, NULL,
+			       0, &err))
+			die("%s", err.buf);
+	} else { /* Merge has unresolved conflicts */
 		/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
-		update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
-			   0, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
+			       0, &err))
+			die("%s", err.buf);
 		/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
 		if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
 			die("Failed to store link to current notes ref (%s)",
diff --git a/builtin/reset.c b/builtin/reset.c
index f368266..0e8775d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -244,6 +244,7 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
 {
 	int update_ref_status;
 	struct strbuf msg = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	unsigned char *orig = NULL, sha1_orig[20],
 		*old_orig = NULL, sha1_old_orig[20];
 
@@ -252,13 +253,16 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
 	if (!get_sha1("HEAD", sha1_orig)) {
 		orig = sha1_orig;
 		set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
-		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
-			   UPDATE_REFS_MSG_ON_ERR);
+		if (update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, &err))
+			error("%s", err.buf);
+		strbuf_release(&err);
 	} else if (old_orig)
 		delete_ref("ORIG_HEAD", old_orig, 0);
 	set_reflog_message(&msg, "updating HEAD", rev);
-	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0,
-				       UPDATE_REFS_MSG_ON_ERR);
+	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, &err);
+	if (update_ref_status)
+		error("%s", err.buf);
+	strbuf_release(&err);
 	strbuf_release(&msg);
 	return update_ref_status;
 }
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index d62871d..062a29c 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -349,6 +349,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	const char *refname, *oldval;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -406,6 +407,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (delete)
 		return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
 	else
-		return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
-				  flags, UPDATE_REFS_DIE_ON_ERR);
+		if (update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
+			       flags, &err))
+			die("%s", err.buf);
+	return 0;
 }
diff --git a/notes-cache.c b/notes-cache.c
index 97dfd63..882b56d 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -62,7 +62,7 @@ int notes_cache_write(struct notes_cache *c)
 	if (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)
 		return -1;
 	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
-		       0, UPDATE_REFS_QUIET_ON_ERR) < 0)
+		       0, NULL) < 0)
 		return -1;
 
 	return 0;
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..805bc31 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -33,6 +33,7 @@ void commit_notes(struct notes_tree *t, const char *msg)
 {
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char commit_sha1[20];
+	struct strbuf err = STRBUF_INIT;
 
 	if (!t)
 		t = &default_notes_tree;
@@ -48,8 +49,8 @@ void commit_notes(struct notes_tree *t, const char *msg)
 
 	create_notes_commit(t, NULL, &buf, commit_sha1);
 	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
-	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0,
-		   UPDATE_REFS_DIE_ON_ERR);
+	if (update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, &err))
+		die("%s", err.buf);
 
 	strbuf_release(&buf);
 }
diff --git a/refs.c b/refs.c
index 46a9595..65eee72 100644
--- a/refs.c
+++ b/refs.c
@@ -3549,7 +3549,7 @@ int transaction_delete_sha1(struct ref_transaction *transaction,
 
 int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
-	       int flags, enum action_on_err onerr)
+	       int flags, struct strbuf *e)
 {
 	struct ref_transaction *t;
 	struct strbuf err = STRBUF_INIT;
@@ -3562,16 +3562,8 @@ int update_ref(const char *action, const char *refname,
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		transaction_free(t);
-		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR:
-			error(str, refname, err.buf);
-			break;
-		case UPDATE_REFS_DIE_ON_ERR:
-			die(str, refname, err.buf);
-			break;
-		case UPDATE_REFS_QUIET_ON_ERR:
-			break;
-		}
+		if (e)
+			strbuf_addf(e, str, refname, err.buf);
 		strbuf_release(&err);
 		return 1;
 	}
diff --git a/refs.h b/refs.h
index 54dbe4b..dee9a98 100644
--- a/refs.h
+++ b/refs.h
@@ -202,12 +202,6 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg
  */
 extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);
 
-enum action_on_err {
-	UPDATE_REFS_MSG_ON_ERR,
-	UPDATE_REFS_DIE_ON_ERR,
-	UPDATE_REFS_QUIET_ON_ERR
-};
-
 /*
  * Begin a reference transaction.  The reference transaction must
  * be freed by calling transaction_free().
@@ -332,8 +326,8 @@ void transaction_free(struct ref_transaction *transaction);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-		const unsigned char *sha1, const unsigned char *oldval,
-		int flags, enum action_on_err onerr);
+	       const unsigned char *sha1, const unsigned char *oldval,
+	       int flags, struct strbuf *err);
 
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
 extern int ref_is_hidden(const char *);
diff --git a/transport-helper.c b/transport-helper.c
index 270ae28..d46624a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -733,6 +733,7 @@ static int push_update_refs_status(struct helper_data *data,
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct ref *ref = remote_refs;
+	struct strbuf err = STRBUF_INIT;
 	int ret = 0;
 
 	for (;;) {
@@ -756,7 +757,11 @@ static int push_update_refs_status(struct helper_data *data,
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
 		if (!private)
 			continue;
-		update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0);
+		if (update_ref("update by helper", private, ref->new_sha1,
+			       NULL, 0, &err))
+			error("%s", err.buf);
+		strbuf_release(&err);
+
 		free(private);
 	}
 	strbuf_release(&buf);
diff --git a/transport.c b/transport.c
index f40e950..a3b7f48 100644
--- a/transport.c
+++ b/transport.c
@@ -607,6 +607,7 @@ int transport_refs_pushed(struct ref *ref)
 void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
 {
 	struct refspec rs;
+	struct strbuf err = STRBUF_INIT;
 
 	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
 		return;
@@ -619,9 +620,11 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
 			delete_ref(rs.dst, NULL, 0);
-		} else
-			update_ref("update by push", rs.dst,
-					ref->new_sha1, NULL, 0, 0);
+		} else if (update_ref("update by push", rs.dst,
+				      ref->new_sha1, NULL, 0, &err))
+			error("%s", err.buf);
+
+		strbuf_release(&err);
 		free(rs.dst);
 	}
 }
-- 
2.0.1.523.g70700c9

[PATCH 2/5] refs.c: make add_packed_ref return an error instead of calling die

[Ronnie Sahlberg]

Change add_packed_ref to return an error instead of calling die().
Update all callers to check the return value of add_packed_ref.

We can also skip checking the refname format since this function is now
static and only called from the transaction code.
If we are updating a ref and the refname is bad then we fail the transaction
already in transaction_update_sha1().
For the ref deletion case the only caveat is that we would not want
to move the badly named ref into the packed refs file during transaction
commit. This again is not a problem since if the name is bad, then
resolve_ref_unsafe() will fail which protects us from calling add_packed_ref()
with the bad name.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 65eee72..0aad8c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1135,17 +1135,16 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
 	return get_packed_ref_dir(get_packed_ref_cache(refs));
 }
 
-static void add_packed_ref(const char *refname, const unsigned char *sha1)
+static int add_packed_ref(const char *refname, const unsigned char *sha1)
 {
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(&ref_cache);
 
 	if (!packed_ref_cache->lock)
-		die("internal error: packed refs not locked");
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
-		die("Reference has invalid format: '%s'", refname);
+		return -1;
 	add_ref(get_packed_ref_dir(packed_ref_cache),
 		create_ref_entry(refname, sha1, REF_ISPACKED));
+	return 0;
 }
 
 /*
@@ -3666,7 +3665,13 @@ int transaction_commit(struct ref_transaction *transaction,
 					RESOLVE_REF_READING, NULL))
 			continue;
 
-		add_packed_ref(update->refname, sha1);
+		if (add_packed_ref(update->refname, sha1)) {
+			if (err)
+				strbuf_addf(err, "Failed to add %s to packed "
+					    "refs", update->refname);
+			ret = -1;
+			goto cleanup;
+		}
 		need_repack = 1;
 	}
 	if (need_repack) {
@@ -3778,7 +3783,13 @@ int transaction_commit(struct ref_transaction *transaction,
 
 		packed = get_packed_refs(&ref_cache);
 		remove_entry(packed, update->refname);
-		add_packed_ref(update->refname, update->new_sha1);
+		if (add_packed_ref(update->refname, update->new_sha1)) {
+			if (err)
+				strbuf_addf(err, "Failed to add %s to packed "
+					    "refs", update->refname);
+			ret = -1;
+			goto cleanup;
+		}
 		need_repack = 1;
 
 		try_remove_empty_parents((char *)update->refname);
-- 
2.0.1.523.g70700c9

[PATCH 3/5] refs.c: make lock_packed_refs take an err argument

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 0aad8c8..cfe1292 100644
--- a/refs.c
+++ b/refs.c
@@ -2270,13 +2270,17 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-/* This should return a meaningful errno on failure */
-static int lock_packed_refs(int flags)
+static int lock_packed_refs(struct strbuf *err)
 {
 	struct packed_ref_cache *packed_ref_cache;
 
-	if (hold_lock_file_for_update(&packlock, git_path("packed-refs"), flags) < 0)
+	if (hold_lock_file_for_update(&packlock, git_path("packed-refs"),
+				      0) < 0) {
+		if (err)
+			unable_to_lock_message(git_path("packed-refs"),
+					       errno, err);
 		return -1;
+	}
 	/*
 	 * Get the current packed-refs while holding the lock.  If the
 	 * packed-refs file has been modified since we last read it,
@@ -2460,11 +2464,14 @@ static void prune_refs(struct ref_to_prune *r)
 int pack_refs(unsigned int flags)
 {
 	struct pack_refs_cb_data cbdata;
+	struct strbuf err = STRBUF_INIT;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	lock_packed_refs(LOCK_DIE_ON_ERROR);
+	if (lock_packed_refs(&err))
+		die("%s", err.buf);
+
 	cbdata.packed_refs = get_packed_refs(&ref_cache);
 
 	do_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,
@@ -3626,10 +3633,7 @@ int transaction_commit(struct ref_transaction *transaction,
 	}
 
 	/* Lock packed refs during commit */
-	if (lock_packed_refs(0)) {
-		if (err)
-			unable_to_lock_message(git_path("packed-refs"),
-					       errno, err);
+	if (lock_packed_refs(err)) {
 		ret = -1;
 		goto cleanup;
 	}
@@ -3684,10 +3688,7 @@ int transaction_commit(struct ref_transaction *transaction,
 			goto cleanup;
 		}
 		/* lock the packed refs again so no one can change it */
-		if (lock_packed_refs(0)) {
-			if (err)
-				unable_to_lock_message(git_path("packed-refs"),
-						       errno, err);
+		if (lock_packed_refs(err)) {
 			ret = -1;
 			goto cleanup;
 		}
-- 
2.0.1.523.g70700c9

[PATCH 4/5] refs.c: add an err argument to commit_packed_refs

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 85 +++++++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

diff --git a/refs.c b/refs.c
index cfe1292..19e73f3 100644
--- a/refs.c
+++ b/refs.c
@@ -2232,8 +2232,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
  * 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.
  */
-static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
-			       unsigned char *peeled)
+static int write_packed_entry(int fd, char *refname, unsigned char *sha1,
+			      unsigned char *peeled, struct strbuf *err)
 {
 	char line[PATH_MAX + 100];
 	int len;
@@ -2241,33 +2241,50 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
 	len = snprintf(line, sizeof(line), "%s %s\n",
 		       sha1_to_hex(sha1), refname);
 	/* this should not happen but just being defensive */
-	if (len > sizeof(line))
-		die("too long a refname '%s'", refname);
-	write_or_die(fd, line, len);
-
+	if (len > sizeof(line)) {
+		strbuf_addf(err, "too long a refname '%s'", refname);
+		return -1;
+	}
+	if (write_in_full(fd, line, len) != len) {
+		strbuf_addf(err, "error writing to packed-refs. %s",
+			    strerror(errno));
+		return -1;
+	}
 	if (peeled) {
 		if (snprintf(line, sizeof(line), "^%s\n",
 			     sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
 			die("internal error");
-		write_or_die(fd, line, PEELED_LINE_LENGTH);
+		len = PEELED_LINE_LENGTH;
+		if (write_in_full(fd, line, len) != len) {
+			strbuf_addf(err, "error writing to packed-refs. %s",
+				    strerror(errno));
+			return -1;
+		}
 	}
+	return 0;
 }
 
+struct write_packed_data {
+	int fd;
+	struct strbuf *err;
+};
+
 /*
  * An each_ref_entry_fn that writes the entry to a packed-refs file.
  */
 static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
 {
-	int *fd = cb_data;
+	struct write_packed_data *data = cb_data;
 	enum peel_status peel_status = peel_entry(entry, 0);
 
-	if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
-		error("internal error: %s is not a valid packed reference!",
-		      entry->name);
-	write_packed_entry(*fd, entry->name, entry->u.value.sha1,
-			   peel_status == PEEL_PEELED ?
-			   entry->u.value.peeled : NULL);
-	return 0;
+	if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG) {
+		strbuf_addf(data->err, "internal error: %s is not a "
+			    "valid packed reference!", entry->name);
+		return -1;
+	}
+	return write_packed_entry(data->fd, entry->name, entry->u.value.sha1,
+				  peel_status == PEEL_PEELED ?
+				  entry->u.value.peeled : NULL, data->err);
 }
 
 static int lock_packed_refs(struct strbuf *err)
@@ -2296,30 +2313,34 @@ static int lock_packed_refs(struct strbuf *err)
 
 /*
  * Commit the packed refs changes.
- * On error we must make sure that errno contains a meaningful value.
  */
-static int commit_packed_refs(void)
+static int commit_packed_refs(struct strbuf *err)
 {
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(&ref_cache);
 	int error = 0;
-	int save_errno = 0;
+	struct write_packed_data data;
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
-	write_or_die(packed_ref_cache->lock->fd,
-		     PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
+	if (write_in_full(packed_ref_cache->lock->fd,
+			  PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER)) < 0) {
+		strbuf_addf(err, "error writing packed-refs. %s",
+			    strerror(errno));
+		return -1;
+       }
 
+	data.fd = packed_ref_cache->lock->fd;
+	data.err = err;
 	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
-				 0, write_packed_entry_fn,
-				 &packed_ref_cache->lock->fd);
+				 0, write_packed_entry_fn, &data);
 	if (commit_lock_file(packed_ref_cache->lock)) {
-		save_errno = errno;
+		strbuf_addf(err, "unable to overwrite old ref-pack "
+			    "file. %s", strerror(errno));
 		error = -1;
 	}
 	packed_ref_cache->lock = NULL;
 	release_packed_ref_cache(packed_ref_cache);
-	errno = save_errno;
 	return error;
 }
 
@@ -2477,8 +2498,8 @@ int pack_refs(unsigned int flags)
 	do_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,
 				 pack_if_possible_fn, &cbdata);
 
-	if (commit_packed_refs())
-		die_errno("unable to overwrite old ref-pack file");
+	if (commit_packed_refs(&err))
+		die("%s", err.buf);
 
 	prune_refs(cbdata.ref_to_prune);
 	return 0;
@@ -2549,7 +2570,7 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret;
+	int i;
 
 	/* Look for a packed ref */
 	for (i = 0; i < n; i++)
@@ -2572,11 +2593,7 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	}
 
 	/* Write what remains */
-	ret = commit_packed_refs();
-	if (ret && err)
-		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
-			    strerror(errno));
-	return ret;
+	return commit_packed_refs(err);
 }
 
 static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
@@ -3681,9 +3698,7 @@ int transaction_commit(struct ref_transaction *transaction,
 	if (need_repack) {
 		packed = get_packed_refs(&ref_cache);
 		sort_ref_dir(packed);
-		if (commit_packed_refs()){
-			strbuf_addf(err, "unable to overwrite old ref-pack "
-				    "file");
+		if (commit_packed_refs(err)){
 			ret = -1;
 			goto cleanup;
 		}
-- 
2.0.1.523.g70700c9

[PATCH 5/5] refs.c: add an err argument to pack_refs

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/pack-refs.c |  8 +++++++-
 refs.c              | 13 ++++++-------
 refs.h              |  3 ++-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index b20b1ec..da5d46a 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -10,6 +10,7 @@ static char const * const pack_refs_usage[] = {
 int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	unsigned int flags = PACK_REFS_PRUNE;
+	struct strbuf err = STRBUF_INIT;
 	struct option opts[] = {
 		OPT_BIT(0, "all",   &flags, N_("pack everything"), PACK_REFS_ALL),
 		OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
@@ -17,5 +18,10 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	};
 	if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
 		usage_with_options(pack_refs_usage, opts);
-	return pack_refs(flags);
+	if (pack_refs(flags, &err)) {
+		error("%s", err.buf);
+		strbuf_release(&err);
+		return 1;
+	}
+	return 0;
 }
diff --git a/refs.c b/refs.c
index 19e73f3..5875c29 100644
--- a/refs.c
+++ b/refs.c
@@ -2328,7 +2328,7 @@ static int commit_packed_refs(struct strbuf *err)
 		strbuf_addf(err, "error writing packed-refs. %s",
 			    strerror(errno));
 		return -1;
-       }
+	}
 
 	data.fd = packed_ref_cache->lock->fd;
 	data.err = err;
@@ -2482,24 +2482,23 @@ static void prune_refs(struct ref_to_prune *r)
 	}
 }
 
-int pack_refs(unsigned int flags)
+int pack_refs(unsigned int flags, struct strbuf *err)
 {
 	struct pack_refs_cb_data cbdata;
-	struct strbuf err = STRBUF_INIT;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	if (lock_packed_refs(&err))
-		die("%s", err.buf);
+	if (lock_packed_refs(err))
+		return -1;
 
 	cbdata.packed_refs = get_packed_refs(&ref_cache);
 
 	do_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,
 				 pack_if_possible_fn, &cbdata);
 
-	if (commit_packed_refs(&err))
-		die("%s", err.buf);
+	if (commit_packed_refs(err))
+		return -1;
 
 	prune_refs(cbdata.ref_to_prune);
 	return 0;
diff --git a/refs.h b/refs.h
index dee9a98..1a98e27 100644
--- a/refs.h
+++ b/refs.h
@@ -122,8 +122,9 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
 /*
  * Write a packed-refs file for the current repository.
  * flags: Combination of the above PACK_REFS_* flags.
+ * Returns 0 on success and fills in err on failure.
  */
-int pack_refs(unsigned int flags);
+int pack_refs(unsigned int flags, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.0.1.523.g70700c9

Re: [PATCH 0/5] ref-transactions-req-strbuf-err

[Ronnie Sahlberg]

Ping ?

On Thu, Jul 31, 2014 at 2:25 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> List,
>
> This is the next patch series in the ref transaction work.
> This patch series is called ref-transactions-req-strbuf-err and builds ontop
> of the series called ref-transactions-req-packed-refs which is origin/pu
>
>
> This patch series mainly adds some nice strbuf arguments to some functions to
> pass errors back to callers.
> The only thing noteworthy is that we finally get to remove
> -enum action_on_err {
> -       UPDATE_REFS_MSG_ON_ERR,
> -       UPDATE_REFS_DIE_ON_ERR,
> -       UPDATE_REFS_QUIET_ON_ERR
> -};
>
> aside from that there is little/nothing much interesting in there.
>
>
> Ronnie Sahlberg (5):
>   refs.c: replace the onerr argument in update_ref with a strbuf err
>   refs.c: make add_packed_ref return an error instead of calling die
>   refs.c: make lock_packed_refs take an err argument
>   refs.c: add an err argument to commit_packed_refs
>   refs.c: add an err argument to pack_refs
>
>  builtin/checkout.c   |   7 ++-
>  builtin/clone.c      |  23 +++++---
>  builtin/merge.c      |  20 ++++---
>  builtin/notes.c      |  24 +++++----
>  builtin/pack-refs.c  |   8 ++-
>  builtin/reset.c      |  12 +++--
>  builtin/update-ref.c |   7 ++-
>  notes-cache.c        |   2 +-
>  notes-utils.c        |   5 +-
>  refs.c               | 148 +++++++++++++++++++++++++++++----------------------
>  refs.h               |  13 ++---
>  transport-helper.c   |   7 ++-
>  transport.c          |   9 ++--
>  13 files changed, 170 insertions(+), 115 deletions(-)
>
> --
> 2.0.1.523.g70700c9
>

[PATCH 0/5] ref-transactions-req-strbuf-err

[Ronnie Sahlberg]

List,

This is the next patch series in the ref transaction work.
This patch series is called ref-transactions-req-strbuf-err and builds ontop
of the series called ref-transactions-req-packed-refs which is origin/pu


This patch series mainly adds some nice strbuf arguments to some functions to
pass errors back to callers.
The only thing noteworthy is that we finally get to remove
-enum action_on_err {
-       UPDATE_REFS_MSG_ON_ERR,
-       UPDATE_REFS_DIE_ON_ERR,
-       UPDATE_REFS_QUIET_ON_ERR
-};

aside from that there is little/nothing much interesting in there.


Ronnie Sahlberg (5):
  refs.c: replace the onerr argument in update_ref with a strbuf err
  refs.c: make add_packed_ref return an error instead of calling die
  refs.c: make lock_packed_refs take an err argument
  refs.c: add an err argument to commit_packed_refs
  refs.c: add an err argument to pack_refs

 builtin/checkout.c   |   7 ++-
 builtin/clone.c      |  23 +++++---
 builtin/merge.c      |  20 ++++---
 builtin/notes.c      |  24 +++++----
 builtin/pack-refs.c  |   8 ++-
 builtin/reset.c      |  12 +++--
 builtin/update-ref.c |   7 ++-
 notes-cache.c        |   2 +-
 notes-utils.c        |   5 +-
 refs.c               | 148 +++++++++++++++++++++++++++++----------------------
 refs.h               |  13 ++---
 transport-helper.c   |   7 ++-
 transport.c          |   9 ++--
 13 files changed, 170 insertions(+), 115 deletions(-)

-- 
2.0.1.556.ge8f7cba.dirty