From 026bd1d3e27a9e9b8f6b0098644c5753a2a1e355 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:42 -0700 Subject: [PATCH 01/19] refs.c: remove ref_transaction_rollback We do not yet need both a rollback and a free function for transactions. Remove ref_transaction_rollback and use ref_transaction_free instead. At a later stage we may reintroduce a rollback function if we want to start adding reusable transactions and similar. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 7 +------ refs.h | 16 +++++++--------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/refs.c b/refs.c index dc457742ea..6d841a0645 100644 --- a/refs.c +++ b/refs.c @@ -3334,7 +3334,7 @@ struct ref_transaction *ref_transaction_begin(void) return xcalloc(1, sizeof(struct ref_transaction)); } -static void ref_transaction_free(struct ref_transaction *transaction) +void ref_transaction_free(struct ref_transaction *transaction) { int i; @@ -3345,11 +3345,6 @@ static void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -void ref_transaction_rollback(struct ref_transaction *transaction) -{ - ref_transaction_free(transaction); -} - static struct ref_update *add_update(struct ref_transaction *transaction, const char *refname) { diff --git a/refs.h b/refs.h index 4e3050d976..cfd1832f3f 100644 --- a/refs.h +++ b/refs.h @@ -219,17 +219,11 @@ enum action_on_err { /* * Begin a reference transaction. The reference transaction must - * eventually be commited using ref_transaction_commit() or rolled - * back using ref_transaction_rollback(). + * eventually be commited using ref_transaction_commit() or freed by + * calling ref_transaction_free(). */ struct ref_transaction *ref_transaction_begin(void); -/* - * Roll back a ref_transaction and free all associated data. - */ -void ref_transaction_rollback(struct ref_transaction *transaction); - - /* * The following functions add a reference check or update to a * ref_transaction. In all of them, refname is the name of the @@ -238,7 +232,6 @@ void ref_transaction_rollback(struct ref_transaction *transaction); * can be REF_NODEREF; it is passed to update_ref_lock(). */ - /* * Add a reference update to transaction. new_sha1 is the value that * the reference should have after the update, or zeros if it should @@ -280,6 +273,11 @@ void ref_transaction_delete(struct ref_transaction *transaction, int ref_transaction_commit(struct ref_transaction *transaction, const char *msg, enum action_on_err onerr); +/* + * Free an existing transaction and all associated data. + */ +void ref_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, From 33f9fc593247322ddea5515f548a06cf34d12c2f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:43 -0700 Subject: [PATCH 02/19] refs.c: ref_transaction_commit should not free the transaction Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/update-ref.c | 1 + refs.c | 1 - refs.h | 5 ++--- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 405267f6e2..1fd7a89186 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) update_refs_stdin(); ret = ref_transaction_commit(transaction, msg, UPDATE_REFS_DIE_ON_ERR); + ref_transaction_free(transaction); return ret; } diff --git a/refs.c b/refs.c index 6d841a0645..d9cac6db18 100644 --- a/refs.c +++ b/refs.c @@ -3509,7 +3509,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (updates[i]->lock) unlock_ref(updates[i]->lock); free(delnames); - ref_transaction_free(transaction); return ret; } diff --git a/refs.h b/refs.h index cfd1832f3f..7d946f6b8b 100644 --- a/refs.h +++ b/refs.h @@ -219,8 +219,7 @@ enum action_on_err { /* * Begin a reference transaction. The reference transaction must - * eventually be commited using ref_transaction_commit() or freed by - * calling ref_transaction_free(). + * be freed by calling ref_transaction_free(). */ struct ref_transaction *ref_transaction_begin(void); @@ -268,7 +267,7 @@ void ref_transaction_delete(struct ref_transaction *transaction, /* * Commit all of the changes that have been queued in transaction, as * atomically as possible. Return a nonzero value if there is a - * problem. The ref_transaction is freed by this function. + * problem. */ int ref_transaction_commit(struct ref_transaction *transaction, const char *msg, enum action_on_err onerr); From f1c9350ad784eca97184b73973fad353baa99a1d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:44 -0700 Subject: [PATCH 03/19] refs.c: constify the sha arguments for ref_transaction_create|delete|update ref_transaction_create|delete|update has no need to modify the sha1 arguments passed to it so it should use const unsigned char* instead of unsigned char*. Some functions, such as fast_forward_to(), already have its old/new sha1 arguments as consts. This function will at some point need to use ref_transaction_update() in which case this change is required. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 7 ++++--- refs.h | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index d9cac6db18..21ed46534a 100644 --- a/refs.c +++ b/refs.c @@ -3359,7 +3359,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction, void ref_transaction_update(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, unsigned char *old_sha1, + const unsigned char *new_sha1, + const unsigned char *old_sha1, int flags, int have_old) { struct ref_update *update = add_update(transaction, refname); @@ -3373,7 +3374,7 @@ void ref_transaction_update(struct ref_transaction *transaction, void ref_transaction_create(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, + const unsigned char *new_sha1, int flags) { struct ref_update *update = add_update(transaction, refname); @@ -3387,7 +3388,7 @@ void ref_transaction_create(struct ref_transaction *transaction, void ref_transaction_delete(struct ref_transaction *transaction, const char *refname, - unsigned char *old_sha1, + const unsigned char *old_sha1, int flags, int have_old) { struct ref_update *update = add_update(transaction, refname); diff --git a/refs.h b/refs.h index 7d946f6b8b..8c7f9c44dc 100644 --- a/refs.h +++ b/refs.h @@ -240,7 +240,8 @@ struct ref_transaction *ref_transaction_begin(void); */ void ref_transaction_update(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, unsigned char *old_sha1, + const unsigned char *new_sha1, + const unsigned char *old_sha1, int flags, int have_old); /* @@ -251,7 +252,7 @@ void ref_transaction_update(struct ref_transaction *transaction, */ void ref_transaction_create(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, + const unsigned char *new_sha1, int flags); /* @@ -261,7 +262,7 @@ void ref_transaction_create(struct ref_transaction *transaction, */ void ref_transaction_delete(struct ref_transaction *transaction, const char *refname, - unsigned char *old_sha1, + const unsigned char *old_sha1, int flags, int have_old); /* From 1b07255c95cdf2f7dbe7989a734248f60799f506 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:45 -0700 Subject: [PATCH 04/19] refs.c: allow passing NULL to ref_transaction_free Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free easier to use and more similar to plain 'free'. In particular, it lets us rollback unconditionally as part of cleanup code after setting 'transaction = NULL' if a transaction has been committed or rolled back already. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/refs.c b/refs.c index 21ed46534a..1d6dece426 100644 --- a/refs.c +++ b/refs.c @@ -3338,6 +3338,9 @@ void ref_transaction_free(struct ref_transaction *transaction) { int i; + if (!transaction) + return; + for (i = 0; i < transaction->nr; i++) free(transaction->updates[i]); From 995f8746bc421a537e12622770a90be2205cd26f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:46 -0700 Subject: [PATCH 05/19] refs.c: add a strbuf argument to ref_transaction_commit for error logging Add a strbuf argument to _commit so that we can pass an error string back to the caller. So that we can do error logging from the caller instead of from _commit. Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR and craft any log messages from the callers themselves and finally remove the onerr argument completely. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/update-ref.c | 2 +- refs.c | 6 +++++- refs.h | 5 ++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1fd7a89186..22617afb0a 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (end_null) line_termination = '\0'; update_refs_stdin(); - ret = ref_transaction_commit(transaction, msg, + ret = ref_transaction_commit(transaction, msg, NULL, UPDATE_REFS_DIE_ON_ERR); ref_transaction_free(transaction); return ret; diff --git a/refs.c b/refs.c index 1d6dece426..db05602590 100644 --- a/refs.c +++ b/refs.c @@ -3444,7 +3444,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, } int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, enum action_on_err onerr) + const char *msg, struct strbuf *err, + enum action_on_err onerr) { int ret = 0, delnum = 0, i; const char **delnames; @@ -3473,6 +3474,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, update->flags, &update->type, onerr); if (!update->lock) { + if (err) + strbuf_addf(err, "Cannot lock the ref '%s'.", + update->refname); ret = 1; goto cleanup; } diff --git a/refs.h b/refs.h index 8c7f9c44dc..09d3564116 100644 --- a/refs.h +++ b/refs.h @@ -269,9 +269,12 @@ void ref_transaction_delete(struct ref_transaction *transaction, * Commit all of the changes that have been queued in transaction, as * atomically as possible. Return a nonzero value if there is a * problem. + * If err is non-NULL we will add an error string to it to explain why + * the transaction failed. The string does not end in newline. */ int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, enum action_on_err onerr); + const char *msg, struct strbuf *err, + enum action_on_err onerr); /* * Free an existing transaction and all associated data. From 6af926e8bcab15b42faeee9f2fe1c69b5fea1b15 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:47 -0700 Subject: [PATCH 06/19] lockfile.c: add a new public function unable_to_lock_message Introducing a new unable_to_lock_message helper, which has nicer semantics than unable_to_lock_error and cleans up lockfile.c a little. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- cache.h | 2 ++ lockfile.c | 22 ++++++++++++---------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/cache.h b/cache.h index cbe1935ba6..8b12aa8766 100644 --- a/cache.h +++ b/cache.h @@ -559,6 +559,8 @@ struct lock_file { #define LOCK_DIE_ON_ERROR 1 #define LOCK_NODEREF 2 extern int unable_to_lock_error(const char *path, int err); +extern void unable_to_lock_message(const char *path, int err, + struct strbuf *buf); extern NORETURN void unable_to_lock_index_die(const char *path, int err); extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int hold_lock_file_for_append(struct lock_file *, const char *path, int); diff --git a/lockfile.c b/lockfile.c index 8fbcb6a98a..464031bf1c 100644 --- a/lockfile.c +++ b/lockfile.c @@ -157,33 +157,35 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) return lk->fd; } -static char *unable_to_lock_message(const char *path, int err) +void unable_to_lock_message(const char *path, int err, struct strbuf *buf) { - struct strbuf buf = STRBUF_INIT; - if (err == EEXIST) { - strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n" + strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n" "If no other git process is currently running, this probably means a\n" "git process crashed in this repository earlier. Make sure no other git\n" "process is running and remove the file manually to continue.", absolute_path(path), strerror(err)); } else - strbuf_addf(&buf, "Unable to create '%s.lock': %s", + strbuf_addf(buf, "Unable to create '%s.lock': %s", absolute_path(path), strerror(err)); - return strbuf_detach(&buf, NULL); } int unable_to_lock_error(const char *path, int err) { - char *msg = unable_to_lock_message(path, err); - error("%s", msg); - free(msg); + struct strbuf buf = STRBUF_INIT; + + unable_to_lock_message(path, err, &buf); + error("%s", buf.buf); + strbuf_release(&buf); return -1; } NORETURN void unable_to_lock_index_die(const char *path, int err) { - die("%s", unable_to_lock_message(path, err)); + struct strbuf buf = STRBUF_INIT; + + unable_to_lock_message(path, err, &buf); + die("%s", buf.buf); } int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags) From 447ff1bf0acf9a1d7d2dc3aed032c209f105fb8a Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:48 -0700 Subject: [PATCH 07/19] lockfile.c: make lock_file return a meaningful errno on failurei Making errno when returning from lock_file() meaningful, which should fix * an existing almost-bug in lock_ref_sha1_basic where it assumes errno==ENOENT is meaningful and could waste some work on retries * an existing bug in repack_without_refs where it prints strerror(errno) and picks advice based on errno, despite errno potentially being zero and potentially having been clobbered by that point Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- lockfile.c | 17 ++++++++++++----- refs.c | 1 + refs.h | 1 + 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lockfile.c b/lockfile.c index 464031bf1c..a921d77afc 100644 --- a/lockfile.c +++ b/lockfile.c @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s) return p; } - +/* Make sure errno contains a meaningful value on error */ static int lock_file(struct lock_file *lk, const char *path, int flags) { /* @@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) */ static const size_t max_path_len = sizeof(lk->filename) - 5; - if (strlen(path) >= max_path_len) + if (strlen(path) >= max_path_len) { + errno = ENAMETOOLONG; return -1; + } strcpy(lk->filename, path); if (!(flags & LOCK_NODEREF)) resolve_symlink(lk->filename, max_path_len); @@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) lock_file_list = lk; lk->on_list = 1; } - if (adjust_shared_perm(lk->filename)) - return error("cannot fix permission bits on %s", - lk->filename); + if (adjust_shared_perm(lk->filename)) { + int save_errno = errno; + error("cannot fix permission bits on %s", + lk->filename); + errno = save_errno; + return -1; + } } else lk->filename[0] = 0; @@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path, int err) die("%s", buf.buf); } +/* This should return a meaningful errno on failure */ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags) { int fd = lock_file(lk, path, flags); diff --git a/refs.c b/refs.c index db05602590..e9d53e4a57 100644 --- a/refs.c +++ b/refs.c @@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data) return 0; } +/* This should return a meaningful errno on failure */ int lock_packed_refs(int flags) { struct packed_ref_cache *packed_ref_cache; diff --git a/refs.h b/refs.h index 09d3564116..64f25d99a3 100644 --- a/refs.h +++ b/refs.h @@ -82,6 +82,7 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st /* * Lock the packed-refs file for writing. Flags is passed to * hold_lock_file_for_update(). Return 0 on success. + * Errno is set to something meaningful on error. */ extern int lock_packed_refs(int flags); From 60bca085c893f1866829b55bfee63888943cfe14 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:49 -0700 Subject: [PATCH 08/19] refs.c: add an err argument to repack_without_refs Update repack_without_refs to take an err argument and update it if there is a failure. Pass the err variable from ref_transaction_commit to this function so that callers can print a meaningful error message if _commit fails due to this function. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/remote.c | 5 +++-- refs.c | 19 ++++++++++++++----- refs.h | 3 ++- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index c9102e8fe9..401feb3970 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -755,7 +755,7 @@ static int remove_branches(struct string_list *branches) branch_names = xmalloc(branches->nr * sizeof(*branch_names)); for (i = 0; i < branches->nr; i++) branch_names[i] = branches->items[i].string; - result |= repack_without_refs(branch_names, branches->nr); + result |= repack_without_refs(branch_names, branches->nr, NULL); free(branch_names); for (i = 0; i < branches->nr; i++) { @@ -1333,7 +1333,8 @@ static int prune_remote(const char *remote, int dry_run) for (i = 0; i < states.stale.nr; i++) delete_refs[i] = states.stale.items[i].util; if (!dry_run) - result |= repack_without_refs(delete_refs, states.stale.nr); + result |= repack_without_refs(delete_refs, + states.stale.nr, NULL); free(delete_refs); } diff --git a/refs.c b/refs.c index e9d53e4a57..67a0217a8d 100644 --- a/refs.c +++ b/refs.c @@ -2456,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data) return 0; } -int repack_without_refs(const char **refnames, int n) +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, removed = 0; + int i, ret, removed = 0; /* Look for a packed ref */ for (i = 0; i < n; i++) @@ -2473,6 +2473,11 @@ int repack_without_refs(const char **refnames, int n) return 0; /* no refname exists in packed refs */ if (lock_packed_refs(0)) { + if (err) { + unable_to_lock_message(git_path("packed-refs"), errno, + err); + return -1; + } unable_to_lock_error(git_path("packed-refs"), errno); return error("cannot delete '%s' from packed refs", refnames[i]); } @@ -2499,12 +2504,16 @@ int repack_without_refs(const char **refnames, int n) } /* Write what remains */ - return commit_packed_refs(); + ret = commit_packed_refs(); + if (ret && err) + strbuf_addf(err, "unable to overwrite old ref-pack file: %s", + strerror(errno)); + return ret; } static int repack_without_ref(const char *refname) { - return repack_without_refs(&refname, 1); + return repack_without_refs(&refname, 1, NULL); } static int delete_ref_loose(struct ref_lock *lock, int flag) @@ -3508,7 +3517,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, } } - ret |= repack_without_refs(delnames, delnum); + ret |= repack_without_refs(delnames, delnum, err); for (i = 0; i < delnum; i++) unlink_or_warn(git_path("logs/%s", delnames[i])); clear_loose_ref_cache(&ref_cache); diff --git a/refs.h b/refs.h index 64f25d99a3..65f76376fe 100644 --- a/refs.h +++ b/refs.h @@ -122,7 +122,8 @@ extern void rollback_packed_refs(void); */ int pack_refs(unsigned int flags); -extern int repack_without_refs(const char **refnames, int n); +extern int repack_without_refs(const char **refnames, int n, + struct strbuf *err); extern int ref_exists(const char *); From bd3b02daec241651231f63b3fd8ee2a8ea1dac68 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:50 -0700 Subject: [PATCH 09/19] refs.c: make sure log_ref_setup returns a meaningful errno Making errno when returning from log_ref_setup() meaningful, Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 27 +++++++++++++++++++-------- refs.h | 4 +++- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index 67a0217a8d..9ea519c3fd 100644 --- a/refs.c +++ b/refs.c @@ -2751,6 +2751,7 @@ static int copy_msg(char *buf, const char *msg) return cp - buf; } +/* This function must set a meaningful errno on failure */ int log_ref_setup(const char *refname, char *logfile, int bufsize) { int logfd, oflags = O_APPEND | O_WRONLY; @@ -2761,9 +2762,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) starts_with(refname, "refs/remotes/") || starts_with(refname, "refs/notes/") || !strcmp(refname, "HEAD"))) { - if (safe_create_leading_directories(logfile) < 0) - return error("unable to create directory for %s", - logfile); + if (safe_create_leading_directories(logfile) < 0) { + int save_errno = errno; + error("unable to create directory for %s", logfile); + errno = save_errno; + return -1; + } oflags |= O_CREAT; } @@ -2774,15 +2778,22 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) if ((oflags & O_CREAT) && errno == EISDIR) { if (remove_empty_directories(logfile)) { - return error("There are still logs under '%s'", - logfile); + int save_errno = errno; + error("There are still logs under '%s'", + logfile); + errno = save_errno; + return -1; } logfd = open(logfile, oflags, 0666); } - if (logfd < 0) - return error("Unable to append to %s: %s", - logfile, strerror(errno)); + if (logfd < 0) { + int save_errno = errno; + error("Unable to append to %s: %s", logfile, + strerror(errno)); + errno = save_errno; + return -1; + } } adjust_shared_perm(logfile); diff --git a/refs.h b/refs.h index 65f76376fe..82cc5cb2a1 100644 --- a/refs.h +++ b/refs.h @@ -158,7 +158,9 @@ extern void unlock_ref(struct ref_lock *lock); /** Writes sha1 into the ref specified by the lock. **/ extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg); -/** Setup reflog before using. **/ +/* + * Setup reflog before using. Set errno to something meaningful on failure. + */ int log_ref_setup(const char *refname, char *logfile, int bufsize); /** Reads log for the value of ref during at_time. **/ From 835e3c992fd453d172466be29283b55a792cac76 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:51 -0700 Subject: [PATCH 10/19] refs.c: verify_lock should set errno to something meaningful Making errno when returning from verify_lock() meaningful, which should almost but not completely fix * a bug in "git fetch"'s s_update_ref, which trusts the result of an errno == ENOTDIR check to detect D/F conflicts ENOTDIR makes sense as a sign that a file was in the way of a directory we wanted to create. Should "git fetch" also look for ENOTEMPTY or EEXIST to catch cases where a directory was in the way of a file to be created? Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 4 ++++ refs.h | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 9ea519c3fd..a48f805128 100644 --- a/refs.c +++ b/refs.c @@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char *full_name) return 0; } +/* This function should make sure errno is meaningful on error */ static struct ref_lock *verify_lock(struct ref_lock *lock, const unsigned char *old_sha1, int mustexist) { if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) { + int save_errno = errno; error("Can't verify ref %s", lock->ref_name); unlock_ref(lock); + errno = save_errno; return NULL; } if (hashcmp(lock->old_sha1, old_sha1)) { error("Ref %s is at %s but expected %s", lock->ref_name, sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1)); unlock_ref(lock); + errno = EBUSY; return NULL; } return lock; diff --git a/refs.h b/refs.h index 82cc5cb2a1..8d6cac717b 100644 --- a/refs.h +++ b/refs.h @@ -137,11 +137,15 @@ extern int ref_exists(const char *); */ extern int peel_ref(const char *refname, unsigned char *sha1); -/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/ +/* + * Locks a "refs/" ref returning the lock on success and NULL on failure. + * On failure errno is set to something meaningful. + */ extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1); /** Locks any ref (for 'HEAD' type refs). */ #define REF_NODEREF 0x01 +/* errno is set to something meaningful on failure */ extern struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p); From 470a91ef75d61d102c9cb655a7b8ea1555337d2d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:52 -0700 Subject: [PATCH 11/19] refs.c: make remove_empty_directories always set errno to something sane Making errno when returning from remove_empty_directories() more obviously meaningful, which should provide some peace of mind for people auditing lock_ref_sha1_basic. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index a48f805128..cc6958141c 100644 --- a/refs.c +++ b/refs.c @@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file) * only empty directories), remove them. */ struct strbuf path; - int result; + int result, save_errno; strbuf_init(&path, 20); strbuf_addstr(&path, file); result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); + save_errno = errno; strbuf_release(&path); + errno = save_errno; return result; } @@ -2056,6 +2058,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) return logs_found; } +/* This function should make sure errno is meaningful on error */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) From d3f665550588d19d6d6c9f9064aa3d4685afdf4d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:53 -0700 Subject: [PATCH 12/19] refs.c: commit_packed_refs to return a meaningful errno on failure Making errno when returning from commit_packed_refs() meaningful, which should fix * a bug in "git clone" where it prints strerror(errno) based on errno, despite errno possibly being zero and potentially having been clobbered by that point * the same kind of bug in "git pack-refs" and prepares for repack_without_refs() to get a meaningful error message when commit_packed_refs() fails without falling into the same bug. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 10 +++++++++- refs.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index cc6958141c..7a815be38e 100644 --- a/refs.c +++ b/refs.c @@ -2239,11 +2239,16 @@ int lock_packed_refs(int flags) return 0; } +/* + * Commit the packed refs changes. + * On error we must make sure that errno contains a meaningful value. + */ int commit_packed_refs(void) { struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(&ref_cache); int error = 0; + int save_errno = 0; if (!packed_ref_cache->lock) die("internal error: packed-refs not locked"); @@ -2253,10 +2258,13 @@ int commit_packed_refs(void) do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache), 0, write_packed_entry_fn, &packed_ref_cache->lock->fd); - if (commit_lock_file(packed_ref_cache->lock)) + if (commit_lock_file(packed_ref_cache->lock)) { + save_errno = errno; error = -1; + } packed_ref_cache->lock = NULL; release_packed_ref_cache(packed_ref_cache); + errno = save_errno; return error; } diff --git a/refs.h b/refs.h index 8d6cac717b..e588ff842f 100644 --- a/refs.h +++ b/refs.h @@ -98,6 +98,7 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1); * 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. + * Sets errno to something meaningful on error. */ extern int commit_packed_refs(void); From 76d70dc0c63fe89958308f249136864aec5cdf03 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:54 -0700 Subject: [PATCH 13/19] refs.c: make resolve_ref_unsafe set errno to something meaningful on error Making errno when returning from resolve_ref_unsafe() meaningful, which should fix * a bug in lock_ref_sha1_basic, where it assumes EISDIR means it failed due to a directory being in the way Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- cache.h | 2 +- refs.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 8b12aa8766..e7ec6267bd 100644 --- a/cache.h +++ b/cache.h @@ -979,7 +979,7 @@ extern int read_ref(const char *refname, unsigned char *sha1); * NULL. If more than MAXDEPTH recursive symbolic lookups are needed, * give up and return NULL. * - * errno is sometimes set on errors, but not always. + * errno is set to something meaningful on error. */ extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag); extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag); diff --git a/refs.c b/refs.c index 7a815be38e..211429df06 100644 --- a/refs.c +++ b/refs.c @@ -1334,6 +1334,7 @@ static const char *handle_missing_loose_ref(const char *refname, } } +/* This function needs to return a meaningful errno on failure */ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag) { int depth = MAXDEPTH; @@ -1344,8 +1345,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (flag) *flag = 0; - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + errno = EINVAL; return NULL; + } for (;;) { char path[PATH_MAX]; @@ -1353,8 +1356,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea char *buf; int fd; - if (--depth < 0) + if (--depth < 0) { + errno = ELOOP; return NULL; + } git_snpath(path, sizeof(path), "%s", refname); @@ -1416,9 +1421,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea return NULL; } len = read_in_full(fd, buffer, sizeof(buffer)-1); - close(fd); - if (len < 0) + if (len < 0) { + int save_errno = errno; + close(fd); + errno = save_errno; return NULL; + } + close(fd); while (len && isspace(buffer[len-1])) len--; buffer[len] = '\0'; @@ -1435,6 +1444,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea (buffer[40] != '\0' && !isspace(buffer[40]))) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } return refname; @@ -1447,6 +1457,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } refname = strcpy(refname_buffer, buf); From dc615de8610c807633c63313937056bf45df0b8f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:55 -0700 Subject: [PATCH 14/19] refs.c: log_ref_write should try to return meaningful errno MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making errno from write_ref_sha1() meaningful, which should fix * a bug in "git checkout -b" where it prints strerror(errno)  despite errno possibly being zero or clobbered * a bug in "git fetch"'s s_update_ref, which trusts the result of an  errno == ENOTDIR check to detect D/F conflicts Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index 211429df06..61570c9036 100644 --- a/refs.c +++ b/refs.c @@ -2859,8 +2859,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1, len += copy_msg(logrec + len - 1, msg) - 1; written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); - if (close(logfd) != 0 || written != len) - return error("Unable to append to %s", log_file); + if (written != len) { + int save_errno = errno; + close(logfd); + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } + if (close(logfd)) { + int save_errno = errno; + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } return 0; } @@ -2869,14 +2880,17 @@ static int is_branch(const char *refname) return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/"); } +/* This function must return a meaningful errno */ int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg) { static char term = '\n'; struct object *o; - if (!lock) + if (!lock) { + errno = EINVAL; return -1; + } if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) { unlock_ref(lock); return 0; @@ -2886,19 +2900,23 @@ int write_ref_sha1(struct ref_lock *lock, error("Trying to write ref %s with nonexistent object %s", lock->ref_name, sha1_to_hex(sha1)); unlock_ref(lock); + errno = EINVAL; return -1; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { error("Trying to write non-commit object %s to branch %s", sha1_to_hex(sha1), lock->ref_name); unlock_ref(lock); + errno = EINVAL; return -1; } if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 - || close_ref(lock) < 0) { + write_in_full(lock->lock_fd, &term, 1) != 1 || + close_ref(lock) < 0) { + int save_errno = errno; error("Couldn't write %s", lock->lk->filename); unlock_ref(lock); + errno = save_errno; return -1; } clear_loose_ref_cache(&ref_cache); From 038d005129467a514189e8cc1321022f71f6a51e Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:56 -0700 Subject: [PATCH 15/19] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors Make ref_update_reject_duplicates return any error that occurs through a new strbuf argument. This means that when a transaction commit fails in this function we will now be able to pass a helpful error message back to the caller. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 61570c9036..115f1431f7 100644 --- a/refs.c +++ b/refs.c @@ -3488,6 +3488,7 @@ static int ref_update_compare(const void *r1, const void *r2) } static int ref_update_reject_duplicates(struct ref_update **updates, int n, + struct strbuf *err, enum action_on_err onerr) { int i; @@ -3495,6 +3496,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) { const char *str = "Multiple updates for ref '%s' not allowed."; + if (err) + strbuf_addf(err, str, updates[i]->refname); + switch (onerr) { case UPDATE_REFS_MSG_ON_ERR: error(str, updates[i]->refname); break; @@ -3525,7 +3529,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); - ret = ref_update_reject_duplicates(updates, n, onerr); + ret = ref_update_reject_duplicates(updates, n, err, onerr); if (ret) goto cleanup; From c1703d7634a61b4d08d77fa0d9ca7e7ad92c3cb8 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:57 -0700 Subject: [PATCH 16/19] refs.c: make update_ref_write update a strbuf on failure Change update_ref_write to also update an error strbuf on failure. This makes the error available to ref_transaction_commit callers if the transaction failed due to update_ref_sha1/write_ref_sha1 failures. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/refs.c b/refs.c index 115f1431f7..003b313ade 100644 --- a/refs.c +++ b/refs.c @@ -3353,10 +3353,13 @@ static struct ref_lock *update_ref_lock(const char *refname, static int update_ref_write(const char *action, const char *refname, const unsigned char *sha1, struct ref_lock *lock, - enum action_on_err onerr) + struct strbuf *err, enum action_on_err onerr) { if (write_ref_sha1(lock, sha1, action) < 0) { const char *str = "Cannot update the ref '%s'."; + if (err) + strbuf_addf(err, str, refname); + switch (onerr) { case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break; case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break; @@ -3477,7 +3480,7 @@ int update_ref(const char *action, const char *refname, lock = update_ref_lock(refname, oldval, flags, NULL, onerr); if (!lock) return 1; - return update_ref_write(action, refname, sha1, lock, onerr); + return update_ref_write(action, refname, sha1, lock, NULL, onerr); } static int ref_update_compare(const void *r1, const void *r2) @@ -3559,7 +3562,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = update_ref_write(msg, update->refname, update->new_sha1, - update->lock, onerr); + update->lock, err, onerr); update->lock = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; From 8bcd37482e95b47119f3a8f03a1c8ce5815594a3 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:58 -0700 Subject: [PATCH 17/19] update-ref: use err argument to get error from ref_transaction_commit Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is returned to print a log message if/after the transaction fails. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/update-ref.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 22617afb0a..aec9004207 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) const char *refname, *oldval, *msg = NULL; 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")), @@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) die("Refusing to perform update with empty message."); if (read_stdin) { - int ret; transaction = ref_transaction_begin(); - if (delete || no_deref || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; update_refs_stdin(); - ret = ref_transaction_commit(transaction, msg, NULL, - UPDATE_REFS_DIE_ON_ERR); + if (ref_transaction_commit(transaction, msg, &err, + UPDATE_REFS_QUIET_ON_ERR)) + die("%s", err.buf); ref_transaction_free(transaction); - return ret; + return 0; } if (end_null) From 01319837c53050109c60e6740dfa9462327161f0 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:59 -0700 Subject: [PATCH 18/19] refs.c: remove the onerr argument to ref_transaction_commit Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr argument any more. Remove the onerr argument from the ref_transaction_commit signature. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/update-ref.c | 3 +-- refs.c | 22 +++++++--------------- refs.h | 3 +-- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index aec9004207..88ab785ea0 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (end_null) line_termination = '\0'; update_refs_stdin(); - if (ref_transaction_commit(transaction, msg, &err, - UPDATE_REFS_QUIET_ON_ERR)) + if (ref_transaction_commit(transaction, msg, &err)) die("%s", err.buf); ref_transaction_free(transaction); return 0; diff --git a/refs.c b/refs.c index 003b313ade..4f78bd9250 100644 --- a/refs.c +++ b/refs.c @@ -3491,8 +3491,7 @@ static int ref_update_compare(const void *r1, const void *r2) } static int ref_update_reject_duplicates(struct ref_update **updates, int n, - struct strbuf *err, - enum action_on_err onerr) + struct strbuf *err) { int i; for (i = 1; i < n; i++) @@ -3502,22 +3501,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, if (err) strbuf_addf(err, str, updates[i]->refname); - switch (onerr) { - case UPDATE_REFS_MSG_ON_ERR: - error(str, updates[i]->refname); break; - case UPDATE_REFS_DIE_ON_ERR: - die(str, updates[i]->refname); break; - case UPDATE_REFS_QUIET_ON_ERR: - break; - } return 1; } return 0; } int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, struct strbuf *err, - enum action_on_err onerr) + const char *msg, struct strbuf *err) { int ret = 0, delnum = 0, i; const char **delnames; @@ -3532,7 +3522,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); - ret = ref_update_reject_duplicates(updates, n, err, onerr); + ret = ref_update_reject_duplicates(updates, n, err); if (ret) goto cleanup; @@ -3544,7 +3534,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, (update->have_old ? update->old_sha1 : NULL), update->flags, - &update->type, onerr); + &update->type, + UPDATE_REFS_QUIET_ON_ERR); if (!update->lock) { if (err) strbuf_addf(err, "Cannot lock the ref '%s'.", @@ -3562,7 +3553,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = update_ref_write(msg, update->refname, update->new_sha1, - update->lock, err, onerr); + update->lock, err, + UPDATE_REFS_QUIET_ON_ERR); update->lock = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; diff --git a/refs.h b/refs.h index e588ff842f..163b45c50a 100644 --- a/refs.h +++ b/refs.h @@ -282,8 +282,7 @@ void ref_transaction_delete(struct ref_transaction *transaction, * the transaction failed. The string does not end in newline. */ int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, struct strbuf *err, - enum action_on_err onerr); + const char *msg, struct strbuf *err); /* * Free an existing transaction and all associated data. From 8e34800e5bce1dc6f1ab5520a4c866a2a73639de Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:43:00 -0700 Subject: [PATCH 19/19] refs.c: change ref_transaction_update() to do error checking and return status Update ref_transaction_update() do some basic error checking and return non-zero on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future. Add an err argument that will be updated on failure. In future patches we will start doing both locking and checking for name conflicts in _update instead of _commit at which time this function will start returning errors for these conditions. Also check for BUGs during update and die(BUG:...) if we are calling _update with have_old but the old_sha1 pointer is NULL. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- builtin/update-ref.c | 12 +++++++----- refs.c | 18 ++++++++++++------ refs.h | 14 +++++++++----- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 88ab785ea0..3067b11310 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -16,6 +16,7 @@ static struct ref_transaction *transaction; static char line_termination = '\n'; static int update_flags; +static struct strbuf err = STRBUF_INIT; /* * Parse one whitespace- or NUL-terminated, possibly C-quoted argument @@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) if (*next != line_termination) die("update %s: extra input: %s", refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old, &err)) + die("%s", err.buf); update_flags = 0; free(refname); @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) if (*next != line_termination) die("verify %s: extra input: %s", refname, next); - ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, have_old); + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old, &err)) + die("%s", err.buf); update_flags = 0; free(refname); @@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) const char *refname, *oldval, *msg = NULL; 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")), diff --git a/refs.c b/refs.c index 4f78bd9250..3f05e88329 100644 --- a/refs.c +++ b/refs.c @@ -3428,19 +3428,25 @@ static struct ref_update *add_update(struct ref_transaction *transaction, return update; } -void ref_transaction_update(struct ref_transaction *transaction, - const char *refname, - const unsigned char *new_sha1, - const unsigned char *old_sha1, - int flags, int have_old) +int ref_transaction_update(struct ref_transaction *transaction, + const char *refname, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + int flags, int have_old, + struct strbuf *err) { - struct ref_update *update = add_update(transaction, refname); + struct ref_update *update; + if (have_old && !old_sha1) + die("BUG: have_old is true but old_sha1 is NULL"); + + update = add_update(transaction, refname); hashcpy(update->new_sha1, new_sha1); update->flags = flags; update->have_old = have_old; if (have_old) hashcpy(update->old_sha1, old_sha1); + return 0; } void ref_transaction_create(struct ref_transaction *transaction, diff --git a/refs.h b/refs.h index 163b45c50a..c5376cee8d 100644 --- a/refs.h +++ b/refs.h @@ -246,12 +246,16 @@ struct ref_transaction *ref_transaction_begin(void); * be deleted. If have_old is true, then old_sha1 holds the value * that the reference should have had before the update, or zeros if * it must not have existed beforehand. + * Function returns 0 on success and non-zero on failure. A failure to update + * means that the transaction as a whole has failed and will need to be + * rolled back. On failure the err buffer will be updated. */ -void ref_transaction_update(struct ref_transaction *transaction, - const char *refname, - const unsigned char *new_sha1, - const unsigned char *old_sha1, - int flags, int have_old); +int ref_transaction_update(struct ref_transaction *transaction, + const char *refname, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + int flags, int have_old, + struct strbuf *err); /* * Add a reference creation to transaction. new_sha1 is the value