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

[PATCH v14 06/40] refs.c: add an err argument to repack_without_refs

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Jun 6, 2014, 22:28 UTC
Message-ID
<1402093758-3162-7-git-send-email-sahlberg@google.com>
In-Reply-To
<1402093758-3162-1-git-send-email-sahlberg@google.com>

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 a problem in repack_without_refs.

Add a new function unable_to_lock_message that takes a strbuf argument and fills in the reason for the failure.

In commit_packed_refs, make sure that we propagate any errno that commit_lock_file might have set back to our caller.

Make sure both commit_packed_refs and lock_file has errno set to a meaningful value on error. Update a whole bunch of other places to keep errno from beeing clobbered.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 cache.h    |   2 ++
 lockfile.c |  37 +++++++++++++--------
 refs.c     | 111 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 110 insertions(+), 40 deletions(-)
diff --git a/cache.h b/cache.h
index cbe1935..8b12aa8 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 8fbcb6a..f5da18c 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,42 +150,49 @@ 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;
 	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)
diff --git a/refs.c b/refs.c
index 25c3a93..7ec1f32 100644
--- a/refs.c
+++ b/refs.c
@@ -1333,8 +1333,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];
@@ -1342,8 +1344,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);
 
@@ -1405,9 +1409,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);
+		if (len < 0) {
+			int save_errno = errno;
+			close(fd);
+			errno = save_errno;
+ 			return NULL;
+		}
 		close(fd);
-		if (len < 0)
-			return NULL;
 		while (len && isspace(buffer[len-1]))
 			len--;
 		buffer[len] = '\0';
@@ -1424,6 +1432,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;
@@ -1436,6 +1445,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);
@@ -1908,14 +1918,17 @@ 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;
@@ -1928,13 +1941,15 @@ 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;
 
+	errno = save_errno;
 	strbuf_release(&path);
 
 	return result;
@@ -2203,11 +2218,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");
@@ -2217,10 +2237,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;
 }
 
@@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-static int repack_without_refs(const char **refnames, int n)
+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, removed = 0;
+	int i, ret, removed = 0;
 
 	/* Look for a packed ref */
 	for (i = 0; i < n; i++)
@@ -2444,6 +2467,11 @@ static 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]);
 	}
@@ -2470,12 +2498,16 @@ static 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)
@@ -2723,9 +2755,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;
 	}
 
@@ -2736,15 +2771,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);
@@ -2784,8 +2826,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;
 }
 
@@ -2800,8 +2853,10 @@ int write_ref_sha1(struct ref_lock *lock,
 	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;
@@ -2811,19 +2866,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);
-		return -1;
+		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);
@@ -3481,7 +3540,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);

-- 
2.0.0.582.ge25c160
Previous: Ronnie SahlbergNext: Jonathan Nieder
Message 7 of 51 in “Use ref transactions”
  1. 00/40 Use ref transactionsRonnie Sahlberg, Jun 6, 2014
  2. 01/40 refs.c: remove ref_transaction_rollbackRonnie Sahlberg, Jun 6, 2014
  3. 02/40 refs.c: ref_transaction_commit should not free the transactionRonnie Sahlberg, Jun 6, 2014
  4. 03/40 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Jun 6, 2014
  5. 04/40 refs.c: allow passing NULL to ref_transaction_freeRonnie Sahlberg, Jun 6, 2014
  6. 05/40 refs.c: add a strbuf argument to ref_transaction_commit for error loggingRonnie Sahlberg, Jun 6, 2014
  7. 06/40 refs.c: add an err argument to repack_without_refsRonnie Sahlberg, Jun 6, 2014
  8. Jonathan NiederJun 10, 2014
  9. Ronnie SahlbergJun 10, 2014
  10. 07/40 refs.c: make ref_update_reject_duplicates take a strbuf argument for errorsRonnie Sahlberg, Jun 6, 2014
  11. 08/40 refs.c: add an err argument to delete_ref_looseRonnie Sahlberg, Jun 6, 2014
  12. Jonathan NiederJun 10, 2014
  13. Ronnie SahlbergJun 11, 2014
  14. 09/40 refs.c: make update_ref_write update a strbuf on failureRonnie Sahlberg, Jun 6, 2014
  15. 10/40 update-ref: use err argument to get error from ref_transaction_commitRonnie Sahlberg, Jun 6, 2014
  16. 11/40 refs.c: remove the onerr argument to ref_transaction_commitRonnie Sahlberg, Jun 6, 2014
  17. 12/40 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Jun 6, 2014
  18. 13/40 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Jun 6, 2014
  19. 14/40 refs.c: update ref_transaction_delete to check for error and return statusRonnie Sahlberg, Jun 6, 2014
  20. 15/40 refs.c: make ref_transaction_begin take an err argumentRonnie Sahlberg, Jun 6, 2014
  21. 16/40 refs.c: add transaction.status and track OPEN/CLOSED/ERRORRonnie Sahlberg, Jun 6, 2014
  22. Jonathan NiederJun 10, 2014
  23. 17/40 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Jun 6, 2014
  24. 18/40 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Jun 6, 2014
  25. 19/40 commit.c: use ref transactions for updatesRonnie Sahlberg, Jun 6, 2014
  26. 20/40 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Jun 6, 2014
  27. 21/40 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Jun 6, 2014
  28. 22/40 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Jun 6, 2014
  29. 23/40 refs.c: change update_ref to use a transactionRonnie Sahlberg, Jun 6, 2014
  30. 24/40 receive-pack.c: use a reference transaction for updating the refsRonnie Sahlberg, Jun 6, 2014
  31. Jonathan NiederJun 10, 2014
  32. Ronnie SahlbergJun 11, 2014
  33. 25/40 fast-import.c: use a ref transaction when dumping tagsRonnie Sahlberg, Jun 6, 2014
  34. Jonathan NiederJun 10, 2014
  35. 26/40 walker.c: use ref transaction for ref updatesRonnie Sahlberg, Jun 6, 2014
  36. Jonathan NiederJun 10, 2014
  37. 27/40 refs.c: make lock_ref_sha1 staticRonnie Sahlberg, Jun 6, 2014
  38. 28/40 refs.c: remove the update_ref_lock functionRonnie Sahlberg, Jun 6, 2014
  39. 29/40 refs.c: remove the update_ref_write functionRonnie Sahlberg, Jun 6, 2014
  40. 30/40 refs.c: remove lock_ref_sha1Ronnie Sahlberg, Jun 6, 2014
  41. 31/40 refs.c: make prune_ref use a transaction to delete the refRonnie Sahlberg, Jun 6, 2014
  42. Jonathan NiederJun 10, 2014
  43. 32/40 refs.c: make delete_ref use a transactionRonnie Sahlberg, Jun 6, 2014
  44. 33/40 refs.c: pass the ref log message to _create/delete/update instead of _commitRonnie Sahlberg, Jun 6, 2014
  45. 34/40 refs.c: pass NULL as *flags to read_ref_fullRonnie Sahlberg, Jun 6, 2014
  46. 35/40 refs.c: move the check for valid refname to lock_ref_sha1_basicRonnie Sahlberg, Jun 6, 2014
  47. 36/40 refs.c: call lock_ref_sha1_basic directly from commitRonnie Sahlberg, Jun 6, 2014
  48. 37/40 refs.c: pass a skip list to name_conflict_fnRonnie Sahlberg, Jun 6, 2014
  49. 38/40 refs.c: propagate any errno==ENOTDIR from _commit back to the callersRonnie Sahlberg, Jun 6, 2014
  50. 39/40 fetch.c: change s_update_ref to use a ref transactionRonnie Sahlberg, Jun 6, 2014
  51. 40/40 refs.c: make write_ref_sha1 staticRonnie Sahlberg, Jun 6, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.