[PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf

[Jonathan Nieder <jrnieder@gmail.com>]

This way, the code does not need to carefully safeguard errno to allow
callers to print a reasonable error message when they choose to do
some cleanup before die()-ing.

Fixes a bug waiting to happen where copy_fd would clobber the errno
passed back via hold_lock_file_for_append from read() or write() when
flags did not contain LOCK_DIE_ON_ERROR.  Luckily the only caller uses
flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.

Reported-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The advertised bugfix.

 lockfile.c  | 29 ++++++++++-------------------
 lockfile.h  |  3 ++-
 sha1_file.c |  7 ++++++-
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index b3020f3..8685c68 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 	return fd;
 }
 
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+			      int flags, struct strbuf *err)
 {
 	int fd, orig_fd;
-	struct strbuf err = STRBUF_INIT;
+
+	assert(!(flags & LOCK_DIE_ON_ERROR));
+	assert(err && !err->len);
 
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
-		if (flags & LOCK_DIE_ON_ERROR)
-			unable_to_lock_die(path, errno);
+		unable_to_lock_message(path, errno, err);
 		return fd;
 	}
 
 	orig_fd = open(path, O_RDONLY);
 	if (orig_fd < 0) {
 		if (errno != ENOENT) {
-			int save_errno = errno;
-
-			if (flags & LOCK_DIE_ON_ERROR)
-				die("cannot open '%s' for copying", path);
+			strbuf_addf(err, "cannot open '%s' for copying: %s",
+				    path, strerror(errno));
 			rollback_lock_file(lk);
-			error("cannot open '%s' for copying", path);
-			errno = save_errno;
 			return -1;
 		}
-	} else if (copy_fd(orig_fd, fd, &err)) {
-		int save_errno = errno;
-
-		error("copy-fd: %s", err.buf);
-		strbuf_release(&err);
-		if (flags & LOCK_DIE_ON_ERROR)
-			exit(128);
+	} else if (copy_fd(orig_fd, fd, err)) {
+		strbuf_prefixf(err, "cannot copy '%s': ", path);
 		close(orig_fd);
 		rollback_lock_file(lk);
-		errno = save_errno;
 		return -1;
 	} else {
 		close(orig_fd);
 	}
-	strbuf_release(&err);
 	return fd;
 }
 
diff --git a/lockfile.h b/lockfile.h
index cd2ec95..ca36a1d 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,
 				   struct strbuf *buf);
 extern NORETURN void unable_to_lock_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);
+extern int hold_lock_file_for_append(struct lock_file *, const char *path,
+				     int, struct strbuf *err);
 extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
 extern char *get_locked_file_path(struct lock_file *);
 extern int commit_lock_file_to(struct lock_file *, const char *path);
diff --git a/sha1_file.c b/sha1_file.c
index e1945e2..6c0ab3b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -406,17 +406,22 @@ void add_to_alternates_file(const char *reference)
 	struct lock_file *lock;
 	int fd;
 	char *alt;
+	struct strbuf err = STRBUF_INIT;
 
 	lock = xcalloc(1, sizeof(*lock));
 	fd = hold_lock_file_for_append(lock, mkpath("%s/info/alternates",
 						    get_object_directory()),
-				       LOCK_DIE_ON_ERROR);
+				       0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	alt = mkpath("%s\n", reference);
 	write_or_die(fd, alt, strlen(alt));
 	if (commit_lock_file(lock))
 		die("could not close alternates file");
 	if (alt_odb_tail)
 		link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
+
+	strbuf_release(&err);
 }
 
 int foreach_alt_odb(alt_odb_fn fn, void *cb)