threads / patch / 37988

patchcopy.c: make copy_fd preserve meaningful errno

Subject: [PATCH] copy.c: make copy_fd preserve meaningful errno

## tl;dr

83 messages between Nov 17, 2014 and Feb 17, 2015. Diffs are folded; open one to read it.

replies: 82people: 7as markdown or json

Stefan Beller· Nov 17, 2014, 22:14 UTC · lore
From: Ronnie Sahlberg <sahlberg@google.com>

Update copy_fd to return a meaningful errno on failure and also preserve the existing errno variable.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
As announced in [1], I'm going to take over the 
ref-transactions-reflog series by Ronnie Sahlberg.

This patch was sent previously to the list as part of that series[2], but it seems to be unrelated to me.

Thanks, Stefan

[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
 copy.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
Show changes to copy.c +10 −5
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
 		if (!len)
 			break;
 		if (len < 0) {
-			return error("copy-fd: read returned %s",
-				     strerror(errno));
+			int save_errno = errno;
+			error("copy-fd: read returned %s", strerror(errno));
+			errno = save_errno;
+			return -1;
+		}
+		if (write_in_full(ofd, buffer, len) < 0) {
+			int save_errno = errno;
+			error("copy-fd: write returned %s", strerror(errno));
+			errno = save_errno;
+			return -1;
 		}
-		if (write_in_full(ofd, buffer, len) < 0)
-			return error("copy-fd: write returned %s",
-				     strerror(errno));
 	}
 	return 0;
 }

-- 
2.2.0.rc1.24.g562add4
Junio C Hamano· Nov 17, 2014, 23:08 UTC · re: Stefan Beller · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Stefan Beller <sbeller@google.com> writes:
> This patch was sent previously to the list as part of
> that series[2], but it seems to be unrelated to me.

I am fine to queue obvious and trivial bits first before the larger main course. For now I'll queue this one and also the series that has been queued for a while, but at some point I suspect we would have to drop the latter.

Thanks.
Show 32 quoted lines
> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
> [2] http://www.spinics.net/lists/git/msg240784.html
>  copy.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/copy.c b/copy.c
> index f2970ec..a8d366e 100644
> --- a/copy.c
> +++ b/copy.c
> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>  		if (!len)
>  			break;
>  		if (len < 0) {
> -			return error("copy-fd: read returned %s",
> -				     strerror(errno));
> +			int save_errno = errno;
> +			error("copy-fd: read returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
> +		}
> +		if (write_in_full(ofd, buffer, len) < 0) {
> +			int save_errno = errno;
> +			error("copy-fd: write returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
>  		}
> -		if (write_in_full(ofd, buffer, len) < 0)
> -			return error("copy-fd: write returned %s",
> -				     strerror(errno));
>  	}
>  	return 0;
>  }
Stefan Beller· Nov 17, 2014, 23:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

I am reviewing the series and about to resend it with very minor nits fixed. I just want to point out this fix is orthogonal to the series and can be picked up no matter how long the reviewing/discussion of the series goes.

Thanks, Stefan

On Mon, Nov 17, 2014 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 45 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> This patch was sent previously to the list as part of
>> that series[2], but it seems to be unrelated to me.
>
> I am fine to queue obvious and trivial bits first before the larger
> main course.  For now I'll queue this one and also the series that
> has been queued for a while, but at some point I suspect we would
> have to drop the latter.
>
> Thanks.
>
>
>> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
>> [2] http://www.spinics.net/lists/git/msg240784.html
>>  copy.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/copy.c b/copy.c
>> index f2970ec..a8d366e 100644
>> --- a/copy.c
>> +++ b/copy.c
>> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>>               if (!len)
>>                       break;
>>               if (len < 0) {
>> -                     return error("copy-fd: read returned %s",
>> -                                  strerror(errno));
>> +                     int save_errno = errno;
>> +                     error("copy-fd: read returned %s", strerror(errno));
>> +                     errno = save_errno;
>> +                     return -1;
>> +             }
>> +             if (write_in_full(ofd, buffer, len) < 0) {
>> +                     int save_errno = errno;
>> +                     error("copy-fd: write returned %s", strerror(errno));
>> +                     errno = save_errno;
>> +                     return -1;
>>               }
>> -             if (write_in_full(ofd, buffer, len) < 0)
>> -                     return error("copy-fd: write returned %s",
>> -                                  strerror(errno));
>>       }
>>       return 0;
>>  }
Jonathan Nieder· Nov 17, 2014, 23:35 UTC · re: Stefan Beller · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Hi,
Stefan Beller wrote:
> This patch was sent previously to the list as part of
> that series[2], but it seems to be unrelated to me.
Thanks.  Good call.
[...]
> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Update copy_fd to return a meaningful errno on failure and also
> preserve the existing errno variable.

Some functions in git make errno meaningful on error and others don't. In general, the more we only use errno immediately after a system call, the better, so based on the above description this seems like a step in the wrong direction.

Do any callers care about errno? Does the function's API documentation say it will make errno meaningful on error, so people making changes to copy_fd in the future know to maintain that property?

*searches*
Looks like callers are:
 convert.c::filter_buffer_or_fd, which doesn't care
 copy.c::copy_file, which also doesn't care
 lockfile.c::hold_lock_file_for_append, which started caring
 in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
 before returning, 2014-10-01).  But no callers of that function
 care yet.

So this is about fixing a bug-waiting-to-happen in hold_lock_file_for_append. That would be enough to motivate the change.

[...]
>  copy.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Please also update the API documentation in cache.h so we remember not to backslide in the future.

[...]
Show 12 quoted lines
> --- a/copy.c
> +++ b/copy.c
> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>  		if (!len)
>  			break;
>  		if (len < 0) {
> -			return error("copy-fd: read returned %s",
> -				     strerror(errno));
> +			int save_errno = errno;
> +			error("copy-fd: read returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;

Any caller is presumably going to turn around and print strerror(errno) again, producing repetitive output.

Can we do better?  E.g., if the signature were
	int copy_fd(int ifd, int ofd, struct strbuf *err);

then we could write the error message to the err strbuf for the caller to print. The error handling would be more explicit so there would be no need to protect errno from clobbering by other system calls (both here and in callers).

Something like this:
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Show changes to diff +72 −32
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
 extern void maybe_flush_or_die(FILE *, const char *);
 __attribute__((format (printf, 2, 3)))
 extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	if (params->src) {
 		write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
 	} else {
-		write_err = copy_fd(params->fd, child_process.in);
+		struct strbuf err = STRBUF_INIT;
+		write_err = copy_fd(params->fd, child_process.in, &err);
+		if (write_err)
+			error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
 	}
 
 	if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
 #include "cache.h"
 
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
 {
+	assert(err);
+
 	while (1) {
 		char buffer[8192];
 		ssize_t len = xread(ifd, buffer, sizeof(buffer));
 		if (!len)
 			break;
 		if (len < 0) {
-			return error("copy-fd: read returned %s",
-				     strerror(errno));
+			strbuf_addf(err, "read returned %s", strerror(errno));
+			return -1;
+		}
+		if (write_in_full(ofd, buffer, len) < 0) {
+			strbuf_addf(err, "write returned %s", strerror(errno));
+			return -1;
 		}
-		if (write_in_full(ofd, buffer, len) < 0)
-			return error("copy-fd: write returned %s",
-				     strerror(errno));
 	}
 	return 0;
 }
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
 
 int copy_file(const char *dst, const char *src, int mode)
 {
-	int fdi, fdo, status;
+	int fdi, fdo;
+	struct strbuf err = STRBUF_INIT;
 
 	mode = (mode & 0111) ? 0777 : 0666;
 	if ((fdi = open(src, O_RDONLY)) < 0)
@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
 		close(fdi);
 		return fdo;
 	}
-	status = copy_fd(fdi, fdo);
+	if (copy_fd(fdi, fdo, &err)) {
+		close(fdi);
+		close(fdo);
+		error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+	strbuf_release(&err);
 	close(fdi);
 	if (close(fdo) != 0)
 		return error("%s: close error: %s", dst, strerror(errno));
-
-	if (!status && adjust_shared_perm(dst))
+	if (adjust_shared_perm(dst))
 		return -1;
 
-	return status;
+	return 0;
 }
 
 int copy_file_with_time(const char *dst, const char *src, int mode)
diff --git i/lockfile.c w/lockfile.c
index 4f16ee7..c47976e 100644
--- i/lockfile.c
+++ w/lockfile.c
@@ -179,37 +179,52 @@ 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)
+/*
+ * Like strbuf_addf but inserts at the front of a strbuf instead
+ * of appending.
+ */
+static void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
+{
+	size_t pos, len;
+	va_list ap;
+
+	pos = sb->len;
+
+	va_start(ap, fmt);
+	strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+
+	len = sb->len - pos;
+	strbuf_insert(sb, 0, sb->buf + pos, len);
+	strbuf_remove(sb, pos + len, len);
+}
+
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+			      int flags, struct strbuf *err)
 {
 	int fd, orig_fd;
 
+	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)) {
-		int save_errno = errno;
-
-		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);
diff --git i/lockfile.h w/lockfile.h
index cd2ec95..ca36a1d 100644
--- i/lockfile.h
+++ w/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 i/sha1_file.c w/sha1_file.c
index d7f1838..9ae1839 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -403,14 +403,24 @@ void read_info_alternates(const char * relative_base, int depth)
 
 void add_to_alternates_file(const char *reference)
 {
-	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
-	char *alt = mkpath("%s\n", 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, git_path("objects/info/alternates"),
+				       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)
Stefan Beller· Nov 18, 2014, 00:18 UTC · re: Jonathan Nieder · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 19 quoted lines
> Hi,
>
> Stefan Beller wrote:
>
>> This patch was sent previously to the list as part of
>> that series[2], but it seems to be unrelated to me.
>
> Thanks.  Good call.
>
> [...]
>> From: Ronnie Sahlberg <sahlberg@google.com>
>>
>> Update copy_fd to return a meaningful errno on failure and also
>> preserve the existing errno variable.
>
> Some functions in git make errno meaningful on error and others don't.
> In general, the more we only use errno immediately after a system
> call, the better, so based on the above description this seems like a
> step in the wrong direction.

I did reword the commit message badly. Before it just read "Update copy_fd to return a meaningful errno".

In fact the proposed patch doesn't guarantee the errno, which is set at the beginning of the function to be the same at the end.

What it really should preserve is the errno from xread, while evaluating error("copy-fd: read returned %s", strerror(errno)); So the function call of error(...) or strerror(...) may change the errno.

Show 57 quoted lines
>
> Do any callers care about errno?  Does the function's API
> documentation say it will make errno meaningful on error, so people
> making changes to copy_fd in the future know to maintain that
> property?
>
> *searches*
>
> Looks like callers are:
>
>  convert.c::filter_buffer_or_fd, which doesn't care
>
>  copy.c::copy_file, which also doesn't care
>
>  lockfile.c::hold_lock_file_for_append, which started caring
>  in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
>  before returning, 2014-10-01).  But no callers of that function
>  care yet.
>
> So this is about fixing a bug-waiting-to-happen in
> hold_lock_file_for_append.  That would be enough to motivate the
> change.
>
> [...]
>>  copy.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> Please also update the API documentation in cache.h so we remember not
> to backslide in the future.
>
> [...]
>> --- a/copy.c
>> +++ b/copy.c
>> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>>               if (!len)
>>                       break;
>>               if (len < 0) {
>> -                     return error("copy-fd: read returned %s",
>> -                                  strerror(errno));
>> +                     int save_errno = errno;
>> +                     error("copy-fd: read returned %s", strerror(errno));
>> +                     errno = save_errno;
>> +                     return -1;
>
> Any caller is presumably going to turn around and print strerror(errno)
> again, producing repetitive output.
>
> Can we do better?  E.g., if the signature were
>
>         int copy_fd(int ifd, int ofd, struct strbuf *err);
>
> then we could write the error message to the err strbuf for the
> caller to print.  The error handling would be more explicit so
> there would be no need to protect errno from clobbering by other
> system calls (both here and in callers).
>
> Something like this:

I like this approach, though your patch is not about the original intention as this one (having strerror(...) not messing with the errno), but rather accumulating the errors not in numbers but string buffers?

Show 220 quoted lines
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> diff --git i/cache.h w/cache.h
> index 99ed096..ddaa30f 100644
> --- i/cache.h
> +++ w/cache.h
> @@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
>  extern void maybe_flush_or_die(FILE *, const char *);
>  __attribute__((format (printf, 2, 3)))
>  extern void fprintf_or_die(FILE *, const char *fmt, ...);
> -extern int copy_fd(int ifd, int ofd);
> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
>  extern int copy_file(const char *dst, const char *src, int mode);
>  extern int copy_file_with_time(const char *dst, const char *src, int mode);
>  extern void write_or_die(int fd, const void *buf, size_t count);
> diff --git i/convert.c w/convert.c
> index 9a5612e..e301447 100644
> --- i/convert.c
> +++ w/convert.c
> @@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
>         if (params->src) {
>                 write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
>         } else {
> -               write_err = copy_fd(params->fd, child_process.in);
> +               struct strbuf err = STRBUF_INIT;
> +               write_err = copy_fd(params->fd, child_process.in, &err);
> +               if (write_err)
> +                       error("copy-fd: %s", err.buf);
> +               strbuf_release(&err);
>         }
>
>         if (close(child_process.in))
> diff --git i/copy.c w/copy.c
> index f2970ec..828661a 100644
> --- i/copy.c
> +++ w/copy.c
> @@ -1,19 +1,22 @@
>  #include "cache.h"
>
> -int copy_fd(int ifd, int ofd)
> +int copy_fd(int ifd, int ofd, struct strbuf *err)
>  {
> +       assert(err);
> +
>         while (1) {
>                 char buffer[8192];
>                 ssize_t len = xread(ifd, buffer, sizeof(buffer));
>                 if (!len)
>                         break;
>                 if (len < 0) {
> -                       return error("copy-fd: read returned %s",
> -                                    strerror(errno));
> +                       strbuf_addf(err, "read returned %s", strerror(errno));
> +                       return -1;
> +               }
> +               if (write_in_full(ofd, buffer, len) < 0) {
> +                       strbuf_addf(err, "write returned %s", strerror(errno));
> +                       return -1;
>                 }
> -               if (write_in_full(ofd, buffer, len) < 0)
> -                       return error("copy-fd: write returned %s",
> -                                    strerror(errno));
>         }
>         return 0;
>  }
> @@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
>
>  int copy_file(const char *dst, const char *src, int mode)
>  {
> -       int fdi, fdo, status;
> +       int fdi, fdo;
> +       struct strbuf err = STRBUF_INIT;
>
>         mode = (mode & 0111) ? 0777 : 0666;
>         if ((fdi = open(src, O_RDONLY)) < 0)
> @@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
>                 close(fdi);
>                 return fdo;
>         }
> -       status = copy_fd(fdi, fdo);
> +       if (copy_fd(fdi, fdo, &err)) {
> +               close(fdi);
> +               close(fdo);
> +               error("copy-fd: %s", err.buf);
> +               strbuf_release(&err);
> +               return -1;
> +       }
> +       strbuf_release(&err);
>         close(fdi);
>         if (close(fdo) != 0)
>                 return error("%s: close error: %s", dst, strerror(errno));
> -
> -       if (!status && adjust_shared_perm(dst))
> +       if (adjust_shared_perm(dst))
>                 return -1;
>
> -       return status;
> +       return 0;
>  }
>
>  int copy_file_with_time(const char *dst, const char *src, int mode)
> diff --git i/lockfile.c w/lockfile.c
> index 4f16ee7..c47976e 100644
> --- i/lockfile.c
> +++ w/lockfile.c
> @@ -179,37 +179,52 @@ 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)
> +/*
> + * Like strbuf_addf but inserts at the front of a strbuf instead
> + * of appending.
> + */
> +static void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
> +{
> +       size_t pos, len;
> +       va_list ap;
> +
> +       pos = sb->len;
> +
> +       va_start(ap, fmt);
> +       strbuf_vaddf(sb, fmt, ap);
> +       va_end(ap);
> +
> +       len = sb->len - pos;
> +       strbuf_insert(sb, 0, sb->buf + pos, len);
> +       strbuf_remove(sb, pos + len, len);
> +}
> +
> +int hold_lock_file_for_append(struct lock_file *lk, const char *path,
> +                             int flags, struct strbuf *err)
>  {
>         int fd, orig_fd;
>
> +       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)) {
> -               int save_errno = errno;
> -
> -               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);
> diff --git i/lockfile.h w/lockfile.h
> index cd2ec95..ca36a1d 100644
> --- i/lockfile.h
> +++ w/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 i/sha1_file.c w/sha1_file.c
> index d7f1838..9ae1839 100644
> --- i/sha1_file.c
> +++ w/sha1_file.c
> @@ -403,14 +403,24 @@ void read_info_alternates(const char * relative_base, int depth)
>
>  void add_to_alternates_file(const char *reference)
>  {
> -       struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
> -       int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
> -       char *alt = mkpath("%s\n", 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, git_path("objects/info/alternates"),
> +                                      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)
Jonathan Nieder· Nov 18, 2014, 00:48 UTC · re: Stefan Beller · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

(meta-comment: please snip out the context you are not responding to,
 to make reading easier)
Stefan Beller wrote:
> On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Stefan Beller wrote:
Show 17 quoted lines
>>> Update copy_fd to return a meaningful errno on failure and also
>>> preserve the existing errno variable.
>>
>> Some functions in git make errno meaningful on error and others don't.
>> In general, the more we only use errno immediately after a system
>> call, the better, so based on the above description this seems like a
>> step in the wrong direction.
>
> I did reword the commit message badly. Before it just read
> "Update copy_fd to return a meaningful errno".
>
> In fact the proposed patch doesn't guarantee the errno, which is set
> at the beginning of the function to be the same at the end.
>
> What it really should preserve is the errno from xread, while
> evaluating error("copy-fd: read returned %s", strerror(errno));
> So the function call of error(...) or strerror(...) may change the errno.

A successful call to strerror() is guaranteed not to change errno, but a call to error() does I/O so it can clobber errno.

The basic question is whether errno is and should be part of copy_fd()'s contract. Until v2.2.0-rc0~53^2~2 (2014-10-01), it wasn't. Even after that change, there's no user-visible effect to clobbering errno (so this patch is about maintainability, as opposed to fixing a user-visible bad behavior).

[...]
Show 16 quoted lines
>> Can we do better?  E.g., if the signature were
>>
>>         int copy_fd(int ifd, int ofd, struct strbuf *err);
>>
>> then we could write the error message to the err strbuf for the
>> caller to print.  The error handling would be more explicit so
>> there would be no need to protect errno from clobbering by other
>> system calls (both here and in callers).
>>
>> Something like this:
>
> I like this approach, though your patch is not about the original
> intention as this one
> (having strerror(...) not messing with the errno), but rather
> accumulating the errors not
> in numbers but string buffers?

After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone.

But it's a little more invasive.  What do you think?

Thanks, Jonathan

Stefan Beller· Nov 18, 2014, 01:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (meta-comment: please snip out the context you are not responding to,
>  to make reading easier)
will do
Show 6 quoted lines
>
> After this patch, setting errno is not part of the contract of
> copy_fd, so the bug Ronnie was fixing is gone.
>
> But it's a little more invasive.  What do you think?
>

I really like that approach and would be happy if we'd take it instead of the one I sent.

Jonathan Nieder· Dec 3, 2014, 05:02 UTC · re: Stefan Beller · lore

[PATCH 0/14] Re: copy.c: make copy_fd preserve meaningful errno

Stefan Beller wrote:
> On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
>> After this patch, setting errno is not part of the contract of
>> copy_fd, so the bug Ronnie was fixing is gone.
>>
>> But it's a little more invasive.  What do you think?
>
> I really like that approach and would be happy if

Thanks for looking it over, and sorry for the delay. Here's a series that carries out that approach.

The patch I'm least happy with in this series is 12/14, mostly because it's just so noisy. It would be safe (and non-leaky) to leave out most of those strbuf_release calls since nobody appends to 'err' in the non-error case, but always free-ing means that normal escape analysis can work. I could be convinced to go either way.

Jonathan Nieder (14):
  strbuf: introduce strbuf_prefixf()
  add_to_alternates_file: respect GIT_OBJECT_DIRECTORY
  copy_fd: pass error message back through a strbuf
  hold_lock_file_for_append: pass error message back through a strbuf
  lock_packed_refs: pass error message back through a strbuf
  lockfile: introduce flag for locks outside .git
  fast-import: use message from lockfile API when writing marks fails
  credentials: use message from lockfile API when locking
    ~/.git-credentials fails
  config: use message from lockfile API when locking config file fails
  rerere: error out on autoupdate failure
  hold_locked_index: pass error message back through a strbuf
  hold_lock_file_for_update: pass error message back through a strbuf
  lockfile: remove unused function 'unable_to_lock_die'
  lockfile: make 'unable_to_lock_message' private
 Documentation/technical/api-lockfile.txt | 41 ++++++---------
 builtin/add.c                            | 12 +++--
 builtin/apply.c                          | 15 ++++--
 builtin/checkout-index.c                 | 10 +++-
 builtin/checkout.c                       | 55 ++++++++++++++------
 builtin/clone.c                          | 12 ++++-
 builtin/commit.c                         | 36 +++++++++----
 builtin/describe.c                       | 11 ++--
 builtin/diff.c                           | 12 +++--
 builtin/gc.c                             |  8 ++-
 builtin/merge.c                          | 14 +++--
 builtin/mv.c                             |  5 +-
 builtin/read-tree.c                      |  9 +++-
 builtin/reset.c                          | 10 +++-
 builtin/rm.c                             |  9 +++-
 builtin/update-index.c                   | 10 ++--
 bundle.c                                 | 10 ++--
 cache-tree.c                             | 18 +++++--
 cache.h                                  |  4 +-
 config.c                                 | 14 +++--
 convert.c                                |  6 ++-
 copy.c                                   | 32 ++++++++----
 credential-store.c                       |  8 ++-
 fast-import.c                            |  9 ++--
 lockfile.c                               | 89 ++++++++++++++------------------
 lockfile.h                               | 13 +++--
 merge-recursive.c                        | 13 +++--
 merge.c                                  | 17 ++++--
 read-cache.c                             |  7 +--
 refs.c                                   | 28 ++++++----
 refs.h                                   |  8 +--
 rerere.c                                 | 24 +++++----
 sequencer.c                              | 33 +++++++++---
 sha1_file.c                              | 17 ++++--
 shallow.c                                | 16 ++++--
 strbuf.c                                 | 16 ++++++
 strbuf.h                                 |  4 ++
 test-scrap-cache-tree.c                  |  5 +-
 38 files changed, 434 insertions(+), 226 deletions(-)
Jonathan Nieder· Dec 3, 2014, 05:10 UTC · re: Jonathan Nieder · lore

[PATCH 01/14] strbuf: introduce strbuf_prefixf()

When preparing an error message in a strbuf, it can be convenient to add a formatted string to the beginning:

	if (transaction_commit(&t, err)) {
		strbuf_prefixf(err, "cannot fetch '%s': ", remotename);
		return -1;
	}

The new strbuf_prefixf is like strbuf_addf, except it writes its result to the beginning of a strbuf instead of the end.

The current implementation uses strlen(strfmt(fmt, ...)) extra bytes at the end of the strbuf as temporary scratch space for convenience and simplicity. A later patch can optimize if needed.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 strbuf.c | 16 ++++++++++++++++
 strbuf.h |  4 ++++
 2 files changed, 20 insertions(+)
Show changes to 2 files +20 −0

strbuf.c, strbuf.h

diff --git a/strbuf.c b/strbuf.c
index 0346e74..3f4aaa3 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -219,6 +219,22 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	va_end(ap);
 }
 
+void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list ap;
+	size_t pos, len;
+
+	pos = sb->len;
+
+	va_start(ap, fmt);
+	strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+
+	len = sb->len - pos;
+	strbuf_insert(sb, 0, sb->buf + pos, len);
+	strbuf_remove(sb, pos + len, len);
+}
+
 static void add_lines(struct strbuf *out,
 			const char *prefix1,
 			const char *prefix2,
diff --git a/strbuf.h b/strbuf.h
index 652b6c4..5dae48e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -158,6 +158,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
+/* Like strbuf_addf but insert at the front of sb instead of appending. */
+__attribute__((format (printf,2,3)))
+extern void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...);
+
 /*
  * Append s to sb, with the characters '<', '>', '&' and '"' converted
  * into XML entities.
Eric Sunshine· Dec 3, 2014, 20:10 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 39 quoted lines
> When preparing an error message in a strbuf, it can be convenient
> to add a formatted string to the beginning:
>
>         if (transaction_commit(&t, err)) {
>                 strbuf_prefixf(err, "cannot fetch '%s': ", remotename);
>                 return -1;
>         }
>
> The new strbuf_prefixf is like strbuf_addf, except it writes its
> result to the beginning of a strbuf instead of the end.
>
> The current implementation uses strlen(strfmt(fmt, ...)) extra bytes
> at the end of the strbuf as temporary scratch space for convenience
> and simplicity.  A later patch can optimize if needed.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> diff --git a/strbuf.c b/strbuf.c
> index 0346e74..3f4aaa3 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -219,6 +219,22 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
>         va_end(ap);
>  }
>
> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
> +{
> +       va_list ap;
> +       size_t pos, len;
> +
> +       pos = sb->len;
> +
> +       va_start(ap, fmt);
> +       strbuf_vaddf(sb, fmt, ap);
> +       va_end(ap);
> +
> +       len = sb->len - pos;
> +       strbuf_insert(sb, 0, sb->buf + pos, len);
> +       strbuf_remove(sb, pos + len, len);

Would a strbuf_setlen(sb, pos), rather than strbuf_remove(), make it clearer to the reader that this is merely performing a truncation?

Show 5 quoted lines
> +}
> +
>  static void add_lines(struct strbuf *out,
>                         const char *prefix1,
>                         const char *prefix2,
Jonathan Nieder· Dec 3, 2014, 20:14 UTC · re: Eric Sunshine · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

Eric Sunshine wrote:
> On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 17 quoted lines
>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
>> +{
>> +       va_list ap;
>> +       size_t pos, len;
>> +
>> +       pos = sb->len;
>> +
>> +       va_start(ap, fmt);
>> +       strbuf_vaddf(sb, fmt, ap);
>> +       va_end(ap);
>> +
>> +       len = sb->len - pos;
>> +       strbuf_insert(sb, 0, sb->buf + pos, len);
>> +       strbuf_remove(sb, pos + len, len);
>
> Would a strbuf_setlen(sb, pos), rather than strbuf_remove(), make it
> clearer to the reader that this is merely performing a truncation?
Good idea.  I'll do that.

Thanks, Jonathan

Junio C Hamano· Dec 3, 2014, 21:45 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> When preparing an error message in a strbuf, it can be convenient
> to add a formatted string to the beginning:
>
> 	if (transaction_commit(&t, err)) {
> 		strbuf_prefixf(err, "cannot fetch '%s': ", remotename);
> 		return -1;
> 	}

I am somewhat unhappy with this justification, as it is not very clear why "cannot fetch '%s': " must come at the beginning without knowing what kind of string transaction_commit() is expected to add to err when it is called. It could be a sign that the convention for transaction_commit() to report its errors is screwed up, and not a sign that prefixf is necessary (not that I think that is the case---there is not enough explanation here to decide).

Show 15 quoted lines
> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
> +{
> +	va_list ap;
> +	size_t pos, len;
> +
> +	pos = sb->len;
> +
> +	va_start(ap, fmt);
> +	strbuf_vaddf(sb, fmt, ap);
> +	va_end(ap);
> +
> +	len = sb->len - pos;
> +	strbuf_insert(sb, 0, sb->buf + pos, len);
> +	strbuf_remove(sb, pos + len, len);
> +}

This indeed is strange to read; it would be more straightforward to use a second strbuf for temporary storage you need to do this, instead of using the tail-end of the original strbuf and shuffling bytes around.

In any case, instead of this:
	struct strbuf tc_err = STRBUF_INIT;
        if (transaction_commit(&t, &tc_err)) {
		strbuf_addf(err, "cannot fetch '%s': %s", remotename, 
			tc_err.buf);        
		strbuf_release(&tc_err);
		return -1;
	}

you can use the four-line version you cited above, which might be an improvement. I dunno if it is such a big deal, though.

Jonathan Nieder· Dec 3, 2014, 21:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 20 quoted lines
>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
>> +{
>> +	va_list ap;
>> +	size_t pos, len;
>> +
>> +	pos = sb->len;
>> +
>> +	va_start(ap, fmt);
>> +	strbuf_vaddf(sb, fmt, ap);
>> +	va_end(ap);
>> +
>> +	len = sb->len - pos;
>> +	strbuf_insert(sb, 0, sb->buf + pos, len);
>> +	strbuf_remove(sb, pos + len, len);
>> +}
>
> This indeed is strange to read; it would be more straightforward to
> use a second strbuf for temporary storage you need to do this,
> instead of using the tail-end of the original strbuf and shuffling
> bytes around.

I could do that. It's less efficient but if the prevailing sentiment is that it's worth it then I don't mind.

Would adding a comment to the implementation of strbuf_prefixf help?
Show 12 quoted lines
> In any case, instead of this:
>
> 	struct strbuf tc_err = STRBUF_INIT;
>       if (transaction_commit(&t, &tc_err)) {
> 		strbuf_addf(err, "cannot fetch '%s': %s", remotename,
> 			tc_err.buf);
> 		strbuf_release(&tc_err);
> 		return -1;
> 	}
>
> you can use the four-line version you cited above, which might be an
> improvement.
Yes, that's the idea.

I'll do the tc_err thing, since I'm not getting a clear feeling that I've offered enough motivation for the prefixf approach.

Jonathan
Jonathan Nieder· Dec 3, 2014, 22:09 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

Jonathan Nieder wrote:
> Junio C Hamano wrote:
Show 17 quoted lines
>> In any case, instead of this:
>>
>> 	struct strbuf tc_err = STRBUF_INIT;
>>       if (transaction_commit(&t, &tc_err)) {
>> 		strbuf_addf(err, "cannot fetch '%s': %s", remotename,
>> 			tc_err.buf);
>> 		strbuf_release(&tc_err);
>> 		return -1;
>> 	}
>>
>> you can use the four-line version you cited above, which might be an
>> improvement.
>
> Yes, that's the idea.
>
> I'll do the tc_err thing, since I'm not getting a clear feeling that
> I've offered enough motivation for the prefixf approach.
The tc_err approach looks like this.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
 lockfile.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
Show changes to diff +15 −10
diff --git i/lockfile.c w/lockfile.c
index 8685c68..4e2dfa3 100644
--- i/lockfile.c
+++ w/lockfile.c
@@ -182,15 +182,16 @@ int hold_lock_file_for_update(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 tc_err = STRBUF_INIT;
+	int fd, orig_fd = -1;
 
 	assert(!(flags & LOCK_DIE_ON_ERROR));
-	assert(err && !err->len);
+	assert(err);
 
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
 		unable_to_lock_message(path, errno, err);
-		return fd;
+		goto fail;
 	}
 
 	orig_fd = open(path, O_RDONLY);
@@ -198,18 +199,22 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,
 		if (errno != ENOENT) {
 			strbuf_addf(err, "cannot open '%s' for copying: %s",
 				    path, strerror(errno));
-			rollback_lock_file(lk);
-			return -1;
+			goto fail;
 		}
-	} else if (copy_fd(orig_fd, fd, err)) {
-		strbuf_prefixf(err, "cannot copy '%s': ", path);
-		close(orig_fd);
-		rollback_lock_file(lk);
-		return -1;
+	} else if (copy_fd(orig_fd, fd, &tc_err)) {
+		strbuf_addf(err, "cannot copy '%s': %s", path, tc_err.buf);
+		goto fail;
 	} else {
 		close(orig_fd);
 	}
+	strbuf_release(&tc_err);
 	return fd;
+fail:
+	if (orig_fd >= 0)
+		close(orig_fd);
+	rollback_lock_file(lk);
+	strbuf_release(&tc_err);
+	return -1;
 }
 
 FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
Junio C Hamano· Dec 3, 2014, 22:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 28 quoted lines
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
>>> +{
>>> +	va_list ap;
>>> +	size_t pos, len;
>>> +
>>> +	pos = sb->len;
>>> +
>>> +	va_start(ap, fmt);
>>> +	strbuf_vaddf(sb, fmt, ap);
>>> +	va_end(ap);
>>> +
>>> +	len = sb->len - pos;
>>> +	strbuf_insert(sb, 0, sb->buf + pos, len);
>>> +	strbuf_remove(sb, pos + len, len);
>>> +}
>>
>> This indeed is strange to read; it would be more straightforward to
>> use a second strbuf for temporary storage you need to do this,
>> instead of using the tail-end of the original strbuf and shuffling
>> bytes around.
>
> I could do that.  It's less efficient but if the prevailing sentiment
> is that it's worth it then I don't mind.
>
> Would adding a comment to the implementation of strbuf_prefixf help?
Perhaps.

The reason why it felt strange to me was primarily because this was a short-hand way of writing something like this in the caller:

	if (transaction_commit(&t, err)) {
		struct strbuf scratch = STRBUF_INIT;
		strbuf_addf(&scratch, "cannot fetch '%s': ", remotename);
		strbuf_splice(err, 0, 0, sctach.buf, scratch.len);
                strbuf_reset(&scratch);
	}

Coming from that point of view, it looked strange not to be using a separate scratch area; that's all.

Jonathan Nieder· Dec 3, 2014, 05:12 UTC · re: Jonathan Nieder · lore

[PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY

The objects directory is spelled as get_object_directory(), not git_path("objects"). Some other code still hard-codes the objects/ directory name, so in the long term we may want to get rid of the pretense of support for GIT_OBJECT_DIRECTORY altogether, but this makes the code more consistent for now.

While at it, split variable declarations from the rest of the function. This makes the function a little easier to read, at the cost of some vertical space.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is mainly for consistency / cleanliness.  I wouldn't mind
dropping it.

The rest of 'git clone' is not careful about paying attention to GIT_OBJECT_DIRECTORY either. I suspect GIT_OBJECT_DIRECTORY was a bit of a failed experiment.

 sha1_file.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Show changes to sha1_file.c +9 −3
diff --git a/sha1_file.c b/sha1_file.c
index d7f1838..e1945e2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -403,9 +403,15 @@ void read_info_alternates(const char * relative_base, int depth)
 
 void add_to_alternates_file(const char *reference)
 {
-	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
-	char *alt = mkpath("%s\n", reference);
+	struct lock_file *lock;
+	int fd;
+	char *alt;
+
+	lock = xcalloc(1, sizeof(*lock));
+	fd = hold_lock_file_for_append(lock, mkpath("%s/info/alternates",
+						    get_object_directory()),
+				       LOCK_DIE_ON_ERROR);
+	alt = mkpath("%s\n", reference);
 	write_or_die(fd, alt, strlen(alt));
 	if (commit_lock_file(lock))
 		die("could not close alternates file");
Junio C Hamano· Dec 3, 2014, 18:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 14 quoted lines
> The objects directory is spelled as get_object_directory(), not
> git_path("objects").  Some other code still hard-codes the objects/
> directory name, so in the long term we may want to get rid of the
> pretense of support for GIT_OBJECT_DIRECTORY altogether, but this
> makes the code more consistent for now.
>
> While at it, split variable declarations from the rest of the
> function.  This makes the function a little easier to read, at the
> cost of some vertical space.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This is mainly for consistency / cleanliness.  I wouldn't mind
> dropping it.

I think this is a worthwhile thing to do, but you probably would want to Cc author of nd/multiple-work-trees topic which would need to address similar concern.

> The rest of 'git clone' is not careful about paying attention to
> GIT_OBJECT_DIRECTORY either.  I suspect GIT_OBJECT_DIRECTORY was a bit
> of a failed experiment.

"clone" is a bit special in that it needs to be careful about the "local" case. Does GIT_OBJECT_DIRECTORY (or GIT_DIR for that matter) refer to the source or to the destination?

Show 26 quoted lines
>  sha1_file.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index d7f1838..e1945e2 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -403,9 +403,15 @@ void read_info_alternates(const char * relative_base, int depth)
>  
>  void add_to_alternates_file(const char *reference)
>  {
> -	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
> -	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
> -	char *alt = mkpath("%s\n", reference);
> +	struct lock_file *lock;
> +	int fd;
> +	char *alt;
> +
> +	lock = xcalloc(1, sizeof(*lock));
> +	fd = hold_lock_file_for_append(lock, mkpath("%s/info/alternates",
> +						    get_object_directory()),
> +				       LOCK_DIE_ON_ERROR);
> +	alt = mkpath("%s\n", reference);
>  	write_or_die(fd, alt, strlen(alt));
>  	if (commit_lock_file(lock))
>  		die("could not close alternates file");
Jonathan Nieder· Dec 3, 2014, 05:13 UTC · re: Jonathan Nieder · lore

[PATCH 03/14] copy_fd: pass error message back through a strbuf

This way, callers can put the message in context or even avoid printing the message altogether.

Currently hold_lock_file_for_append tries to save errno in order to produce a meaningful message about the failure and tries to print a second message using errno. Unfortunately the errno it uses is not meaningful because copy_fd makes no effort to set a meaningful errno value. This change is preparation for simplifying the hold_lock_file_for_append behavior.

No user-visible change intended yet.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The title feature.
 cache.h    |  2 +-
 convert.c  |  6 +++++-
 copy.c     | 32 +++++++++++++++++++++-----------
 lockfile.c |  6 +++++-
 4 files changed, 32 insertions(+), 14 deletions(-)
Show changes to 4 files +32 −14

cache.h, convert.c, copy.c, lockfile.c

diff --git a/cache.h b/cache.h
index 99ed096..ddaa30f 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
 extern void maybe_flush_or_die(FILE *, const char *);
 __attribute__((format (printf, 2, 3)))
 extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 extern void write_or_die(int fd, const void *buf, size_t count);
diff --git a/convert.c b/convert.c
index 9a5612e..e301447 100644
--- a/convert.c
+++ b/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
 	if (params->src) {
 		write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
 	} else {
-		write_err = copy_fd(params->fd, child_process.in);
+		struct strbuf err = STRBUF_INIT;
+		write_err = copy_fd(params->fd, child_process.in, &err);
+		if (write_err)
+			error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
 	}
 
 	if (close(child_process.in))
diff --git a/copy.c b/copy.c
index f2970ec..828661a 100644
--- a/copy.c
+++ b/copy.c
@@ -1,19 +1,22 @@
 #include "cache.h"
 
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
 {
+	assert(err);
+
 	while (1) {
 		char buffer[8192];
 		ssize_t len = xread(ifd, buffer, sizeof(buffer));
 		if (!len)
 			break;
 		if (len < 0) {
-			return error("copy-fd: read returned %s",
-				     strerror(errno));
+			strbuf_addf(err, "read returned %s", strerror(errno));
+			return -1;
+		}
+		if (write_in_full(ofd, buffer, len) < 0) {
+			strbuf_addf(err, "write returned %s", strerror(errno));
+			return -1;
 		}
-		if (write_in_full(ofd, buffer, len) < 0)
-			return error("copy-fd: write returned %s",
-				     strerror(errno));
 	}
 	return 0;
 }
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
 
 int copy_file(const char *dst, const char *src, int mode)
 {
-	int fdi, fdo, status;
+	int fdi, fdo;
+	struct strbuf err = STRBUF_INIT;
 
 	mode = (mode & 0111) ? 0777 : 0666;
 	if ((fdi = open(src, O_RDONLY)) < 0)
@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
 		close(fdi);
 		return fdo;
 	}
-	status = copy_fd(fdi, fdo);
+	if (copy_fd(fdi, fdo, &err)) {
+		close(fdi);
+		close(fdo);
+		error("copy-fd: %s", err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
+	strbuf_release(&err);
 	close(fdi);
 	if (close(fdo) != 0)
 		return error("%s: close error: %s", dst, strerror(errno));
-
-	if (!status && adjust_shared_perm(dst))
+	if (adjust_shared_perm(dst))
 		return -1;
 
-	return status;
+	return 0;
 }
 
 int copy_file_with_time(const char *dst, const char *src, int mode)
diff --git a/lockfile.c b/lockfile.c
index 4f16ee7..b3020f3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -182,6 +182,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 {
 	int fd, orig_fd;
+	struct strbuf err = STRBUF_INIT;
 
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
@@ -202,9 +203,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 			errno = save_errno;
 			return -1;
 		}
-	} else if (copy_fd(orig_fd, fd)) {
+	} 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);
 		close(orig_fd);
@@ -214,6 +217,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 	} else {
 		close(orig_fd);
 	}
+	strbuf_release(&err);
 	return fd;
 }
Junio C Hamano· Dec 3, 2014, 19:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Jonathan Nieder <jrnieder@gmail.com> writes:
> -extern int copy_fd(int ifd, int ofd);
> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);

It is not limited to this single function, but what contract do we envision this "error messages are given back to the caller via strbuf" convention should give between the callers and the callee?

For example, is it a bug in the callee to touch "err" when there is no error to report? Another example is if we should allow the callers to pass NULL there when they do not care about the nature of the error (e.g. "git cmd -q").

There may be other rules we want to enforce consistently across functions that adopt this convention.

The change in this patch looks sensible.
Thanks.
Jonathan Nieder· Dec 3, 2014, 20:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> -extern int copy_fd(int ifd, int ofd);
>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
>
> It is not limited to this single function, but what contract do we
> envision this "error messages are given back to the caller via
> strbuf" convention should give between the callers and the callee?

Good point. I'll add a Documentation/technical/api-error-handling.txt describing die(), error(), warning(), and the strbuf-based method.

Thanks, Jonathan

Jeff King· Dec 3, 2014, 21:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On Wed, Dec 03, 2014 at 11:01:08AM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > -extern int copy_fd(int ifd, int ofd);
> > +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
> 
> It is not limited to this single function, but what contract do we
> envision this "error messages are given back to the caller via
> strbuf" convention should give between the callers and the callee?
> 
> For example, is it a bug in the callee to touch "err" when there is
> no error to report?  Another example is if we should allow the
> callers to pass NULL there when they do not care about the nature of
> the error (e.g. "git cmd -q").
> 
> There may be other rules we want to enforce consistently across
> functions that adopt this convention.

It seems like we are really re-designing the error-handling chain here. Maybe it is worth taking a step back and thinking about our overall strategy.

Why do we dislike errno? Because it is global state, and it is easy for it to get stomped by unrelated operations. Another reason is that it carries no parameters. You see "ENOENT", but you do not have any context (e.g., which file).

What's good about errno? It is machine-readable (i.e., callers can check for ENOENT, as opposed to text in a strbuf). It does not require any allocation. Besides making it slightly more robust, it removes any responsibility for resource cleanup from the caller. It's globalness is also convenient; you do not need to add an extra parameter to each function to handle errors.

So what are some alternatives?

Passing back "-errno" instead of "-1" helps with the stomping issue (and without adding extra parameters). It also retains machine-readability (which I'll call just readability from now on). But it doesn't help with context, or better messaging.

Your solution adds a strbuf. That helps with context and stomping, but loses readability and adds allocation.

If we changed the strbuf to a fixed-size buffer, that would help the allocation issue. Some messages might be truncated, but it seems unlikely in practice. It still loses readability, though.

What about a struct that has an errno-like value _and_ a fixed-size buffer? I'm thinking something like:

  struct error {
	int code;
	char msg[1024];
  };
  /* caller who wants to print the message; no need to free message */
  struct error err;
  if (copy_fd(from, to, &err))
	return error("%s", err.msg);
  /* caller who does not; they can pass NULL */
  if (copy_fd(from, to, NULL))
	return -1;
  /* or they can use it to grab the errno value */
  struct error err;
  if (copy_fd(from, to, &err)) {
	if (err.code == EPIPE)
		exit(141);
	return -1;
  }
  /* function which generates error */
  void read_foo(const char *fn, struct error *err)
  {
	if (open(fn, O_RDONLY))
		return mkerror(err, errno, "unable to open %s", fn);
	... do other stuff ...
	return 0;
  }
  /* helper function for generating errors */
  int mkerror(struct error *err, int code, const char *fmt, ...)
  {
	va_list ap;
	int len;
	if (!err)
		return;
	err->code = code;
	va_start(ap, fmt);
	len = vsnprintf(err->msg, sizeof(err->msg), fmt, ap);
	va_end(ap);
	if (code)
		snprintf(err->msg + len, sizeof(err->msg) - len,
			 ": %s", strerror(code));
	return -1;
  }
You can also take the machine-readable thing a step further, like:
  struct error {
	int code;
	char param1[1024];
	char param2[1024];
	/* 2 parameters should be big enough for anyone, right? */
  }

and then generate the message on the fly when printing. This gives the callers more information. But it also means defining a constant for "code" for every error message, which is annoying. Libraries often do this, but I do not think we need to go that far here.

-Peff
Jonathan Nieder· Dec 3, 2014, 21:38 UTC · re: Jeff King · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Hi,
Jeff King wrote:
Show 7 quoted lines
> What about a struct that has an errno-like value _and_ a fixed-size
> buffer? I'm thinking something like:
>
>   struct error {
> 	int code;
> 	char msg[1024];
>   };

My experience with errno is that it is very hard to anticipate what granularity to use with error codes, especially if the error code is going to (in some contexts) determine the message printed to the user. In practice, it is easier to come up with a message at the error detection site and use a generic "something happened" error code except in those places where the caller is going to act on specific error types that need to be distinguished.

Passing a message back through a strbuf leaves room for such interpretations for the int return value.

The "< 0 means error" convention gives room to use different exit codes for the errors that need to be programmatically distinguished. For example, the ref transaction API uses this to distinguish D/F conflicts from other errors, while still returning an error message through a strbuf output parameter.

This doesn't help with functions that want to return a pointer (and NULL on error). For those, we can use an int output parameter, copy the kernel's err_ptr trick, or use other sentinels for recoverable errors that the caller wants to know about. It hasn't come up in practice enough for me to be motivated to want to choose between those.

> If we changed the strbuf to a fixed-size buffer, that would help the
> allocation issue. Some messages might be truncated, but it seems
> unlikely in practice. It still loses readability, though.

I don't like the idea of having to choose a size in advance and not being able to fit more detailed (perhaps language-specific, and including user-specified input) messages lest the buffer overflow. The allocation of a variable-sized buffer is a small overhead that I don't mind incurring on error. In the non-error case, the caller doesn't actually have to free the buffer, and if they choose to, the overhead incurred is that of free(NULL)'.

My two cents, Jonathan

Jeff King· Dec 4, 2014, 07:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:
Show 15 quoted lines
> > What about a struct that has an errno-like value _and_ a fixed-size
> > buffer? I'm thinking something like:
> >
> >   struct error {
> > 	int code;
> > 	char msg[1024];
> >   };
> 
> My experience with errno is that it is very hard to anticipate what
> granularity to use with error codes, especially if the error code is
> going to (in some contexts) determine the message printed to the user.
> In practice, it is easier to come up with a message at the error
> detection site and use a generic "something happened" error code
> except in those places where the caller is going to act on specific
> error types that need to be distinguished.

Yeah, I agree. I think error.msg there would be the main use, and error.code is just gravy. I agree it could simply come back through the integer return value, too.

Show 5 quoted lines
> The "< 0 means error" convention gives room to use different exit
> codes for the errors that need to be programmatically distinguished.
> For example, the ref transaction API uses this to distinguish D/F
> conflicts from other errors, while still returning an error message
> through a strbuf output parameter.

Yup. One nice thing about stuffing it into the error struct is that it saves the caller from declaring a separate variable (assuming they need "err" anyway to collect the message). I.e.:

  struct error err;
  if (some_func(fd, &err))
	react_to(err.code);
as opposed to:
  struct error err;
  int ret;
  ret = some_func(fd, &err);
  if (ret)
	react_to(ret);

But I think it's a minority of cases where we care about the specific value anyway, so it's probably not a big deal either way.

Show 7 quoted lines
> > If we changed the strbuf to a fixed-size buffer, that would help the
> > allocation issue. Some messages might be truncated, but it seems
> > unlikely in practice. It still loses readability, though.
> 
> I don't like the idea of having to choose a size in advance and not
> being able to fit more detailed (perhaps language-specific, and
> including user-specified input) messages lest the buffer overflow.

Is that really a problem in practice? Is a message larger than 1024 characters actually readable? It would have to be broken across many lines, and then I think we are no longer dealing with an error message, but some advice (which probably shouldn't go through this mechanism anyway).

> The allocation of a variable-sized buffer is a small overhead that I
> don't mind incurring on error.  In the non-error case, the caller
> doesn't actually have to free the buffer, and if they choose to, the
> overhead incurred is that of free(NULL)'.

I don't care at all about overhead. I care about extra work on the part of the caller to avoid a leak. It turns:

  if (some_func(fd, &err))
	return error("%s", err.msg);
into:
  if (some_func(fd, &err)) {
	error("%s", err.buf);
	strbuf_release(&err);
	return -1;
  }

It may make things a little easier when an intermediate function has to further munge the message. For example, your 04/14 uses strbuf_prefixf. But that can also be expressed in a buffer world as:

  /* "parent_err" is passed to us from caller, "err" is a local buffer */
  if (copy_fd(orig, fd, &err))
	return mkerror(&parent_err, "cannot copy '%s': %s", path, err.msg);

Strbufs make more complicated munging possible, but I'd expect prefixing to be the most common case (except for not munging at all, which I think is even more common).

I dunno. I just hate to see a system where it's very easy for callers to introduce memory leaks by forgetting a strbuf_release, or that bogs callers down with error-handling boilerplate.

-Peff
Stefan Beller· Dec 4, 2014, 08:36 UTC · re: Jeff King · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

> Your solution adds a strbuf. That helps with context and stomping, but
> loses readability and adds allocation.
> If we changed the strbuf to a fixed-size buffer, that would help the
> allocation issue. Some messages might be truncated, but it seems
> unlikely in practice. It still loses readability, though.
> What about a struct that has an errno-like value _and_ a fixed-size
> buffer? I'm thinking something like:

What do you mean by the allocation being an issue? We're only populating the error buffer in the error case, so you're not talking about performance/speed I'd assume? As error handling breaks in the least expected ways, I'd rather go with well tested string buffer codes there?

Jeff King· Dec 4, 2014, 09:04 UTC · re: Stefan Beller · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote:
Show 11 quoted lines
> > Your solution adds a strbuf. That helps with context and stomping, but
> > loses readability and adds allocation.
> 
> > If we changed the strbuf to a fixed-size buffer, that would help the
> > allocation issue. Some messages might be truncated, but it seems
> > unlikely in practice. It still loses readability, though.
> 
> > What about a struct that has an errno-like value _and_ a fixed-size
> > buffer? I'm thinking something like:
> 
> What do you mean by the allocation being an issue?

I mean that the caller has to take care of releasing the memory. This adds boilerplate to the caller, and is a potential source of leaks.

> We're only populating the error buffer in the error case, so you're
> not talking about performance/speed I'd assume?
No, I don't care about performance here. Only code maintainability.
> As error handling breaks in the least expected ways, I'd rather go
> with well tested string buffer codes there?

We'd still be building on snprintf. And with a known-size buffer, we can wrap the formatting so that the callers don't even have to care (see the mkerror example I posted).

-Peff
Michael Haggerty· Dec 10, 2014, 17:02 UTC · re: Jeff King · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On 12/04/2014 08:59 AM, Jeff King wrote:
Show 19 quoted lines
> On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:
>> The allocation of a variable-sized buffer is a small overhead that I
>> don't mind incurring on error.  In the non-error case, the caller
>> doesn't actually have to free the buffer, and if they choose to, the
>> overhead incurred is that of free(NULL)'.
> 
> I don't care at all about overhead. I care about extra work on the part
> of the caller to avoid a leak. It turns:
> 
>   if (some_func(fd, &err))
> 	return error("%s", err.msg);
> 
> into:
> 
>   if (some_func(fd, &err)) {
> 	error("%s", err.buf);
> 	strbuf_release(&err);
> 	return -1;
>   }

What if we go in the direction not of less infrastructure, but a little bit more? Like

	struct result {
		int code;
		struct strbuf msg;
	};
	int report_errors(struct result *result)
	{
		int code = result->code;
		if (code) {
			error(result->msg.buf);
		}
		result->code = 0;
		strbuf_release(result->msg);
		return code; /* or alternatively (code ? -1 : 0) */
	}
	int report_warnings(struct result *result)
	{
		...
	}
	int report_with_prefix(struct result *result, const char *fmt, ...)
	{
		...
	}
Then a caller could look pretty much like before:
	struct result result = RESULT_INIT;
	if (some_func(fd, &result))
		return report_errors(&result);

Other callers might not even bother to check the return value of the function, relying instead on result.code via process_error():

	char *ptr = some_func(fd, &result);
	if (report_errors(&result))
		return -1;

If the result code is considered superfluous, we could use naked strbufs and use msg.len as the indicator that there was an error.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Junio C Hamano· Dec 10, 2014, 19:00 UTC · re: Michael Haggerty · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 45 quoted lines
> What if we go in the direction not of less infrastructure, but a little
> bit more? Like
>
> 	struct result {
> 		int code;
> 		struct strbuf msg;
> 	};
>
> 	int report_errors(struct result *result)
> 	{
> 		int code = result->code;
> 		if (code) {
> 			error(result->msg.buf);
> 		}
> 		result->code = 0;
> 		strbuf_release(result->msg);
> 		return code; /* or alternatively (code ? -1 : 0) */
> 	}
>
> 	int report_warnings(struct result *result)
> 	{
> 		...
> 	}
>
> 	int report_with_prefix(struct result *result, const char *fmt, ...)
> 	{
> 		...
> 	}
>
> Then a caller could look pretty much like before:
>
> 	struct result result = RESULT_INIT;
>
> 	if (some_func(fd, &result))
> 		return report_errors(&result);
>
> Other callers might not even bother to check the return value of the
> function, relying instead on result.code via process_error():
>
> 	char *ptr = some_func(fd, &result);
> 	if (report_errors(&result))
> 		return -1;
>
> If the result code is considered superfluous, we could use naked strbufs
> and use msg.len as the indicator that there was an error.
Two potential issues are:
 - Callers that ignore errors need to actively ignore errors with
   strbuf_release(&result.msg);
 - Callers have to remember that once the report_errors() function
   is called on a "struct result", the struct loses its information.

Neither is insurmountable, but the latter might turn out to be cumbersome to work around in some codepaths.

Other than that, I think it is OK.

Another alternative may be to have the reporting storage on the side of the callee, and have callers that are interested in errors to supply a place to store a pointer to it, i.e.

	int some_func(..., struct result **errors) {
        	static struct result mine;
		clear_result(&mine);
		... do its thing ...
                if (... error detected ...) {
                        mine.code = E_SOMEFUNC;
                        strbuf_addstr(&mine.msg, "some_func: foobared");
		}
		if (errors)
			*errors = &mine;
                return mine.code;
	}
And a caller would do this instead:
	struct result *result = NULL;
        if (some_func(fd, &result))
		return report_errors(result);

where report_errors() and friends will only peek but not clear the result reporting storage. The clear_result() helper would trivially be:

	void clear_result(struct result *result) {
		result->code = 0;
                strbuf_release(&result->msg);
	}
Jeff King· Dec 10, 2014, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On Wed, Dec 10, 2014 at 11:00:18AM -0800, Junio C Hamano wrote:
> Two potential issues are:
> 
>  - Callers that ignore errors need to actively ignore errors with
>    strbuf_release(&result.msg);

That was my first thought, too. If you want to do anything besides report_error, you have to deal with the strbuf. But I'd guess that they often fall into one of two cases:

  1. You are just propagating the error to your caller. In which case
      it is not _your_ result struct in the first place, and you do not
      need to care about deallocating it either way. I.e.:
        int some_func(..., struct result *err)
	{
		if (some_other_func(..., err))
			return -1;
		...
	}
  2. You want to ignore the error. I think anybody taking a result
     struct (or a strbuf, or whatever) should accept NULL as "do not
     bother giving me your message". And the convenience wrappers
     should handle that (I think the mkerror example I sent earlier
     did), so callees can just do:
       return mkerror(err, "whatever: %s", ...);

The remainder could strbuf_release manually, but there would hopefully not be many of them.

I think I could live with something like that.
Show 5 quoted lines
>  - Callers have to remember that once the report_errors() function
>    is called on a "struct result", the struct loses its information.
> 
> Neither is insurmountable, but the latter might turn out to be
> cumbersome to work around in some codepaths.

I suspect the message is not that interesting after calling report_errors(). The "code" flag could remain, as it does not require deallocation.

Show 6 quoted lines
> Another alternative may be to have the reporting storage on the side
> of the callee, and have callers that are interested in errors to
> supply a place to store a pointer to it, i.e.
> 
> 	int some_func(..., struct result **errors) {
>         	static struct result mine;

This makes some_func not reentrant. Which is a hazard both for threaded code, but also for functions which want to do:

  if (some_func(foo, &err_one)) {
          /* didn't work? Try an alternative. */
	  if (!some_func(bar, &err_two))
	          ....
and expect err_one to contain anything useful.
Jonathan Nieder· Dec 4, 2014, 03:01 UTC · re: Junio C Hamano · lore

[PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> -extern int copy_fd(int ifd, int ofd);
>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
>
> It is not limited to this single function, but what contract do we
> envision this "error messages are given back to the caller via
> strbuf" convention should give between the callers and the callee?
Here's a draft for documentation on that.
 Documentation/technical/api-error-handling.txt | 75 ++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/technical/api-error-handling.txt
Show changes to Documentation/technical/api-error-handling.txt +75 −0
diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
new file mode 100644
index 0000000..fc68db1
--- /dev/null
+++ b/Documentation/technical/api-error-handling.txt
@@ -0,0 +1,75 @@
+Error reporting in git
+======================
+
+`die`, `usage`, `error`, and `warning` report errors of various
+kinds.
+
+- `die` is for fatal application errors.  It prints a message to
+  the user and exits with status 128.
+
+- `usage` is for errors in command line usage.  After printing its
+  message, it exits with status 129.  (See also `usage_with_options`
+  in the link:api-parse-options.html[parse-options API].)
+
+- `error` is for non-fatal library errors.  It prints a message
+  to the user and returns -1 for convenience in signaling the error
+  to the caller.
+
+- `warning` is for reporting situations that probably should not
+  occur but which the user (and Git) can continue to work around
+  without running into too many problems.  Like `error`, it
+  returns -1 after reporting the situation to the caller.
+
+Customizable error handlers
+---------------------------
+
+The default behavior of `die` and `error` is to write a message to
+stderr and then exit or return as appropriate.  This behavior can be
+overridden using `set_die_routine` and `set_error_routine`.  For
+example, "git daemon" uses set_die_routine to write the reason `die`
+was called to syslog before exiting.
+
+Library errors
+--------------
+
+Functions return a negative integer on error.  Details beyond that
+vary from function to function:
+
+- Some functions return -1 for all errors.  Others return a more
+  specific value depending on how the caller might want to react
+  to the error.
+
+- Some functions report the error to stderr with `error`,
+  while others leave that for the caller to do.
+
+- errno is not meaningful on return from most functions (except
+  for thin wrappers for system calls).
+
+Check the function's API documentation to be sure.
+
+Caller-handled errors
+---------------------
+
+An increasing number of functions take a parameter 'struct strbuf *err'.
+On error, such functions append a message about what went wrong to the
+'err' strbuf.  The message is meant to be complete enough to be passed
+to `die` or `error` as-is.  For example:
+
+	if (ref_transaction_commit(transaction, &err))
+		die("%s", err.buf);
+
+The 'err' parameter will be untouched if no error occured, so multiple
+function calls can be chained:
+
+	t = ref_transaction_begin(&err);
+	if (!t ||
+	    ref_transaction_update(t, "HEAD", ..., &err) ||
+	    ret_transaction_commit(t, &err))
+		die("%s", err.buf);
+
+The 'err' parameter must be a pointer to a valid strbuf.  To silence
+a message, pass a strbuf that is explicitly ignored:
+
+	if (thing_that_can_fail_in_an_ignorable_way(..., &err))
+		/* This failure is okay. */
+		strbuf_reset(&err);
Junio C Hamano· Dec 4, 2014, 23:27 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> -extern int copy_fd(int ifd, int ofd);
>>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
>>
>> It is not limited to this single function, but what contract do we
>> envision this "error messages are given back to the caller via
>> strbuf" convention should give between the callers and the callee?
>
> Here's a draft for documentation on that.

Thanks; looks reasonable; even if the discussion between you and Peff took us to a slightly different direction than what you described here, the earlier description of long established practice is a welcome addition.

Show 86 quoted lines
>  Documentation/technical/api-error-handling.txt | 75 ++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/technical/api-error-handling.txt
>
> diff --git a/Documentation/technical/api-error-handling.txt
> b/Documentation/technical/api-error-handling.txt
> new file mode 100644
> index 0000000..fc68db1
> --- /dev/null
> +++ b/Documentation/technical/api-error-handling.txt
> @@ -0,0 +1,75 @@
> +Error reporting in git
> +======================
> +
> +`die`, `usage`, `error`, and `warning` report errors of various
> +kinds.
> +
> +- `die` is for fatal application errors.  It prints a message to
> +  the user and exits with status 128.
> +
> +- `usage` is for errors in command line usage.  After printing its
> +  message, it exits with status 129.  (See also `usage_with_options`
> +  in the link:api-parse-options.html[parse-options API].)
> +
> +- `error` is for non-fatal library errors.  It prints a message
> +  to the user and returns -1 for convenience in signaling the error
> +  to the caller.
> +
> +- `warning` is for reporting situations that probably should not
> +  occur but which the user (and Git) can continue to work around
> +  without running into too many problems.  Like `error`, it
> +  returns -1 after reporting the situation to the caller.
> +
> +Customizable error handlers
> +---------------------------
> +
> +The default behavior of `die` and `error` is to write a message to
> +stderr and then exit or return as appropriate.  This behavior can be
> +overridden using `set_die_routine` and `set_error_routine`.  For
> +example, "git daemon" uses set_die_routine to write the reason `die`
> +was called to syslog before exiting.
> +
> +Library errors
> +--------------
> +
> +Functions return a negative integer on error.  Details beyond that
> +vary from function to function:
> +
> +- Some functions return -1 for all errors.  Others return a more
> +  specific value depending on how the caller might want to react
> +  to the error.
> +
> +- Some functions report the error to stderr with `error`,
> +  while others leave that for the caller to do.
> +
> +- errno is not meaningful on return from most functions (except
> +  for thin wrappers for system calls).
> +
> +Check the function's API documentation to be sure.
> +
> +Caller-handled errors
> +---------------------
> +
> +An increasing number of functions take a parameter 'struct strbuf *err'.
> +On error, such functions append a message about what went wrong to the
> +'err' strbuf.  The message is meant to be complete enough to be passed
> +to `die` or `error` as-is.  For example:
> +
> +	if (ref_transaction_commit(transaction, &err))
> +		die("%s", err.buf);
> +
> +The 'err' parameter will be untouched if no error occured, so multiple
> +function calls can be chained:
> +
> +	t = ref_transaction_begin(&err);
> +	if (!t ||
> +	    ref_transaction_update(t, "HEAD", ..., &err) ||
> +	    ret_transaction_commit(t, &err))
> +		die("%s", err.buf);
> +
> +The 'err' parameter must be a pointer to a valid strbuf.  To silence
> +a message, pass a strbuf that is explicitly ignored:
> +
> +	if (thing_that_can_fail_in_an_ignorable_way(..., &err))
> +		/* This failure is okay. */
> +		strbuf_reset(&err);
Jonathan Nieder· Dec 4, 2014, 23:37 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Hi,
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> Here's a draft for documentation on that.
>
> Thanks; looks reasonable; even if the discussion between you and
> Peff took us to a slightly different direction than what you
> described here, the earlier description of long established practice
> is a welcome addition.
Thanks.

Can you say more? I didn't think the discussion had reached a conclusion yet.

Unless there's a strong reason to do otherwise, I prefer to stick with the strbufs in this series for now. I prefer strbuf for this purpose over char[1024]. And switching later doesn't seem hard.

But if others prefer char[1024], I can update the existing code that uses strbuf from the transaction API to use that and reroll this series with that convention. A part of me dislikes that but I'll go along.

Jonathan
Jonathan Nieder· Dec 4, 2014, 23:41 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 6 quoted lines
>> Here's a draft for documentation on that.
>
> Thanks; looks reasonable; even if the discussion between you and
> Peff took us to a slightly different direction than what you
> described here, the earlier description of long established practice
> is a welcome addition.

I think I see what I misunderstood. Do you mean "even if we settle on a different API, this documentation of what you started with should be easy to adapt and will make life easier"?

In other words, did you mean to say that things are still up in the air (which I agree with) or that the project has already settled on a different direction (which I do not)?

Sorry for the confusion, Jonathan

Jeff King· Dec 4, 2014, 23:44 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:
Show 17 quoted lines
> Junio C Hamano wrote:
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >> Here's a draft for documentation on that.
> >
> > Thanks; looks reasonable; even if the discussion between you and
> > Peff took us to a slightly different direction than what you
> > described here, the earlier description of long established practice
> > is a welcome addition.
> 
> I think I see what I misunderstood.  Do you mean "even if we settle on
> a different API, this documentation of what you started with should be
> easy to adapt and will make life easier"?
> 
> In other words, did you mean to say that things are still up in the
> air (which I agree with) or that the project has already settled on a
> different direction (which I do not)?

FWIW, that is how I read it (up in the air), and where I thought our discussion was.

-Peff
Junio C Hamano· Dec 4, 2014, 23:52 UTC · re: Jeff King · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Yeah, that is what I meant. The earlier part will not go to waste no matter what happens to the discussion.

I am not a fan of char[1024], if only because our error message may have to mention things whose length is not under our control, e.g. a filename in the working tree, but I do share your concern that "strbuf"-approach calls for more boilerplate. I offhand do not have a magic silver bullet for it, though.

On Thu, Dec 4, 2014 at 3:44 PM, Jeff King <peff@peff.net> wrote:
Show 24 quoted lines
> On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:
>
>> Junio C Hamano wrote:
>> > Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>> >> Here's a draft for documentation on that.
>> >
>> > Thanks; looks reasonable; even if the discussion between you and
>> > Peff took us to a slightly different direction than what you
>> > described here, the earlier description of long established practice
>> > is a welcome addition.
>>
>> I think I see what I misunderstood.  Do you mean "even if we settle on
>> a different API, this documentation of what you started with should be
>> easy to adapt and will make life easier"?
>>
>> In other words, did you mean to say that things are still up in the
>> air (which I agree with) or that the project has already settled on a
>> different direction (which I do not)?
>
> FWIW, that is how I read it (up in the air), and where I thought our
> discussion was.
>
> -Peff
Jeff King· Dec 5, 2014, 00:01 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On Thu, Dec 04, 2014 at 03:52:45PM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> Yeah, that is what I meant. The earlier part will not go to waste no matter
> what happens to the discussion.
> 
> I am not a fan of char[1024], if only because our error message may have
> to mention things whose length is not under our control, e.g. a filename
> in the working tree, but I do share your concern that "strbuf"-approach
> calls for more boilerplate. I offhand do not have a magic silver bullet for
> it, though.

The only downside I can think of is that we may truncate the message in exceptional circumstances. But is it really any less helpful to say:

  error: unable to open file: some-incredibly-long-filename-aaaaaa...

than printing out an extra 100 lines of "a"? And I mean the "..." literally. I think mkerror() should indicate the truncation with a "...", just so that it is clear to the user. It should almost never happen, but when it does, it can be helpful to show the user that yes, we know we are truncating the message, and it is not that git truncated your filename during the operation.

Is this truncation really a concern, and/or is there some other downside I'm not thinking of?

-Peff
Junio C Hamano· Dec 5, 2014, 18:00 UTC · re: Jeff King · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> The only downside I can think of is that we may truncate the message in
> exceptional circumstances. But is it really any less helpful to say:
>
>   error: unable to open file: some-incredibly-long-filename-aaaaaa...
>
> than printing out an extra 100 lines of "a"? And I mean the "..."
> literally. I think mkerror() should indicate the truncation with a
> "...", just so that it is clear to the user. It should almost never
> happen, but when it does, it can be helpful to show the user that yes,
> we know we are truncating the message, and it is not that git truncated
> your filename during the operation.
>
> Is this truncation really a concern, and/or is there some other downside
> I'm not thinking of?

I am more worried about variable length part pushing the information that is given later out to the right, e.g. "error: missing file '%s' prevents us from doing X". Chomping to [1024] is not a good strategy for that kind of message; abbreviating %s to /path/name/... (again, with literally "...") would be.

Jeff King· Dec 7, 2014, 10:07 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > The only downside I can think of is that we may truncate the message in
> > exceptional circumstances. But is it really any less helpful to say:
> >
> >   error: unable to open file: some-incredibly-long-filename-aaaaaa...
> >
> > than printing out an extra 100 lines of "a"? And I mean the "..."
> > literally. I think mkerror() should indicate the truncation with a
> > "...", just so that it is clear to the user. It should almost never
> > happen, but when it does, it can be helpful to show the user that yes,
> > we know we are truncating the message, and it is not that git truncated
> > your filename during the operation.
> >
> > Is this truncation really a concern, and/or is there some other downside
> > I'm not thinking of?
> 
> I am more worried about variable length part pushing the information
> that is given later out to the right, e.g. "error: missing file '%s'
> prevents us from doing X".  Chomping to [1024] is not a good
> strategy for that kind of message; abbreviating %s to /path/name/...
> (again, with literally "...") would be.

True. Unfortunately I do not think there is an easy way to truncate the expanded strings used by placeholders. The two approaches I could think of are:

  1. Loop over the va_list and tweak any pointers we find. Except that
     loop means we actually need to loop over the _format string_, since
     that is the only thing which tells us which placeholders are strings.
     But I am not even sure there is a portable way to munge a va_list,
     as opposed to just reading it.
  2. Transparently rewrite '%s' in the format string to '%.128s' or
     similar. This also requires iterating over the format string, but
     it's fairly straightforward. Code is below, but it's not _quite_
     right. We would want to conditionally add "..." based on whether or
     not the particular item was truncated. But we cannot know when
     munging the format string if that will happen or not (we know
     _something_ will be truncated, but not which string).

I don't think you can do it right without reimplementing vsnprintf yourself. Which is obviously a terrible idea.

I still think truncation is going to be a non-issue in practice, and I'd rather deal with that potential fallout than have to deal with memory management in every error handler that uses this technique. But I don't have much more to say on the matter, so if you disagree, I guess that's that.

Unless we can do something clever with a set of global error strbufs or something (i.e., that expand as needed, but the caller does not have to free themselves, as they will get recycled eventually). That has its own corner cases, though.

Anyway, the truncating-fmt code is below, for fun.
-Peff
static int abbrev_fmt(char *buf, int len, int abbrev, const char *fmt, va_list ap)
{
	va_list cp;
	int got;
	va_copy(cp, ap);
	got = vsnprintf(buf, len, fmt, ap);
	if (got >= len) {
		struct strbuf munged = STRBUF_INIT;
		while (*fmt) {
			char ch = *fmt++;
			strbuf_addch(&munged, ch);
			if (ch == '%') {
				if (*fmt == 's') {
					strbuf_addf(&munged, ".%ds...", abbrev);
					fmt++;
				} else /* skips past %%-quoting */
					strbuf_addch(&munged, *fmt);
			}
		}
		got = vsnprintf(buf, len, munged.buf, cp);
		strbuf_release(&munged);
	}
	return got;
}
Junio C Hamano· Dec 9, 2014, 18:43 UTC · re: Jeff King · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
>
>> I am more worried about variable length part pushing the information
>> that is given later out to the right, e.g. "error: missing file '%s'
>> prevents us from doing X".  Chomping to [1024] is not a good
>> strategy for that kind of message; abbreviating %s to /path/name/...
>> (again, with literally "...") would be.
>
> True. Unfortunately I do not think there is an easy way to truncate the
> expanded strings used by placeholders...
> ...
> Unless we can do something clever with a set of global error strbufs or
> something (i.e., that expand as needed, but the caller does not have to
> free themselves, as they will get recycled eventually). That has its own
> corner cases, though.

I do share your concern that "strbuf"-approach calls for more boilerplate leading to unmaintainable code, but I offhand do not have a magic silver bullet for it. globals are indeed tempting, but I'd have to say that what Jonathan has may probably be the least bad of the possibilities.

Chomping also has complications when we have to deal with user payload (e.g. pathname in UTF-8), which we may want avoid having to worry about.

Jeff King· Dec 9, 2014, 18:49 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On Tue, Dec 09, 2014 at 10:43:52AM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> > Unless we can do something clever with a set of global error strbufs or
> > something (i.e., that expand as needed, but the caller does not have to
> > free themselves, as they will get recycled eventually). That has its own
> > corner cases, though.
> 
> I do share your concern that "strbuf"-approach calls for more
> boilerplate leading to unmaintainable code, but I offhand do not
> have a magic silver bullet for it.  globals are indeed tempting, but
> I'd have to say that what Jonathan has may probably be the least bad
> of the possibilities.

OK. I'm not sure I agree that it is the least bad, but I don't think it's worth arguing over more. Let's go with it, and you can note my objection in the captain's log. :)

-Peff
Junio C Hamano· Feb 12, 2015, 23:08 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
>>
>>> I am more worried about variable length part pushing the information
>>> that is given later out to the right, e.g. "error: missing file '%s'
>>> prevents us from doing X".  Chomping to [1024] is not a good
>>> strategy for that kind of message; abbreviating %s to /path/name/...
>>> (again, with literally "...") would be.
I have this one in my pile of Undecided topics:
    * jn/doc-api-errors (2014-12-04) 1 commit
     - doc: document error handling functions and conventions
     For discussion.
     What's the status of this one????

I think we all agree that the early part of the new documentation text is good, but the last section that proposes to store more detailed errors in caller supplied strbuf in textual form was controversial (and I have not convinced myself it is a good idea yet).

I could chuck the last section and then start merging the remainder to 'next' to salvage the "obviously good bits". Or do people want to hash its last section a bit more?

Michael Haggerty· Feb 17, 2015, 15:50 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On 02/13/2015 12:08 AM, Junio C Hamano wrote:
Show 29 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
>>>
>>>> I am more worried about variable length part pushing the information
>>>> that is given later out to the right, e.g. "error: missing file '%s'
>>>> prevents us from doing X".  Chomping to [1024] is not a good
>>>> strategy for that kind of message; abbreviating %s to /path/name/...
>>>> (again, with literally "...") would be.
> 
> I have this one in my pile of Undecided topics:
> 
>     * jn/doc-api-errors (2014-12-04) 1 commit
>      - doc: document error handling functions and conventions
> 
>      For discussion.
>      What's the status of this one????
> 
> I think we all agree that the early part of the new documentation
> text is good, but the last section that proposes to store more
> detailed errors in caller supplied strbuf in textual form was
> controversial (and I have not convinced myself it is a good idea
> yet).
> 
> I could chuck the last section and then start merging the remainder
> to 'next' to salvage the "obviously good bits".  Or do people want
> to hash its last section a bit more?

Whether or not we decide on a different error-handling convention in the future, it is a fact of life that a good bit of code already uses the "strbuf" convention documented by Jonathan's patch. So I think it is OK to merge it as is. If we change the preferred convention in the future, one part of the change will be to update this file.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Junio C Hamano· Feb 17, 2015, 16:03 UTC · re: Michael Haggerty · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 15 quoted lines
>> I think we all agree that the early part of the new documentation
>> text is good, but the last section that proposes to store more
>> detailed errors in caller supplied strbuf in textual form was
>> controversial (and I have not convinced myself it is a good idea
>> yet).
>> 
>> I could chuck the last section and then start merging the remainder
>> to 'next' to salvage the "obviously good bits".  Or do people want
>> to hash its last section a bit more?
>
> Whether or not we decide on a different error-handling convention in the
> future, it is a fact of life that a good bit of code already uses the
> "strbuf" convention documented by Jonathan's patch. So I think it is OK
> to merge it as is. If we change the preferred convention in the future,
> one part of the change will be to update this file.

I wasn't sure about "a good bit of code already", but that settles it. Let's take it as-is and see how the code evolves.

Thanks.
Jeff King· Feb 17, 2015, 16:05 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> > Whether or not we decide on a different error-handling convention in the
> > future, it is a fact of life that a good bit of code already uses the
> > "strbuf" convention documented by Jonathan's patch. So I think it is OK
> > to merge it as is. If we change the preferred convention in the future,
> > one part of the change will be to update this file.
> 
> I wasn't sure about "a good bit of code already", but that settles
> it.  Let's take it as-is and see how the code evolves.

I'm not convinced myself after a quick grep. But it's certainly non-zero, and I think I would rather have the technique documented than not, so I withdraw my earlier complaints.

-Peff
Junio C Hamano· Feb 17, 2015, 22:46 UTC · re: Jeff King · lore

Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:
>
>> > Whether or not we decide on a different error-handling convention in the
>> > future, it is a fact of life that a good bit of code already uses the
>> > "strbuf" convention documented by Jonathan's patch. So I think it is OK
>> > to merge it as is. If we change the preferred convention in the future,
>> > one part of the change will be to update this file.
>> 
>> I wasn't sure about "a good bit of code already", but that settles
>> it.  Let's take it as-is and see how the code evolves.
>
> I'm not convinced myself after a quick grep. But it's certainly
> non-zero, and I think I would rather have the technique documented than
> not, so I withdraw my earlier complaints.
OK.
Junio C Hamano· Dec 3, 2014, 20:02 UTC · re: Jonathan Nieder · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 15 quoted lines
> This way, callers can put the message in context or even avoid
> printing the message altogether.
>
> Currently hold_lock_file_for_append tries to save errno in order to
> produce a meaningful message about the failure and tries to print a
> second message using errno.  Unfortunately the errno it uses is not
> meaningful because copy_fd makes no effort to set a meaningful errno
> value.  This change is preparation for simplifying the
> hold_lock_file_for_append behavior.
>
> No user-visible change intended yet.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> The title feature.

By the way, this seems to address the same thing as sb/copy-fd-errno topic that has been cooking in 'pu'? Should we drop that other topic and use this one instead?

Jonathan Nieder· Dec 3, 2014, 20:18 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Junio C Hamano wrote:
> By the way, this seems to address the same thing as sb/copy-fd-errno
> topic that has been cooking in 'pu'?  Should we drop that other
> topic and use this one instead?
Yes, please.

I'll give it an hour or two to collect more comments and then send a reroll reflecting them.

Thanks, Jonathan

Junio C Hamano· Dec 3, 2014, 21:43 UTC · re: Jonathan Nieder · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> Junio C Hamano wrote:
>
>> By the way, this seems to address the same thing as sb/copy-fd-errno
>> topic that has been cooking in 'pu'?  Should we drop that other
>> topic and use this one instead?
>
> Yes, please.
OK.

It felt strange that two people in a same office stomping on each other's toes, which I interpreted as an apparent lack of communication, but perhaps there was an off-line communication between you two with an understanding that this will supersede the other one, which I probably failed to spot in the cover letter.

> I'll give it an hour or two to collect more comments and then send a
> reroll reflecting them.
Make that one revolution of earth, perhaps?
e.g. http://thread.gmane.org/gmane.comp.version-control.git/259831/focus=260349
Jonathan Nieder· Dec 3, 2014, 21:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

Junio C Hamano wrote:
> Jonathan Nieder writes:
>> Junio C Hamano wrote:
Show 10 quoted lines
>>> By the way, this seems to address the same thing as sb/copy-fd-errno
>>> topic that has been cooking in 'pu'?  Should we drop that other
>>> topic and use this one instead?
>>
>> Yes, please.
>
> OK.
>
> It felt strange that two people in a same office stomping on each
> other's toes
I just did a bad job of explaining the context in the cover letter.

I wrote this series at Stefan's request (see the message that the cover letter is a response to for context[1]).

Hope that helps, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259710 and http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259703

Stefan Beller· Dec 3, 2014, 20:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf

On Wed, Dec 3, 2014 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> This way, callers can put the message in context or even avoid
>> printing the message altogether.
>>
>> Currently hold_lock_file_for_append tries to save errno in order to
>> produce a meaningful message about the failure and tries to print a
>> second message using errno.  Unfortunately the errno it uses is not
>> meaningful because copy_fd makes no effort to set a meaningful errno
>> value.  This change is preparation for simplifying the
>> hold_lock_file_for_append behavior.
>>
>> No user-visible change intended yet.
>>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>> The title feature.
>
> By the way, this seems to address the same thing as sb/copy-fd-errno
> topic that has been cooking in 'pu'?  Should we drop that other
> topic and use this one instead?
>

This series makes the code more readable and maintainable in the end as we don't need to hunt down the path of when the errno is changed or accessed. The currently cooking sb/copy-fd-errno is more of a hot fix, while this series is fixing the problem more at the basic level and more in long term.

I'd be happy if this replaces the currently cooking version.
Jonathan Nieder· Dec 3, 2014, 05:14 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to 3 files +18 −21

lockfile.c, lockfile.h, sha1_file.c

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)
Torsten Bögershausen· Dec 3, 2014, 06:09 UTC · re: Jonathan Nieder · lore

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

On 12/03/2014 06:14 AM, Jonathan Nieder wrote:
Show 37 quoted lines
> 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);
>

What do I miss ? In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set. Now we die() in an assert() or two ? And should'nt we assert in strbuf_addf() instead ?

And in general, does the commit message deserve an update?
>Luckily the only caller uses flags == LOCK_DIE_ON_ERROR,

The first impression was: Why do we not simplify the code and remove the flag completely ? This feels somewhat like maintaining dead code, which may be used later. (Unless it will be used in later commit, and the we could mention it here)

Jonathan Nieder· Dec 3, 2014, 07:04 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen wrote:
> On 12/03/2014 06:14 AM, Jonathan Nieder wrote:
Show 17 quoted lines
>> --- 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);
>
> What do I miss ?
> In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.

The assertion documents an assumption that no caller will set the LOCK_DIE_ON_ERROR flag bit. A later patch in the series eliminates that flag bit completely.

> Now we die() in an assert() or two ?

assert() is not safe to use for anything other than documenting assumptions. It can't be relied on to exit (let alone to exit with a nice message like die()). So the die() that was removed and assert() that this patch adds have different purposes.

> And should'nt we assert in  strbuf_addf() instead ?
strbuf_addf can be used to append to a nonempty strbuf.
[...]
> (Unless it will be used in later commit, and the we could mention it here)
Good idea.  I'll add to the commit message if rerolling.

Thanks, Jonathan

Jonathan Nieder· Dec 3, 2014, 05:16 UTC · re: Jonathan Nieder · lore

[PATCH 05/14] lock_packed_refs: pass error message back through a strbuf

This saves us from having to be careful about preserving errno and makes it more explicit in the die-on-error case that the caller is exiting without a chance to clean up.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/clone.c |  6 +++++-
 refs.c          | 17 ++++++++++-------
 refs.h          |  8 ++------
 3 files changed, 17 insertions(+), 14 deletions(-)
Show changes to 3 files +17 −14

builtin/clone.c, refs.c, refs.h

diff --git a/builtin/clone.c b/builtin/clone.c
index d5e7532..b07d740 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -495,8 +495,10 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
 static void write_remote_refs(const struct ref *local_refs)
 {
 	const struct ref *r;
+	struct strbuf err = STRBUF_INIT;
 
-	lock_packed_refs(LOCK_DIE_ON_ERROR);
+	if (lock_packed_refs(&err))
+		die("%s", err.buf);
 
 	for (r = local_refs; r; r = r->next) {
 		if (!r->peer_ref)
@@ -506,6 +508,8 @@ static void write_remote_refs(const struct ref *local_refs)
 
 	if (commit_packed_refs())
 		die_errno("unable to overwrite old ref-pack file");
+
+	strbuf_release(&err);
 }
 
 static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index 5ff457e..917f8fc 100644
--- a/refs.c
+++ b/refs.c
@@ -2371,13 +2371,15 @@ 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)
+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) {
+		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,
@@ -2565,11 +2567,13 @@ 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,
@@ -2579,6 +2583,7 @@ int pack_refs(unsigned int flags)
 		die_errno("unable to overwrite old ref-pack file");
 
 	prune_refs(cbdata.ref_to_prune);
+	strbuf_release(&err);
 	return 0;
 }
 
@@ -2657,10 +2662,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	if (i == n)
 		return 0; /* no refname exists in packed refs */
 
-	if (lock_packed_refs(0)) {
-		unable_to_lock_message(git_path("packed-refs"), errno, err);
+	if (lock_packed_refs(err))
 		return -1;
-	}
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
diff --git a/refs.h b/refs.h
index 2bc3556..ca6a567 100644
--- a/refs.h
+++ b/refs.h
@@ -119,12 +119,8 @@ extern int for_each_rawref(each_ref_fn, void *);
 extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);
 extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);
 
-/*
- * 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);
+/* Lock the packed-refs file for writing.  Returns 0 on success. */
+extern int lock_packed_refs(struct strbuf *err);
 
 /*
  * Add a reference to the in-memory packed reference cache.  This may

-- 
2.2.0.rc0.207.ga3a616c
Jonathan Nieder· Dec 3, 2014, 05:19 UTC · re: Jonathan Nieder · lore

[PATCH 06/14] lockfile: introduce flag for locks outside .git

When the lockfile API was introduced, it was only used for the index file and error messages like

  fatal: Unable to create '/path/to/foo.lock': File exists.
  If no other git process is currently running, this probably means a
  git process crashed in this repository earlier. Make sure no other git
  process is running and remove the file manually to continue.
were appropriate advice for that case.

Nowadays, the lockfile API is used for other files that need similar lock-then-update semantics, including files not associated to any repository, such as /etc/gitconfig, ../my.bundle, and /tmp/temp.marks. Add a flag that makes the message stop referring to "this repository" for such cases.

This should make it possible to print nicer error messages from such non-core users of the lockfile API.

This introduces the flag.  Later patches will use it.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-lockfile.txt | 12 ++++++++++++
 builtin/update-index.c                   |  2 +-
 lockfile.c                               | 26 +++++++++++++++++---------
 lockfile.h                               |  5 +++--
 refs.c                                   |  4 ++--
 5 files changed, 35 insertions(+), 14 deletions(-)
Show changes to 5 files +35 −14

Documentation/technical/api-lockfile.txt, builtin/update-index.c, lockfile.c, lockfile.h, refs.c

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 93b5f23..fa60cfd 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -124,6 +124,18 @@ LOCK_NO_DEREF::
 	for backwards-compatibility reasons can be a symbolic link
 	containing the name of the referred-to-reference.
 
+LOCK_OUTSIDE_REPOSITORY:
+
+	When the ".lock" file being created already exists, the error
+	message usually explains that another git process must have
+	crashed in the same Git repository.  If `LOCK_OUTSIDE_REPOSITORY`
+	is set, then the message is replaced with something more
+	appropriate when updating files that are not inside a
+	repository.
++
+For example, this flag should be set when locking a configuration
+file in the user's home directory.
+
 LOCK_DIE_ON_ERROR::
 
 	If a lock is already taken for the file, `die()` with an error
diff --git a/builtin/update-index.c b/builtin/update-index.c
index b0e3dc9..56abd18 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -943,7 +943,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
-			unable_to_lock_die(get_index_file(), lock_error);
+			unable_to_lock_die(get_index_file(), 0, lock_error);
 		}
 		if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 			die("Unable to write new index file");
diff --git a/lockfile.c b/lockfile.c
index 8685c68..102584f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -149,24 +149,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	return lk->fd;
 }
 
-void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
+void unable_to_lock_message(const char *path, int flags, int err,
+			    struct strbuf *buf)
 {
-	if (err == EEXIST) {
+	if (err != EEXIST) {
+		strbuf_addf(buf, "Unable to create '%s.lock': %s",
+			    absolute_path(path), strerror(err));
+	} else if (flags & LOCK_OUTSIDE_REPOSITORY) {
+		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
+		    "If no other git process is currently running, this probably means\n"
+		    "another git process crashed earlier. Make sure no other git process\n"
+		    "is running and remove the file manually to continue.",
+			    absolute_path(path), strerror(err));
+	} else {
 		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",
-			    absolute_path(path), strerror(err));
+	}
 }
 
-NORETURN void unable_to_lock_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int flags, int err)
 {
 	struct strbuf buf = STRBUF_INIT;
 
-	unable_to_lock_message(path, err, &buf);
+	unable_to_lock_message(path, flags, err, &buf);
 	die("%s", buf.buf);
 }
 
@@ -175,7 +183,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
 	int fd = lock_file(lk, path, flags);
 	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		unable_to_lock_die(path, errno);
+		unable_to_lock_die(path, flags, errno);
 	return fd;
 }
 
@@ -189,7 +197,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,
 
 	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
-		unable_to_lock_message(path, errno, err);
+		unable_to_lock_message(path, flags, errno, err);
 		return fd;
 	}
 
diff --git a/lockfile.h b/lockfile.h
index ca36a1d..779a992 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -70,10 +70,11 @@ struct lock_file {
 
 #define LOCK_DIE_ON_ERROR 1
 #define LOCK_NO_DEREF 2
+#define LOCK_OUTSIDE_REPOSITORY 4
 
-extern void unable_to_lock_message(const char *path, int err,
+extern void unable_to_lock_message(const char *path, int, int err,
 				   struct strbuf *buf);
-extern NORETURN void unable_to_lock_die(const char *path, int err);
+extern NORETURN void unable_to_lock_die(const char *path, int, 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, struct strbuf *err);
diff --git a/refs.c b/refs.c
index 917f8fc..39e43cf 100644
--- a/refs.c
+++ b/refs.c
@@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else
-			unable_to_lock_die(ref_file, errno);
+			unable_to_lock_die(ref_file, lflags, errno);
 	}
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
@@ -2376,7 +2376,7 @@ 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"), 0) < 0) {
-		unable_to_lock_message(git_path("packed-refs"), errno, err);
+		unable_to_lock_message(git_path("packed-refs"), 0, errno, err);
 		return -1;
 	}
Junio C Hamano· Dec 3, 2014, 23:13 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 11 quoted lines
> diff --git a/refs.c b/refs.c
> index 917f8fc..39e43cf 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  			 */
>  			goto retry;
>  		else
> -			unable_to_lock_die(ref_file, errno);
> +			unable_to_lock_die(ref_file, lflags, errno);
>  	}

This has unfortunate interaction with 06839515 (lock_ref_sha1_basic: do not die on locking errors, 2014-11-19). The fact that the helper unable-to-lock-message() is now hidden inside lockfile.c does not help, either.

Jonathan Nieder· Dec 3, 2014, 23:24 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 14 quoted lines
>> diff --git a/refs.c b/refs.c
>> index 917f8fc..39e43cf 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>  			 */
>>  			goto retry;
>>  		else
>> -			unable_to_lock_die(ref_file, errno);
>> +			unable_to_lock_die(ref_file, lflags, errno);
>>  	}
>
> This has unfortunate interaction with 06839515 (lock_ref_sha1_basic:
> do not die on locking errors, 2014-11-19).

I'll add that patch to the series. 06839515 becomes a little simpler with the updated API.

Thanks, Jonathan

Junio C Hamano· Dec 3, 2014, 23:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> diff --git a/refs.c b/refs.c
>> index 917f8fc..39e43cf 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>  			 */
>>  			goto retry;
>>  		else
>> -			unable_to_lock_die(ref_file, errno);
>> +			unable_to_lock_die(ref_file, lflags, errno);
>>  	}
>
> This has unfortunate interaction with 06839515 (lock_ref_sha1_basic:
> do not die on locking errors, 2014-11-19).  The fact that the helper
> unable-to-lock-message() is now hidden inside lockfile.c does not
> help, either.

I tried to merge the 14-patch series with obvious fix-ups after dropping the rerere abortion change you sent separately and in duplicate and also dropping sb/copy-fd, but I've ran out of patience with this step, at least for today's integration cycle. Should we also drop jk/lock-ref-sha1-basec-return-errors topic as well?

The 14-patch series may have been internally consistent and its individual patches, when each of them was taken alone by itself, may have made sense, but it appears that the aggregated whole these separate topics took their root from is inconsistent with itself in minor ways like this here and there X-<.

Jonathan Nieder· Dec 3, 2014, 23:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano wrote:
Show 5 quoted lines
> I tried to merge the 14-patch series with obvious fix-ups after
> dropping the rerere abortion change you sent separately and in
> duplicate and also dropping sb/copy-fd, but I've ran out of patience
> with this step, at least for today's integration cycle.  Should we
> also drop jk/lock-ref-sha1-basec-return-errors topic as well?

I don't mind adding it to the series. Or feel free to hold off on picking up this 14-patch series until tomorrow's reroll. (Helping with today's integration was why I was thinking of re-sending. I should have been clearer about saying that I prefer you don't pick this up yet when I decided to give reviewers a little more time.)

Show 5 quoted lines
> The 14-patch series may have been internally consistent and its
> individual patches, when each of them was taken alone by itself, may
> have made sense, but it appears that the aggregated whole these
> separate topics took their root from is inconsistent with itself
> in minor ways like this here and there X-<.

I don't follow. It's normal for an API change to affect code that uses the API --- what inconsistency are you talking about here?

Puzzled, Jonathan

Junio C Hamano· Dec 3, 2014, 23:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
>> The 14-patch series may have been internally consistent and its
>> individual patches, when each of them was taken alone by itself, may
>> have made sense, but it appears that the aggregated whole these
>> separate topics took their root from is inconsistent with itself
>> in minor ways like this here and there X-<.
>
> I don't follow.  It's normal for an API change to affect code that
> uses the API --- what inconsistency are you talking about here?

I was under the impression that the purpose of the series was to propose an API update to be used together with the remainder of the system, not just "update code in master, breaking unstated set of topics and leaving them behind without updating them for now".

Such a series is perfectly fine as a feeler to see if people are happy with the updated API that it does not completely cover the topics in flight, but I wouldn't have had to waste time trying to figure out what you are doing differently to cause other topics in flight to break if such a feeler series were labeled clearly as RFC/PATCH, and/or "these other topics need to be adjusted to this new API when this series settles", or somesuch.

Jonathan Nieder· Dec 3, 2014, 23:41 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano wrote:
> I was under the impression that the purpose of the series was to
> propose an API update to be used together with the remainder of the
> system, not just "update code in master, breaking unstated set of
> topics and leaving them behind without updating them for now".

Got it --- before the reroll I will try a test-merge against 'next' and 'pu' (and likewise when sending future patches).

Junio C Hamano· Dec 3, 2014, 23:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> The 14-patch series may have been internally consistent and its
>>> individual patches, when each of them was taken alone by itself, may
>>> have made sense, but it appears that the aggregated whole these
>>> separate topics took their root from is inconsistent with itself
>>> in minor ways like this here and there X-<.
>>
>> I don't follow.  It's normal for an API change to affect code that
>> uses the API --- what inconsistency are you talking about here?
>
> I was under the impression that the purpose of the series was to
> propose an API update to be used together with the remainder of the
> system, not just "update code in master, breaking unstated set of
> topics and leaving them behind without updating them for now".
> ...
About the "inconsistent with itself" bit.

The other topic is a piece from the long-ish series by Ronnie, and somehow I had assumed this 14-patch series also originated from another part of that series, especially given that it is largely about refs. That's where "itself" came from.

Looking at it again, this errno thing is more or less separate and independent, so your "I don't follow" is perfectly understandable to me now. Sorry about being grumpy.

Jeff King· Dec 3, 2014, 23:57 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

On Wed, Dec 03, 2014 at 03:29:51PM -0800, Jonathan Nieder wrote:
Show 9 quoted lines
> Junio C Hamano wrote:
> 
> > I tried to merge the 14-patch series with obvious fix-ups after
> > dropping the rerere abortion change you sent separately and in
> > duplicate and also dropping sb/copy-fd, but I've ran out of patience
> > with this step, at least for today's integration cycle.  Should we
> > also drop jk/lock-ref-sha1-basec-return-errors topic as well?
> 
> I don't mind adding it to the series.

Please do add it in, rather than dropping it. It fixes a real racy condition in which "pack-refs --prune" wants to lock a ref only to prune it, and it's OK to continue on if the lock fails (without that patch, pack-refs dies prematurely).

-Peff
Junio C Hamano· Dec 4, 2014, 05:51 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted lines
> Junio C Hamano wrote:
>
>> I tried to merge the 14-patch series with obvious fix-ups after
>> dropping the rerere abortion change you sent separately and in
>> duplicate and also dropping sb/copy-fd, but I've ran out of patience
>> with this step, at least for today's integration cycle.  Should we
>> also drop jk/lock-ref-sha1-basec-return-errors topic as well?
>
> I don't mind adding it to the series....

We may want to see it done the other way around, though. That is, to allow the pack-refs race fix, which was reviewed and has been cooking, graduate without having to depend on this API rewrite, so that we may be able to merge it down even to v2.2.x maintenance track.

Thanks.
Jonathan Nieder· Dec 4, 2014, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git

Junio C Hamano wrote:
Show 5 quoted lines
> We may want to see it done the other way around, though.  That is,
> to allow the pack-refs race fix, which was reviewed and has been
> cooking, graduate without having to depend on this API rewrite, so
> that we may be able to merge it down even to v2.2.x maintenance
> track.

Good idea. When I send out the reroll of this API change today, it will be based on top of master + that patch.

Thanks, Jonathan

Jonathan Nieder· Dec 3, 2014, 05:19 UTC · re: Jonathan Nieder · lore

[PATCH 07/14] fast-import: use message from lockfile API when writing marks fails

The usual way to handle errors from hold_lock_file_for_update is to get a message from unable_to_lock_message or unable_to_lock_die, which can explain to the user how to check for other git processes running concurrently and unlink the .lock file if safe.

fast-import didn't use the unable_to_lock_message helper because at the time it started using the lockfile API (v1.5.1-rc1~69^2~2, 2007-03-07) that helper didn't exist. Later it still was not appropriate to use that helper because the message assumed the file being locked was inside a git repository. Now there is a flag to indicate that this lockfile is not part of the repository, so we can finally use the helper.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
Show changes to fast-import.c +8 −3
diff --git a/fast-import.c b/fast-import.c
index d0bd285..bf8faa9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1799,9 +1799,14 @@ static void dump_marks(void)
 	if (!export_marks_file)
 		return;
 
-	if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {
-		failure |= error("Unable to write marks file %s: %s",
-			export_marks_file, strerror(errno));
+	if (hold_lock_file_for_update(&mark_lock, export_marks_file,
+				      LOCK_OUTSIDE_REPOSITORY) < 0) {
+		struct strbuf err = STRBUF_INIT;
+
+		unable_to_lock_message(export_marks_file,
+				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
+		failure |= error("%s", err.buf);
+		strbuf_release(&err);
 		return;
 	}
Jonathan Nieder· Dec 3, 2014, 05:20 UTC · re: Jonathan Nieder · lore

[PATCH 08/14] credentials: use message from lockfile API when locking ~/.git-credentials fails

If writing $HOME/.git-credentials.lock (or locking another file specified with the --file option) fails due to the lock file already existing, chances are that it is because another git process already locked the file. Use the message from the lockfile API that says so, to help the user to look for running git processes and remove the stray .lock file when it is safe.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 credential-store.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to credential-store.c +2 −2
diff --git a/credential-store.c b/credential-store.c
index d435514..cd71156 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -55,8 +55,8 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
-	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
-		die_errno("unable to get credential storage lock");
+	hold_lock_file_for_update(&credential_lock, fn,
+				  LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);
 	if (extra)
 		print_line(extra);
 	parse_credential_file(fn, c, NULL, print_line);
Jonathan Nieder· Dec 3, 2014, 05:21 UTC · re: Jonathan Nieder · lore

[PATCH 09/14] config: use message from lockfile API when locking config file fails

The message from the lockfile API includes advice about how to remove a stale lock file after a previous git process crashed.

Pass the LOCK_OUTSIDE_REPOSITORY flag to avoid confusing error messages that imply the lockfile is under .git. These functions (and 'git config --file') are useful for arbitrary files in git config format, including both files outside .git such as /etc/gitconfig and $XDG_CONFIG_HOME/git/config and files under .git such as $GIT_DIR/config.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 config.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
Show changes to config.c +16 −4
diff --git a/config.c b/config.c
index 15a2983..dacde5f 100644
--- a/config.c
+++ b/config.c
@@ -1940,9 +1940,15 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	 * contents of .git/config will be written into it.
 	 */
 	lock = xcalloc(1, sizeof(struct lock_file));
-	fd = hold_lock_file_for_update(lock, config_filename, 0);
+	fd = hold_lock_file_for_update(lock, config_filename,
+				       LOCK_OUTSIDE_REPOSITORY);
 	if (fd < 0) {
-		error("could not lock config file %s: %s", config_filename, strerror(errno));
+		struct strbuf err = STRBUF_INIT;
+
+		unable_to_lock_message(config_filename,
+				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
+		error("%s", err.buf);
+		strbuf_release(&err);
 		free(store.key);
 		ret = CONFIG_NO_LOCK;
 		goto out_free;
@@ -2211,9 +2217,15 @@ int git_config_rename_section_in_file(const char *config_filename,
 		config_filename = filename_buf = git_pathdup("config");
 
 	lock = xcalloc(1, sizeof(struct lock_file));
-	out_fd = hold_lock_file_for_update(lock, config_filename, 0);
+	out_fd = hold_lock_file_for_update(lock, config_filename,
+					   LOCK_OUTSIDE_REPOSITORY);
 	if (out_fd < 0) {
-		ret = error("could not lock config file %s", config_filename);
+		struct strbuf err = STRBUF_INIT;
+
+		unable_to_lock_message(config_filename,
+				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
+		ret = error("%s", err.buf);
+		strbuf_release(&err);
 		goto out;
 	}
Junio C Hamano· Dec 3, 2014, 19:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH 09/14] config: use message from lockfile API when locking config file fails

Up to this point the patches looked sensible (I am not saying that the remainder is junk---I haven't looked at them yet is all I am saying).

Thanks.
Jonathan Nieder· Dec 3, 2014, 20:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 09/14] config: use message from lockfile API when locking config file fails

Junio C Hamano wrote:
> Up to this point the patches looked sensible (I am not saying that
> the remainder is junk---I haven't looked at them yet is all I am
> saying).
Nice to hear.  Thanks for looking it over.
Jonathan
Jonathan Nieder· Dec 3, 2014, 05:22 UTC · re: Jonathan Nieder · lore

[PATCH 10/14] rerere: error out on autoupdate failure

We have been silently tolerating errors by returning early with an error that the caller ignores since rerere.autoupdate was introduced in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the index is already locked), rerere can return success silently without updating the index or with only some items in the index updated.

Better to treat such failures as a fatal error so the operator can figure out what is wrong and fix it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Also sent separately at
http://thread.gmane.org/gmane.comp.version-control.git/260623
 rerere.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
Show changes to rerere.c +5 −9
diff --git a/rerere.c b/rerere.c
index 1b0555f..195f663 100644
--- a/rerere.c
+++ b/rerere.c
@@ -477,27 +477,23 @@ out:
 
 static struct lock_file index_lock;
 
-static int update_paths(struct string_list *update)
+static void update_paths(struct string_list *update)
 {
 	int i;
-	int fd = hold_locked_index(&index_lock, 0);
-	int status = 0;
 
-	if (fd < 0)
-		return -1;
+	hold_locked_index(&index_lock, 1);
 
 	for (i = 0; i < update->nr; i++) {
 		struct string_list_item *item = &update->items[i];
 		if (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))
-			status = -1;
+			die("staging updated %s failed", item->string);
 	}
 
-	if (!status && active_cache_changed) {
+	if (active_cache_changed) {
 		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 			die("Unable to write new index file");
-	} else if (fd >= 0)
+	} else
 		rollback_lock_file(&index_lock);
-	return status;
 }
 
 static int do_plain_rerere(struct string_list *rr, int fd)
Jonathan Nieder· Dec 3, 2014, 05:25 UTC · re: Jonathan Nieder · lore

[PATCH 11/14] hold_locked_index: pass error message back through a strbuf

Today hold_locked_index takes a boolean parameter indicating whether to die with a message or to return -1 with errno indicating the nature of the problem on error.

Pass back an error message through an 'err' parameter instead. This method of error reporting was introduced in the ref transaction API and makes it more obvious when callers are not reporting errors (for example, this helped catch rerere.autoupdate skipping the autoupdate when unable to lock the index).

It also makes it easier for callers to clean up before exiting instead of having to die() right away and paves the way for simplifying hold_lock_file_for_update error handling in a later patch.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c            | 12 ++++++++---
 builtin/apply.c          | 10 +++++++--
 builtin/checkout-index.c | 10 +++++++--
 builtin/checkout.c       | 55 ++++++++++++++++++++++++++++++++++--------------
 builtin/clone.c          |  6 +++++-
 builtin/commit.c         | 27 ++++++++++++++++++------
 builtin/describe.c       | 11 +++++++---
 builtin/diff.c           | 12 ++++++++---
 builtin/merge.c          | 14 +++++++++---
 builtin/mv.c             |  5 ++++-
 builtin/read-tree.c      |  9 ++++++--
 builtin/reset.c          | 10 +++++++--
 builtin/rm.c             |  9 ++++++--
 builtin/update-index.c   | 10 ++++-----
 cache-tree.c             | 18 ++++++++++++----
 cache.h                  |  2 +-
 merge-recursive.c        | 13 +++++++++---
 merge.c                  | 17 +++++++++++----
 read-cache.c             | 14 +++++++-----
 rerere.c                 |  5 ++++-
 sequencer.c              | 14 ++++++++++--
 test-scrap-cache-tree.c  |  5 ++++-
 22 files changed, 214 insertions(+), 74 deletions(-)
Show changes to 22 files +214 −74

builtin/add.c, builtin/apply.c, builtin/checkout-index.c, builtin/checkout.c, builtin/clone.c, builtin/commit.c, builtin/describe.c, builtin/diff.c, builtin/merge.c, builtin/mv.c, builtin/read-tree.c, builtin/reset.c, builtin/rm.c, builtin/update-index.c, cache-tree.c, cache.h, merge-recursive.c, merge.c, read-cache.c, rerere.c, sequencer.c, test-scrap-cache-tree.c

diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..e912d77 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -299,6 +299,7 @@ static int add_files(struct dir_struct *dir, int flags)
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
+	struct strbuf err = STRBUF_INIT;
 	struct pathspec pathspec;
 	struct dir_struct dir;
 	int flags;
@@ -315,8 +316,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (add_interactive)
 		exit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));
 
-	if (edit_interactive)
-		return(edit_patch(argc, argv, prefix));
+	if (edit_interactive) {
+		strbuf_release(&err);
+		return edit_patch(argc, argv, prefix);
+	}
 	argc--;
 	argv++;
 
@@ -344,7 +347,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !take_worktree_changes;
 
-	hold_locked_index(&lock_file, 1);
+	if (hold_locked_index(&lock_file, &err) < 0)
+		die("%s", err.buf);
 
 	flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
 		 (show_only ? ADD_CACHE_PRETEND : 0) |
@@ -356,6 +360,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
 		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		strbuf_release(&err);
 		return 0;
 	}
 
@@ -446,5 +451,6 @@ finish:
 			die(_("Unable to write new index file"));
 	}
 
+	strbuf_release(&err);
 	return exit_status;
 }
diff --git a/builtin/apply.c b/builtin/apply.c
index 6696ea4..cda438f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4200,6 +4200,7 @@ static int apply_patch(int fd, const char *filename, int options)
 {
 	size_t offset;
 	struct strbuf buf = STRBUF_INIT; /* owns the patch text */
+	struct strbuf err = STRBUF_INIT;
 	struct patch *list = NULL, **listp = &list;
 	int skipped_patch = 0;
 
@@ -4237,8 +4238,11 @@ static int apply_patch(int fd, const char *filename, int options)
 		apply = 0;
 
 	update_index = check_index && apply;
-	if (update_index && newfd < 0)
-		newfd = hold_locked_index(&lock_file, 1);
+	if (update_index && newfd < 0) {
+		newfd = hold_locked_index(&lock_file, &err);
+		if (newfd < 0)
+			die("%s", err.buf);
+	}
 
 	if (check_index) {
 		if (read_cache() < 0)
@@ -4254,6 +4258,7 @@ static int apply_patch(int fd, const char *filename, int options)
 		if (apply_with_reject)
 			exit(1);
 		/* with --3way, we still need to write the index out */
+		strbuf_release(&err);
 		return 1;
 	}
 
@@ -4272,6 +4277,7 @@ static int apply_patch(int fd, const char *filename, int options)
 	free_patch_list(list);
 	strbuf_release(&buf);
 	string_list_clear(&fn_table, 0);
+	strbuf_release(&err);
 	return 0;
 }
 
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 383dccf..07164f5 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -133,11 +133,17 @@ static int option_parse_u(const struct option *opt,
 			      const char *arg, int unset)
 {
 	int *newfd = opt->value;
+	struct strbuf err = STRBUF_INIT;
 
 	state.refresh_cache = 1;
 	state.istate = &the_index;
-	if (*newfd < 0)
-		*newfd = hold_locked_index(&lock_file, 1);
+	if (*newfd < 0) {
+		int fd = hold_locked_index(&lock_file, &err);
+		if (fd < 0)
+			die("%s", err.buf);
+		*newfd = fd;
+	}
+	strbuf_release(&err);
 	return 0;
 }
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5410dac..d2699cd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -218,6 +218,7 @@ static int checkout_merged(int pos, struct checkout *state)
 static int checkout_paths(const struct checkout_opts *opts,
 			  const char *revision)
 {
+	struct strbuf err = STRBUF_INIT;
 	int pos;
 	struct checkout state;
 	static char *ps_matched;
@@ -249,15 +250,20 @@ static int checkout_paths(const struct checkout_opts *opts,
 		die(_("Cannot update paths and switch to branch '%s' at the same time."),
 		    opts->new_branch);
 
-	if (opts->patch_mode)
+	if (opts->patch_mode) {
+		strbuf_release(&err);
 		return run_add_interactive(revision, "--patch=checkout",
 					   &opts->pathspec);
+	}
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	hold_locked_index(lock_file, 1);
-	if (read_cache_preload(&opts->pathspec) < 0)
+	if (hold_locked_index(lock_file, &err) < 0)
+		die("%s", err.buf);
+	if (read_cache_preload(&opts->pathspec) < 0) {
+		strbuf_release(&err);
 		return error(_("corrupt index file"));
+	}
 
 	if (opts->source_tree)
 		read_tree_some(opts->source_tree, &opts->pathspec);
@@ -302,6 +308,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 
 	if (report_path_error(ps_matched, &opts->pathspec, opts->prefix)) {
 		free(ps_matched);
+		strbuf_release(&err);
 		return 1;
 	}
 	free(ps_matched);
@@ -329,8 +336,10 @@ static int checkout_paths(const struct checkout_opts *opts,
 			pos = skip_same_name(ce, pos) - 1;
 		}
 	}
-	if (errs)
+	if (errs) {
+		strbuf_release(&err);
 		return 1;
+	}
 
 	/* Now we are committed to check them out */
 	memset(&state, 0, sizeof(state));
@@ -359,6 +368,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 	head = lookup_commit_reference_gently(rev, 1);
 
 	errs |= post_checkout_hook(head, head, 0);
+	strbuf_release(&err);
 	return errs;
 }
 
@@ -442,17 +452,22 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			      int *writeout_error)
 {
 	int ret;
+	struct strbuf err = STRBUF_INIT;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	hold_locked_index(lock_file, 1);
-	if (read_cache_preload(NULL) < 0)
-		return error(_("corrupt index file"));
+	if (hold_locked_index(lock_file, &err) < 0)
+		die("%s", err.buf);
+
+	if (read_cache_preload(NULL) < 0) {
+		ret = error(_("corrupt index file"));
+		goto done;
+	}
 
 	resolve_undo_clear();
 	if (opts->force) {
 		ret = reset_tree(new->commit->tree, opts, 1, writeout_error);
 		if (ret)
-			return ret;
+			goto done;
 	} else {
 		struct tree_desc trees[2];
 		struct tree *tree;
@@ -469,7 +484,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
 
 		if (unmerged_cache()) {
 			error(_("you need to resolve your current index first"));
-			return 1;
+			ret = 1;
+			goto done;
 		}
 
 		/* 2-way merge to the new branch */
@@ -501,15 +517,19 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			struct tree *result;
 			struct tree *work;
 			struct merge_options o;
-			if (!opts->merge)
-				return 1;
+			if (!opts->merge) {
+				ret = 1;
+				goto done;
+			}
 
 			/*
 			 * Without old->commit, the below is the same as
 			 * the two-tree unpack we already tried and failed.
 			 */
-			if (!old->commit)
-				return 1;
+			if (!old->commit) {
+				ret = 1;
+				goto done;
+			}
 
 			/* Do more real merge */
 
@@ -539,7 +559,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			ret = reset_tree(new->commit->tree, opts, 1,
 					 writeout_error);
 			if (ret)
-				return ret;
+				goto done;
 			o.ancestor = old->name;
 			o.branch1 = new->name;
 			o.branch2 = "local";
@@ -548,7 +568,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			ret = reset_tree(new->commit->tree, opts, 0,
 					 writeout_error);
 			if (ret)
-				return ret;
+				goto done;
 		}
 	}
 
@@ -564,7 +584,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
 	if (!opts->force && !opts->quiet)
 		show_local_changes(&new->commit->object, &opts->diff_options);
 
-	return 0;
+	ret = 0;
+done:
+	strbuf_release(&err);
+	return ret;
 }
 
 static void report_tracking(struct branch_info *new)
diff --git a/builtin/clone.c b/builtin/clone.c
index b07d740..911a6a4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -613,6 +613,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int checkout(void)
 {
+	struct strbuf msg = STRBUF_INIT;
 	unsigned char sha1[20];
 	char *head;
 	struct lock_file *lock_file;
@@ -628,6 +629,7 @@ static int checkout(void)
 	if (!head) {
 		warning(_("remote HEAD refers to nonexistent ref, "
 			  "unable to checkout.\n"));
+		strbuf_release(&msg);
 		return 0;
 	}
 	if (!strcmp(head, "HEAD")) {
@@ -643,7 +645,8 @@ static int checkout(void)
 	setup_work_tree();
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
-	hold_locked_index(lock_file, 1);
+	if (hold_locked_index(lock_file, &msg) < 0)
+		die("%s", msg.buf);
 
 	memset(&opts, 0, sizeof opts);
 	opts.update = 1;
@@ -668,6 +671,7 @@ static int checkout(void)
 	if (!err && option_recursive)
 		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
 
+	strbuf_release(&msg);
 	return err;
 }
 
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..edc4493 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -319,6 +319,7 @@ static void refresh_cache_or_die(int refresh_flags)
 static const char *prepare_index(int argc, const char **argv, const char *prefix,
 				 const struct commit *current_head, int is_status)
 {
+	struct strbuf err = STRBUF_INIT;
 	struct string_list partial;
 	struct pathspec pathspec;
 	int refresh_flags = REFRESH_QUIET;
@@ -334,8 +335,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 
 	if (interactive) {
 		char *old_index_env = NULL;
-		hold_locked_index(&index_lock, 1);
 
+		if (hold_locked_index(&index_lock, &err) < 0)
+			die("%s", err.buf);
 		refresh_cache_or_die(refresh_flags);
 
 		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
@@ -363,6 +365,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			warning(_("Failed to update main cache tree"));
 
 		commit_style = COMMIT_NORMAL;
+		strbuf_release(&err);
 		return index_lock.filename.buf;
 	}
 
@@ -379,13 +382,15 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * (B) on failure, rollback the real index.
 	 */
 	if (all || (also && pathspec.nr)) {
-		hold_locked_index(&index_lock, 1);
+		if (hold_locked_index(&index_lock, &err) < 0)
+			die("%s", err.buf);
 		add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
 		refresh_cache_or_die(refresh_flags);
 		update_main_cache_tree(WRITE_TREE_SILENT);
 		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
 			die(_("unable to write new_index file"));
 		commit_style = COMMIT_NORMAL;
+		strbuf_release(&err);
 		return index_lock.filename.buf;
 	}
 
@@ -399,7 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	 * We still need to refresh the index here.
 	 */
 	if (!only && !pathspec.nr) {
-		hold_locked_index(&index_lock, 1);
+		if (hold_locked_index(&index_lock, &err) < 0)
+			die("%s", err.buf);
 		refresh_cache_or_die(refresh_flags);
 		if (active_cache_changed
 		    || !cache_tree_fully_valid(active_cache_tree)) {
@@ -414,6 +420,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			rollback_lock_file(&index_lock);
 		}
 		commit_style = COMMIT_AS_IS;
+		strbuf_release(&err);
 		return get_index_file();
 	}
 
@@ -453,7 +460,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (read_cache() < 0)
 		die(_("cannot read the index"));
 
-	hold_locked_index(&index_lock, 1);
+	if (hold_locked_index(&index_lock, &err) < 0)
+		die("%s", err.buf);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 	update_main_cache_tree(WRITE_TREE_SILENT);
@@ -475,6 +483,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	discard_cache();
 	read_cache_from(false_lock.filename.buf);
 
+	strbuf_release(&err);
 	return false_lock.filename.buf;
 }
 
@@ -1356,8 +1365,8 @@ static int git_status_config(const char *k, const char *v, void *cb)
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
-	int fd;
 	unsigned char sha1[20];
+	struct strbuf err = STRBUF_INIT;
 	static struct option builtin_status_options[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT_SET_INT('s', "short", &status_format,
@@ -1405,8 +1414,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	read_cache_preload(&s.pathspec);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL);
 
-	fd = hold_locked_index(&index_lock, 0);
-	if (0 <= fd)
+	/* Refresh the index if possible. */
+	if (hold_locked_index(&index_lock, &err) < 0)
+		/* errors (e.g., read-only filesystem) are fine */
+		strbuf_reset(&err);
+	else
 		update_index_if_able(&the_index, &index_lock);
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
@@ -1433,6 +1445,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		wt_status_print(&s);
 		break;
 	}
+	strbuf_release(&err);
 	return 0;
 }
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 9103193..1e7e970 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -457,19 +457,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	if (argc == 0) {
 		if (dirty) {
+			struct strbuf err = STRBUF_INIT;
 			static struct lock_file index_lock;
-			int fd;
 
 			read_cache_preload(NULL);
 			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
 				      NULL, NULL, NULL);
-			fd = hold_locked_index(&index_lock, 0);
-			if (0 <= fd)
+
+			/* Refresh the index if possible. */
+			if (hold_locked_index(&index_lock, &err) < 0)
+				/* Failure is ok (e.g, read-only filesystem). */
+				strbuf_reset(&err);
+			else
 				update_index_if_able(&the_index, &index_lock);
 
 			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
 					    diff_index_args, prefix))
 				dirty = NULL;
+			strbuf_release(&err);
 		}
 		describe("HEAD", 1);
 	} else if (dirty) {
diff --git a/builtin/diff.c b/builtin/diff.c
index 4326fa5..9dd7b93 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -206,16 +206,22 @@ static int builtin_diff_combined(struct rev_info *revs,
 static void refresh_index_quietly(void)
 {
 	struct lock_file *lock_file;
-	int fd;
+	struct strbuf err = STRBUF_INIT;
 
 	lock_file = xcalloc(1, sizeof(struct lock_file));
-	fd = hold_locked_index(lock_file, 0);
-	if (fd < 0)
+	if (hold_locked_index(lock_file, &err) < 0) {
+		/*
+		 * Locking the index failed (e.g., read-only filesystem).
+		 * That's okay.
+		 */
+		strbuf_release(&err);
 		return;
+	}
 	discard_cache();
 	read_cache();
 	refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
 	update_index_if_able(&the_index, lock_file);
+	strbuf_release(&err);
 }
 
 static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
diff --git a/builtin/merge.c b/builtin/merge.c
index bebbe5b..1ecea33 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -658,12 +658,16 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			      struct commit *head, const char *head_arg)
 {
 	static struct lock_file lock;
+	struct strbuf err = STRBUF_INIT;
 
-	hold_locked_index(&lock, 1);
+	if (hold_locked_index(&lock, &err) < 0)
+		die("%s", err.buf);
 	refresh_cache(REFRESH_QUIET);
 	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	    write_locked_index(&the_index, &lock, COMMIT_LOCK)) {
+		strbuf_release(&err);
 		return error(_("Unable to write index."));
+	}
 	rollback_lock_file(&lock);
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
@@ -675,6 +679,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
 		if (remoteheads->next) {
 			error(_("Not handling anything other than two heads merge."));
+			strbuf_release(&err);
 			return 2;
 		}
 
@@ -696,15 +701,18 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		for (j = common; j; j = j->next)
 			commit_list_insert(j->item, &reversed);
 
-		hold_locked_index(&lock, 1);
+		if (hold_locked_index(&lock, &err) < 0)
+			die("%s", err.buf);
 		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
 		if (active_cache_changed &&
 		    write_locked_index(&the_index, &lock, COMMIT_LOCK))
 			die (_("unable to write %s"), get_index_file());
 		rollback_lock_file(&lock);
+		strbuf_release(&err);
 		return clean ? 0 : 1;
 	} else {
+		strbuf_release(&err);
 		return try_merge_command(strategy, xopts_nr, xopts,
 						common, head_arg, remoteheads);
 	}
diff --git a/builtin/mv.c b/builtin/mv.c
index 563d05b..e87817f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -116,6 +116,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+	struct strbuf err = STRBUF_INIT;
 
 	gitmodules_config();
 	git_config(git_default_config, NULL);
@@ -125,7 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (--argc < 1)
 		usage_with_options(builtin_mv_usage, builtin_mv_options);
 
-	hold_locked_index(&lock_file, 1);
+	if (hold_locked_index(&lock_file, &err) < 0)
+		die("%s", err.buf);
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
 
@@ -278,5 +280,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("Unable to write new index file"));
 
+	strbuf_release(&err);
 	return 0;
 }
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 43b47f7..2328b96 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -100,6 +100,7 @@ static struct lock_file lock_file;
 
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
+	struct strbuf err = STRBUF_INIT;
 	int i, stage = 0;
 	unsigned char sha1[20];
 	struct tree_desc t[MAX_UNPACK_TREES];
@@ -150,7 +151,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
-	hold_locked_index(&lock_file, 1);
+	if (hold_locked_index(&lock_file, &err) < 0)
+		die("%s", err.buf);
 
 	prefix_set = opts.prefix ? 1 : 0;
 	if (1 < opts.merge + opts.reset + prefix_set)
@@ -228,8 +230,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 		parse_tree(tree);
 		init_tree_desc(t+i, tree->buffer, tree->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
+	if (unpack_trees(nr_trees, t, &opts)) {
+		strbuf_release(&err);
 		return 128;
+	}
 
 	if (opts.debug_unpack || opts.dry_run)
 		return 0; /* do not write the index out */
@@ -245,5 +249,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die("unable to write new index file");
+	strbuf_release(&err);
 	return 0;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..f83df72 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -266,6 +266,7 @@ static int reset_refs(const char *rev, const unsigned char *sha1)
 
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
+	struct strbuf msg = STRBUF_INIT;
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
 	int patch_mode = 0, unborn;
 	const char *rev;
@@ -320,6 +321,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die(_("--patch is incompatible with --{hard,mixed,soft}"));
+		strbuf_release(&msg);
 		return run_add_interactive(rev, "--patch=reset", &pathspec);
 	}
 
@@ -354,11 +356,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	if (reset_type != SOFT) {
 		struct lock_file *lock = xcalloc(1, sizeof(*lock));
-		hold_locked_index(lock, 1);
+		if (hold_locked_index(lock, &msg) < 0)
+			die("%s", msg.buf);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-			if (read_from_tree(&pathspec, sha1, intent_to_add))
+			if (read_from_tree(&pathspec, sha1, intent_to_add)) {
+				strbuf_release(&msg);
 				return 1;
+			}
 			if (get_git_work_tree())
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
@@ -385,5 +390,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state();
 
+	strbuf_release(&msg);
 	return update_ref_status;
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index d8a9c86..d54538e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -281,6 +281,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	int i;
 	struct pathspec pathspec;
 	char *seen;
+	struct strbuf err = STRBUF_INIT;
 
 	gitmodules_config();
 	git_config(git_default_config, NULL);
@@ -293,7 +294,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (!index_only)
 		setup_work_tree();
 
-	hold_locked_index(&lock_file, 1);
+	if (hold_locked_index(&lock_file, &err) < 0)
+		die("%s", err.buf);
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
@@ -375,8 +377,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			die(_("git rm: unable to remove %s"), path);
 	}
 
-	if (show_only)
+	if (show_only) {
+		strbuf_release(&err);
 		return 0;
+	}
 
 	/*
 	 * Then, unless we used "--cached", remove the filenames from
@@ -431,5 +435,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			die(_("Unable to write new index file"));
 	}
 
+	strbuf_release(&err);
 	return 0;
 }
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 56abd18..f3875db 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -746,7 +746,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int preferred_index_format = 0;
 	char set_executable_bit = 0;
 	struct refresh_params refresh_args = {0, &has_errors};
-	int lock_error = 0;
+	struct strbuf lock_error = STRBUF_INIT;
 	int split_index = -1;
 	struct lock_file *lock_file;
 	struct parse_opt_ctx_t ctx;
@@ -843,9 +843,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	newfd = hold_locked_index(lock_file, 0);
-	if (newfd < 0)
-		lock_error = errno;
+	newfd = hold_locked_index(lock_file, &lock_error);
 
 	entries = read_cache();
 	if (entries < 0)
@@ -943,13 +941,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		if (newfd < 0) {
 			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
-			unable_to_lock_die(get_index_file(), 0, lock_error);
+			die("%s", lock_error.buf);
 		}
 		if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 			die("Unable to write new index file");
 	}
 
 	rollback_lock_file(lock_file);
-
+	strbuf_release(&lock_error);
 	return has_errors ? 1 : 0;
 }
diff --git a/cache-tree.c b/cache-tree.c
index 32772b9..b883b8f 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -595,6 +595,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 {
 	int entries, was_valid, newfd;
+	struct strbuf err = STRBUF_INIT;
 	struct lock_file *lock_file;
 
 	/*
@@ -603,11 +604,15 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	 */
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	newfd = hold_locked_index(lock_file, 1);
+	newfd = hold_locked_index(lock_file, &err);
+	if (newfd < 0)
+		die("%s", err.buf);
 
 	entries = read_cache();
-	if (entries < 0)
+	if (entries < 0) {
+		strbuf_release(&err);
 		return WRITE_TREE_UNREADABLE_INDEX;
+	}
 	if (flags & WRITE_TREE_IGNORE_CACHE_TREE)
 		cache_tree_free(&(active_cache_tree));
 
@@ -616,8 +621,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 
 	was_valid = cache_tree_fully_valid(active_cache_tree);
 	if (!was_valid) {
-		if (cache_tree_update(&the_index, flags) < 0)
+		if (cache_tree_update(&the_index, flags) < 0) {
+			strbuf_release(&err);
 			return WRITE_TREE_UNMERGED_INDEX;
+		}
 		if (0 <= newfd) {
 			if (!write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 				newfd = -1;
@@ -633,8 +640,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	if (prefix) {
 		struct cache_tree *subtree =
 			cache_tree_find(active_cache_tree, prefix);
-		if (!subtree)
+		if (!subtree) {
+			strbuf_release(&err);
 			return WRITE_TREE_PREFIX_ERROR;
+		}
 		hashcpy(sha1, subtree->sha1);
 	}
 	else
@@ -643,6 +652,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 	if (0 <= newfd)
 		rollback_lock_file(lock_file);
 
+	strbuf_release(&err);
 	return 0;
 }
 
diff --git a/cache.h b/cache.h
index ddaa30f..bfff653 100644
--- a/cache.h
+++ b/cache.h
@@ -572,7 +572,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct
 
 extern void update_index_if_able(struct index_state *, struct lock_file *);
 
-extern int hold_locked_index(struct lock_file *, int);
+extern int hold_locked_index(struct lock_file *, struct strbuf *err);
 extern void set_alternate_index_output(const char *);
 
 extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
diff --git a/merge-recursive.c b/merge-recursive.c
index fdb7d0f..82a2330 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1995,6 +1995,7 @@ int merge_recursive_generic(struct merge_options *o,
 			    struct commit **result)
 {
 	int clean;
+	struct strbuf err = STRBUF_INIT;
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 	struct commit *head_commit = get_ref(head, o->branch1);
 	struct commit *next_commit = get_ref(merge, o->branch2);
@@ -2004,20 +2005,26 @@ int merge_recursive_generic(struct merge_options *o,
 		int i;
 		for (i = 0; i < num_base_list; ++i) {
 			struct commit *base;
-			if (!(base = get_ref(base_list[i], sha1_to_hex(base_list[i]))))
+			if (!(base = get_ref(base_list[i], sha1_to_hex(base_list[i])))) {
+				strbuf_release(&err);
 				return error(_("Could not parse object '%s'"),
 					sha1_to_hex(base_list[i]));
+			}
 			commit_list_insert(base, &ca);
 		}
 	}
 
-	hold_locked_index(lock, 1);
+	if (hold_locked_index(lock, &err) < 0)
+		die("%s", err.buf);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
 			result);
 	if (active_cache_changed &&
-	    write_locked_index(&the_index, lock, COMMIT_LOCK))
+	    write_locked_index(&the_index, lock, COMMIT_LOCK)) {
+		strbuf_release(&err);
 		return error(_("Unable to write index."));
+	}
 
+	strbuf_release(&err);
 	return clean ? 0 : 1;
 }
 
diff --git a/merge.c b/merge.c
index fcff632..b26b7f3 100644
--- a/merge.c
+++ b/merge.c
@@ -53,11 +53,13 @@ int checkout_fast_forward(const unsigned char *head,
 	struct tree_desc t[MAX_UNPACK_TREES];
 	int i, nr_trees = 0;
 	struct dir_struct dir;
+	struct strbuf err = STRBUF_INIT;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	refresh_cache(REFRESH_QUIET);
 
-	hold_locked_index(lock_file, 1);
+	if (hold_locked_index(lock_file, &err) < 0)
+		die("%s", err.buf);
 
 	memset(&trees, 0, sizeof(trees));
 	memset(&opts, 0, sizeof(opts));
@@ -79,18 +81,25 @@ int checkout_fast_forward(const unsigned char *head,
 	setup_unpack_trees_porcelain(&opts, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		strbuf_release(&err);
 		return -1;
+	}
 	trees[nr_trees] = parse_tree_indirect(remote);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		strbuf_release(&err);
 		return -1;
+	}
 	for (i = 0; i < nr_trees; i++) {
 		parse_tree(trees[i]);
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
+	if (unpack_trees(nr_trees, t, &opts)) {
+		strbuf_release(&err);
 		return -1;
+	}
 	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
+	strbuf_release(&err);
 	return 0;
 }
diff --git a/read-cache.c b/read-cache.c
index 8f3e9eb..2ce1a76 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1368,12 +1368,16 @@ static int read_index_extension(struct index_state *istate,
 	return 0;
 }
 
-int hold_locked_index(struct lock_file *lk, int die_on_error)
+int hold_locked_index(struct lock_file *lk, struct strbuf *err)
 {
-	return hold_lock_file_for_update(lk, get_index_file(),
-					 die_on_error
-					 ? LOCK_DIE_ON_ERROR
-					 : 0);
+	const char *path;
+	int fd;
+
+	path = get_index_file();
+	fd = hold_lock_file_for_update(lk, path, 0);
+	if (fd < 0)
+		unable_to_lock_message(path, 0, errno, err);
+	return fd;
 }
 
 int read_index(struct index_state *istate)
diff --git a/rerere.c b/rerere.c
index 195f663..044e279 100644
--- a/rerere.c
+++ b/rerere.c
@@ -479,9 +479,11 @@ static struct lock_file index_lock;
 
 static void update_paths(struct string_list *update)
 {
+	struct strbuf err = STRBUF_INIT;
 	int i;
 
-	hold_locked_index(&index_lock, 1);
+	if (hold_locked_index(&index_lock, &err) < 0)
+		die("%s", err.buf);
 
 	for (i = 0; i < update->nr; i++) {
 		struct string_list_item *item = &update->items[i];
@@ -494,6 +496,7 @@ static void update_paths(struct string_list *update)
 			die("Unable to write new index file");
 	} else
 		rollback_lock_file(&index_lock);
+	strbuf_release(&err);
 }
 
 static int do_plain_rerere(struct string_list *rr, int fd)
diff --git a/sequencer.c b/sequencer.c
index a03d4fa..3b3a869 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -276,9 +276,11 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	int clean;
 	const char **xopt;
+	struct strbuf err = STRBUF_INIT;
 	static struct lock_file index_lock;
 
-	hold_locked_index(&index_lock, 1);
+	if (hold_locked_index(&index_lock, &err) < 0)
+		die("%s", err.buf);
 
 	read_cache();
 
@@ -323,6 +325,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		}
 	}
 
+	strbuf_release(&err);
 	return !clean;
 }
 
@@ -647,7 +650,13 @@ static void prepare_revs(struct replay_opts *opts)
 static void read_and_refresh_cache(struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
-	int index_fd = hold_locked_index(&index_lock, 0);
+	struct strbuf err = STRBUF_INIT;
+	int index_fd;
+
+	index_fd = hold_locked_index(&index_lock, &err);
+	if (index_fd < 0)
+		/* ignore the error */
+		strbuf_reset(&err);
 	if (read_index_preload(&the_index, NULL) < 0)
 		die(_("git %s: failed to read the index"), action_name(opts));
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
@@ -656,6 +665,7 @@ static void read_and_refresh_cache(struct replay_opts *opts)
 			die(_("git %s: failed to refresh the index"), action_name(opts));
 	}
 	rollback_lock_file(&index_lock);
+	strbuf_release(&err);
 }
 
 static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
diff --git a/test-scrap-cache-tree.c b/test-scrap-cache-tree.c
index 6efee31..3e30e8d 100644
--- a/test-scrap-cache-tree.c
+++ b/test-scrap-cache-tree.c
@@ -7,11 +7,14 @@ static struct lock_file index_lock;
 
 int main(int ac, char **av)
 {
-	hold_locked_index(&index_lock, 1);
+	struct strbuf err = STRBUF_INIT;
+	if (hold_locked_index(&index_lock, &err) < 0)
+		die("%s", err.buf);
 	if (read_cache() < 0)
 		die("unable to read index file");
 	active_cache_tree = NULL;
 	if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
 		die("unable to write index file");
+	strbuf_release(&err);
 	return 0;
 }
Jonathan Nieder· Dec 3, 2014, 05:26 UTC · re: Jonathan Nieder · lore

[PATCH 12/14] hold_lock_file_for_update: pass error message back through a strbuf

This makes it more obvious when callers forget to print a message on error, while still giving callers a chance to clean up before exiting.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-lockfile.txt | 33 +++++++-------------------------
 builtin/apply.c                          |  5 ++++-
 builtin/commit.c                         |  9 +++++----
 builtin/gc.c                             |  8 ++++++--
 bundle.c                                 | 10 +++++++---
 config.c                                 | 18 ++++++-----------
 credential-store.c                       |  8 ++++++--
 fast-import.c                            |  8 +++-----
 lockfile.c                               | 26 ++++++++++---------------
 lockfile.h                               |  8 ++++----
 read-cache.c                             |  9 +--------
 refs.c                                   | 17 +++++++++-------
 rerere.c                                 |  7 +++++--
 sequencer.c                              | 19 ++++++++++++++----
 shallow.c                                | 16 ++++++++++++----
 15 files changed, 101 insertions(+), 100 deletions(-)
Show changes to 15 files +101 −100

Documentation/technical/api-lockfile.txt, builtin/apply.c, builtin/commit.c, builtin/gc.c, bundle.c, config.c, credential-store.c, fast-import.c, lockfile.c, lockfile.h, read-cache.c, refs.c, rerere.c, sequencer.c, shallow.c

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index fa60cfd..8cb6704 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -87,21 +87,8 @@ Error handling
 --------------
 
 The `hold_lock_file_*` functions return a file descriptor on success
-or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On
-errors, `errno` describes the reason for failure. Errors can be
-reported by passing `errno` to one of the following helper functions:
-
-unable_to_lock_message::
-
-	Append an appropriate error message to a `strbuf`.
-
-unable_to_lock_error::
-
-	Emit an appropriate error message using `error()`.
-
-unable_to_lock_die::
-
-	Emit an appropriate error message and `die()`.
+or -1 on failure.  On errors, a message describing the reason for
+failure is appended to the strbuf specified using the 'err' argument.
 
 Similarly, `commit_lock_file`, `commit_lock_file_to`, and
 `close_lock_file` return 0 on success. On failure they set `errno`
@@ -111,7 +98,7 @@ appropriately, do their best to roll back the lockfile, and return -1.
 Flags
 -----
 
-The following flags can be passed to `hold_lock_file_for_update` or
+The following flag can be passed to `hold_lock_file_for_update` or
 `hold_lock_file_for_append`:
 
 LOCK_NO_DEREF::
@@ -136,22 +123,16 @@ LOCK_OUTSIDE_REPOSITORY:
 For example, this flag should be set when locking a configuration
 file in the user's home directory.
 
-LOCK_DIE_ON_ERROR::
-
-	If a lock is already taken for the file, `die()` with an error
-	message. If this option is not specified, trying to lock a
-	file that is already locked returns -1 to the caller.
-
-
 The functions
 -------------
 
 hold_lock_file_for_update::
 
 	Take a pointer to `struct lock_file`, the path of the file to
-	be locked (e.g. `$GIT_DIR/index`) and a flags argument (see
-	above). Attempt to create a lockfile for the destination and
-	return the file descriptor for writing to the file.
+	be locked (e.g. `$GIT_DIR/index`), a flags argument (see
+	above), and a strbuf for error messages. Attempt to create a
+	lockfile for the destination and return the file descriptor for
+	writing to the file.
 
 hold_lock_file_for_append::
 
diff --git a/builtin/apply.c b/builtin/apply.c
index cda438f..07626fb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3711,6 +3711,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 	struct patch *patch;
 	struct index_state result = { NULL };
 	static struct lock_file lock;
+	struct strbuf err = STRBUF_INIT;
 
 	/* Once we start supporting the reverse patch, it may be
 	 * worth showing the new sha1 prefix, but until then...
@@ -3748,11 +3749,13 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 			die ("Could not add %s to temporary index", name);
 	}
 
-	hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+	if (hold_lock_file_for_update(&lock, filename, 0, &err) < 0)
+		die("%s", err.buf);
 	if (write_locked_index(&result, &lock, COMMIT_LOCK))
 		die ("Could not write temporary index to %s", filename);
 
 	discard_index(&result);
+	strbuf_release(&err);
 }
 
 static void stat_patch_list(struct patch *patch)
diff --git a/builtin/commit.c b/builtin/commit.c
index edc4493..f64024c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -468,10 +468,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
 		die(_("unable to write new_index file"));
 
-	hold_lock_file_for_update(&false_lock,
-				  git_path("next-index-%"PRIuMAX,
-					   (uintmax_t) getpid()),
-				  LOCK_DIE_ON_ERROR);
+	if (hold_lock_file_for_update(&false_lock,
+				      git_path("next-index-%"PRIuMAX,
+					       (uintmax_t) getpid()), 0,
+				      &err) < 0)
+		die("%s", err.buf);
 
 	create_base_index(current_head);
 	add_remove_files(&partial);
diff --git a/builtin/gc.c b/builtin/gc.c
index 005adbe..86e9eca 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -190,6 +190,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	static struct lock_file lock;
 	char my_host[128];
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	struct stat st;
 	uintmax_t pid;
 	FILE *fp;
@@ -202,8 +203,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	if (gethostname(my_host, sizeof(my_host)))
 		strcpy(my_host, "unknown");
 
-	fd = hold_lock_file_for_update(&lock, git_path("gc.pid"),
-				       LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&lock, git_path("gc.pid"), 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	if (!force) {
 		static char locking_host[128];
 		int should_exit;
@@ -231,6 +233,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 			if (fd >= 0)
 				rollback_lock_file(&lock);
 			*ret_pid = pid;
+			strbuf_release(&err);
 			return locking_host;
 		}
 	}
@@ -245,6 +248,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 	sigchain_push_common(remove_pidfile_on_signal);
 	atexit(remove_pidfile);
 
+	strbuf_release(&err);
 	return NULL;
 }
 
diff --git a/bundle.c b/bundle.c
index 2e2dbd5..db03782 100644
--- a/bundle.c
+++ b/bundle.c
@@ -417,9 +417,13 @@ int create_bundle(struct bundle_header *header, const char *path,
 	bundle_to_stdout = !strcmp(path, "-");
 	if (bundle_to_stdout)
 		bundle_fd = 1;
-	else
-		bundle_fd = hold_lock_file_for_update(&lock, path,
-						      LOCK_DIE_ON_ERROR);
+	else {
+		struct strbuf err = STRBUF_INIT;
+		bundle_fd = hold_lock_file_for_update(&lock, path, 0, &err);
+		if (bundle_fd < 0)
+			die("%s", err.buf);
+		strbuf_release(&err);
+	}
 
 	/* write signature */
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
diff --git a/config.c b/config.c
index dacde5f..4117ff0 100644
--- a/config.c
+++ b/config.c
@@ -1923,6 +1923,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	int fd = -1, in_fd;
 	int ret;
 	struct lock_file *lock = NULL;
+	struct strbuf err = STRBUF_INIT;
 	char *filename_buf = NULL;
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
@@ -1941,14 +1942,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	 */
 	lock = xcalloc(1, sizeof(struct lock_file));
 	fd = hold_lock_file_for_update(lock, config_filename,
-				       LOCK_OUTSIDE_REPOSITORY);
+				       LOCK_OUTSIDE_REPOSITORY, &err);
 	if (fd < 0) {
-		struct strbuf err = STRBUF_INIT;
-
-		unable_to_lock_message(config_filename,
-				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
 		error("%s", err.buf);
-		strbuf_release(&err);
 		free(store.key);
 		ret = CONFIG_NO_LOCK;
 		goto out_free;
@@ -2126,6 +2122,7 @@ out_free:
 	if (lock)
 		rollback_lock_file(lock);
 	free(filename_buf);
+	strbuf_release(&err);
 	return ret;
 
 write_err_out:
@@ -2203,6 +2200,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 	int ret = 0, remove = 0;
 	char *filename_buf = NULL;
 	struct lock_file *lock;
+	struct strbuf err = STRBUF_INIT;
 	int out_fd;
 	char buf[1024];
 	FILE *config_file;
@@ -2218,14 +2216,9 @@ int git_config_rename_section_in_file(const char *config_filename,
 
 	lock = xcalloc(1, sizeof(struct lock_file));
 	out_fd = hold_lock_file_for_update(lock, config_filename,
-					   LOCK_OUTSIDE_REPOSITORY);
+					   LOCK_OUTSIDE_REPOSITORY, &err);
 	if (out_fd < 0) {
-		struct strbuf err = STRBUF_INIT;
-
-		unable_to_lock_message(config_filename,
-				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
 		ret = error("%s", err.buf);
-		strbuf_release(&err);
 		goto out;
 	}
 
@@ -2295,6 +2288,7 @@ unlock_and_out:
 		ret = error("could not commit config file %s", config_filename);
 out:
 	free(filename_buf);
+	strbuf_release(&err);
 	return ret;
 }
 
diff --git a/credential-store.c b/credential-store.c
index cd71156..beffa87 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -55,13 +55,17 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
-	hold_lock_file_for_update(&credential_lock, fn,
-				  LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);
+	struct strbuf err = STRBUF_INIT;
+
+	if (hold_lock_file_for_update(&credential_lock, fn,
+				      LOCK_OUTSIDE_REPOSITORY, &err) < 0);
+		die("%s", err.buf);
 	if (extra)
 		print_line(extra);
 	parse_credential_file(fn, c, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to commit credential store");
+	strbuf_release(&err);
 }
 
 static void store_credential(const char *fn, struct credential *c)
diff --git a/fast-import.c b/fast-import.c
index bf8faa9..736c37b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1794,21 +1794,19 @@ static void dump_marks_helper(FILE *f,
 static void dump_marks(void)
 {
 	static struct lock_file mark_lock;
+	struct strbuf err = STRBUF_INIT;
 	FILE *f;
 
 	if (!export_marks_file)
 		return;
 
 	if (hold_lock_file_for_update(&mark_lock, export_marks_file,
-				      LOCK_OUTSIDE_REPOSITORY) < 0) {
-		struct strbuf err = STRBUF_INIT;
-
-		unable_to_lock_message(export_marks_file,
-				       LOCK_OUTSIDE_REPOSITORY, errno, &err);
+				      LOCK_OUTSIDE_REPOSITORY, &err) < 0) {
 		failure |= error("%s", err.buf);
 		strbuf_release(&err);
 		return;
 	}
+	strbuf_release(&err);
 
 	f = fdopen_lock_file(&mark_lock, "w");
 	if (!f) {
diff --git a/lockfile.c b/lockfile.c
index 102584f..a79679b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -98,8 +98,8 @@ static void resolve_symlink(struct strbuf *path)
 	strbuf_reset(&link);
 }
 
-/* Make sure errno contains a meaningful value on error */
-static int lock_file(struct lock_file *lk, const char *path, int flags)
+static int lock_file(struct lock_file *lk, const char *path,
+		     int flags, struct strbuf *err)
 {
 	size_t pathlen = strlen(path);
 
@@ -134,16 +134,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	strbuf_addstr(&lk->filename, LOCK_SUFFIX);
 	lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (lk->fd < 0) {
+		unable_to_lock_message(path, flags, errno, err);
 		strbuf_reset(&lk->filename);
 		return -1;
 	}
 	lk->owner = getpid();
 	lk->active = 1;
 	if (adjust_shared_perm(lk->filename.buf)) {
-		int save_errno = errno;
-		error("cannot fix permission bits on %s", lk->filename.buf);
+		strbuf_addf(err, "cannot fix permission bits on %s",
+			    lk->filename.buf);
 		rollback_lock_file(lk);
-		errno = save_errno;
 		return -1;
 	}
 	return lk->fd;
@@ -178,13 +178,10 @@ NORETURN void unable_to_lock_die(const char *path, int flags, 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 hold_lock_file_for_update(struct lock_file *lk, const char *path,
+			      int flags, struct strbuf *err)
 {
-	int fd = lock_file(lk, path, flags);
-	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-		unable_to_lock_die(path, flags, errno);
-	return fd;
+	return lock_file(lk, path, flags, err);
 }
 
 int hold_lock_file_for_append(struct lock_file *lk, const char *path,
@@ -192,14 +189,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,
 {
 	int fd, orig_fd;
 
-	assert(!(flags & LOCK_DIE_ON_ERROR));
 	assert(err && !err->len);
 
-	fd = lock_file(lk, path, flags);
-	if (fd < 0) {
-		unable_to_lock_message(path, flags, errno, err);
+	fd = lock_file(lk, path, flags, err);
+	if (fd < 0)
 		return fd;
-	}
 
 	orig_fd = open(path, O_RDONLY);
 	if (orig_fd < 0) {
diff --git a/lockfile.h b/lockfile.h
index 779a992..6d0a9bb 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -68,14 +68,14 @@ struct lock_file {
 #define LOCK_SUFFIX ".lock"
 #define LOCK_SUFFIX_LEN 5
 
-#define LOCK_DIE_ON_ERROR 1
-#define LOCK_NO_DEREF 2
-#define LOCK_OUTSIDE_REPOSITORY 4
+#define LOCK_NO_DEREF 1
+#define LOCK_OUTSIDE_REPOSITORY 2
 
 extern void unable_to_lock_message(const char *path, int, int err,
 				   struct strbuf *buf);
 extern NORETURN void unable_to_lock_die(const char *path, int, int err);
-extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
+extern int hold_lock_file_for_update(struct lock_file *, const char *path,
+				     int, struct strbuf *err);
 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);
diff --git a/read-cache.c b/read-cache.c
index 2ce1a76..229eea0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1370,14 +1370,7 @@ static int read_index_extension(struct index_state *istate,
 
 int hold_locked_index(struct lock_file *lk, struct strbuf *err)
 {
-	const char *path;
-	int fd;
-
-	path = get_index_file();
-	fd = hold_lock_file_for_update(lk, path, 0);
-	if (fd < 0)
-		unable_to_lock_message(path, 0, errno, err);
-	return fd;
+	return hold_lock_file_for_update(lk, get_index_file(), 0, err);
 }
 
 int read_index(struct index_state *istate)
diff --git a/refs.c b/refs.c
index 39e43cf..f39c29c 100644
--- a/refs.c
+++ b/refs.c
@@ -2229,6 +2229,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const struct string_list *skip,
 					    int flags, int *type_p)
 {
+	struct strbuf err = STRBUF_INIT;
 	char *ref_file;
 	const char *orig_refname = refname;
 	struct ref_lock *lock;
@@ -2316,22 +2317,25 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		goto error_return;
 	}
 
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
+	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags, &err);
 	if (lock->lock_fd < 0) {
-		if (errno == ENOENT && --attempts_remaining > 0)
+		if (errno == ENOENT && --attempts_remaining > 0) {
 			/*
 			 * Maybe somebody just deleted one of the
 			 * directories leading to ref_file.  Try
 			 * again:
 			 */
+			strbuf_reset(&err);
 			goto retry;
-		else
-			unable_to_lock_die(ref_file, lflags, errno);
+		}
+		die("%s", err.buf);
 	}
+	strbuf_release(&err);
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
  error_return:
 	unlock_ref(lock);
+	strbuf_release(&err);
 	errno = last_errno;
 	return NULL;
 }
@@ -2375,10 +2379,9 @@ 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"), 0) < 0) {
-		unable_to_lock_message(git_path("packed-refs"), 0, errno, err);
+	if (hold_lock_file_for_update(&packlock,
+				      git_path("packed-refs"), 0, err) < 0)
 		return -1;
-	}
 
 	/*
 	 * Get the current packed-refs while holding the lock.  If the
diff --git a/rerere.c b/rerere.c
index 044e279..c1eaadd 100644
--- a/rerere.c
+++ b/rerere.c
@@ -600,6 +600,7 @@ static int is_rerere_enabled(void)
 
 int setup_rerere(struct string_list *merge_rr, int flags)
 {
+	struct strbuf err = STRBUF_INIT;
 	int fd;
 
 	git_rerere_config();
@@ -609,9 +610,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
 		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
 	merge_rr_path = git_pathdup("MERGE_RR");
-	fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
-				       LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	read_rr(merge_rr);
+	strbuf_release(&err);
 	return fd;
 }
 
diff --git a/sequencer.c b/sequencer.c
index 3b3a869..c0d8864 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -204,14 +204,17 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 static void write_message(struct strbuf *msgbuf, const char *filename)
 {
 	static struct lock_file msg_file;
+	struct strbuf err = STRBUF_INIT;
 
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0, &err);
+	if (msg_fd < 0)
+		die("%s", err.buf);
 	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
 		die_errno(_("Could not write to %s"), filename);
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
 		die(_("Error wrapping up %s"), filename);
+	strbuf_release(&err);
 }
 
 static struct tree *empty_tree(void)
@@ -854,14 +857,18 @@ static void save_head(const char *head)
 	const char *head_file = git_path(SEQ_HEAD_FILE);
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&head_lock, head_file, 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	strbuf_addf(&buf, "%s\n", head);
 	if (write_in_full(fd, buf.buf, buf.len) < 0)
 		die_errno(_("Could not write to %s"), head_file);
 	if (commit_lock_file(&head_lock) < 0)
 		die(_("Error wrapping up %s."), head_file);
+	strbuf_release(&err);
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -935,9 +942,12 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&todo_lock, todo_file, 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	if (format_todo(&buf, todo_list, opts) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
@@ -949,6 +959,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 		die(_("Error wrapping up %s."), todo_file);
 	}
 	strbuf_release(&buf);
+	strbuf_release(&err);
 }
 
 static void save_opts(struct replay_opts *opts)
diff --git a/shallow.c b/shallow.c
index cdd0775..4a30a77 100644
--- a/shallow.c
+++ b/shallow.c
@@ -259,10 +259,13 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 			     const struct sha1_array *extra)
 {
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"),
-				       LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(shallow_lock,
+				       git_path("shallow"), 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
@@ -276,6 +279,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 		 */
 		*alternate_shallow_file = "";
 	strbuf_release(&sb);
+	strbuf_release(&err);
 }
 
 static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *cb)
@@ -301,6 +305,7 @@ void prune_shallow(int show_only)
 {
 	static struct lock_file shallow_lock;
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
 	int fd;
 
 	if (show_only) {
@@ -308,8 +313,10 @@ void prune_shallow(int show_only)
 		strbuf_release(&sb);
 		return;
 	}
-	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
-				       LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&shallow_lock,
+				       git_path("shallow"), 0, &err);
+	if (fd < 0)
+		die("%s", err.buf);
 	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
 		if (write_in_full(fd, sb.buf, sb.len) != sb.len)
@@ -321,6 +328,7 @@ void prune_shallow(int show_only)
 		rollback_lock_file(&shallow_lock);
 	}
 	strbuf_release(&sb);
+	strbuf_release(&err);
 }
 
 struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);
Jonathan Nieder· Dec 3, 2014, 18:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH 12/14] hold_lock_file_for_update: pass error message back through a strbuf

Jonathan Nieder wrote:
Show 13 quoted lines
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -55,13 +55,17 @@ static void print_line(struct strbuf *buf)
>  static void rewrite_credential_file(const char *fn, struct credential *c,
>  				    struct strbuf *extra)
>  {
> -	hold_lock_file_for_update(&credential_lock, fn,
> -				  LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);
> +	struct strbuf err = STRBUF_INIT;
> +
> +	if (hold_lock_file_for_update(&credential_lock, fn,
> +				      LOCK_OUTSIDE_REPOSITORY, &err) < 0);
> +		die("%s", err.buf);

I forgot to squash this in. Sorry for the confusion. (I'm planning to reroll with this and the commit clarification tboegi mentioned --- other comments welcome before then.)

-- >8 --
Subject: fixup! hold_lock_file_for_update: pass error message back through a strbuf
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 credential-store.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to credential-store.c +1 −1
diff --git a/credential-store.c b/credential-store.c
index beffa87..2d5d92f 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -58,7 +58,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 	struct strbuf err = STRBUF_INIT;
 
 	if (hold_lock_file_for_update(&credential_lock, fn,
-				      LOCK_OUTSIDE_REPOSITORY, &err) < 0);
+				      LOCK_OUTSIDE_REPOSITORY, &err) < 0)
 		die("%s", err.buf);
 	if (extra)
 		print_line(extra);
Jonathan Nieder· Dec 3, 2014, 05:26 UTC · re: Jonathan Nieder · lore

[PATCH 13/14] lockfile: remove unused function 'unable_to_lock_die'

The old callers now use the message passed back by hold_lock_file_for_update / hold_lock_file_for_append instead of trying to interpret errno.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 lockfile.c | 8 --------
 lockfile.h | 1 -
 2 files changed, 9 deletions(-)
Show changes to 2 files +0 −9

lockfile.c, lockfile.h

diff --git a/lockfile.c b/lockfile.c
index a79679b..8d8d5ed 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -170,14 +170,6 @@ void unable_to_lock_message(const char *path, int flags, int err,
 	}
 }
 
-NORETURN void unable_to_lock_die(const char *path, int flags, int err)
-{
-	struct strbuf buf = STRBUF_INIT;
-
-	unable_to_lock_message(path, flags, err, &buf);
-	die("%s", buf.buf);
-}
-
 int hold_lock_file_for_update(struct lock_file *lk, const char *path,
 			      int flags, struct strbuf *err)
 {
diff --git a/lockfile.h b/lockfile.h
index 6d0a9bb..b4d29a3 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -73,7 +73,6 @@ struct lock_file {
 
 extern void unable_to_lock_message(const char *path, int, int err,
 				   struct strbuf *buf);
-extern NORETURN void unable_to_lock_die(const char *path, int, int err);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path,
 				     int, struct strbuf *err);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path,
Jonathan Nieder· Dec 3, 2014, 05:27 UTC · re: Jonathan Nieder · lore

[PATCH 14/14] lockfile: make 'unable_to_lock_message' private

The old external callers now use the message passed back by hold_lock_file_for_update / hold_lock_file_for_append instead of trying to interpret errno.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  Thanks for reading.
Thoughts?
 lockfile.c | 42 +++++++++++++++++++++---------------------
 lockfile.h |  2 --
 2 files changed, 21 insertions(+), 23 deletions(-)
Show changes to 2 files +21 −23

lockfile.c, lockfile.h

diff --git a/lockfile.c b/lockfile.c
index 8d8d5ed..7121370 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -98,6 +98,27 @@ static void resolve_symlink(struct strbuf *path)
 	strbuf_reset(&link);
 }
 
+static void unable_to_lock_message(const char *path, int flags, int err,
+			    struct strbuf *buf)
+{
+	if (err != EEXIST) {
+		strbuf_addf(buf, "Unable to create '%s.lock': %s",
+			    absolute_path(path), strerror(err));
+	} else if (flags & LOCK_OUTSIDE_REPOSITORY) {
+		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
+		    "If no other git process is currently running, this probably means\n"
+		    "another git process crashed earlier. Make sure no other git process\n"
+		    "is running and remove the file manually to continue.",
+			    absolute_path(path), strerror(err));
+	} else {
+		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));
+	}
+}
+
 static int lock_file(struct lock_file *lk, const char *path,
 		     int flags, struct strbuf *err)
 {
@@ -149,27 +170,6 @@ static int lock_file(struct lock_file *lk, const char *path,
 	return lk->fd;
 }
 
-void unable_to_lock_message(const char *path, int flags, int err,
-			    struct strbuf *buf)
-{
-	if (err != EEXIST) {
-		strbuf_addf(buf, "Unable to create '%s.lock': %s",
-			    absolute_path(path), strerror(err));
-	} else if (flags & LOCK_OUTSIDE_REPOSITORY) {
-		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
-		    "If no other git process is currently running, this probably means\n"
-		    "another git process crashed earlier. Make sure no other git process\n"
-		    "is running and remove the file manually to continue.",
-			    absolute_path(path), strerror(err));
-	} else {
-		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));
-	}
-}
-
 int hold_lock_file_for_update(struct lock_file *lk, const char *path,
 			      int flags, struct strbuf *err)
 {
diff --git a/lockfile.h b/lockfile.h
index b4d29a3..02e26fe 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -71,8 +71,6 @@ struct lock_file {
 #define LOCK_NO_DEREF 1
 #define LOCK_OUTSIDE_REPOSITORY 2
 
-extern void unable_to_lock_message(const char *path, int, int err,
-				   struct strbuf *buf);
 extern int hold_lock_file_for_update(struct lock_file *, const char *path,
 				     int, struct strbuf *err);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path,
Stefan Beller· Dec 3, 2014, 20:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH 14/14] lockfile: make 'unable_to_lock_message' private

On Tue, Dec 02, 2014 at 09:27:34PM -0800, Jonathan Nieder wrote:
Show 9 quoted lines
> The old external callers now use the message passed back by
> hold_lock_file_for_update / hold_lock_file_for_append instead of
> trying to interpret errno.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> That's the end of the series.  Thanks for reading.
> 
> Thoughts?
I reviewed the whole series and seems fine with me.
Show 78 quoted lines
> 
>  lockfile.c | 42 +++++++++++++++++++++---------------------
>  lockfile.h |  2 --
>  2 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/lockfile.c b/lockfile.c
> index 8d8d5ed..7121370 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -98,6 +98,27 @@ static void resolve_symlink(struct strbuf *path)
>  	strbuf_reset(&link);
>  }
>  
> +static void unable_to_lock_message(const char *path, int flags, int err,
> +			    struct strbuf *buf)
> +{
> +	if (err != EEXIST) {
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s",
> +			    absolute_path(path), strerror(err));
> +	} else if (flags & LOCK_OUTSIDE_REPOSITORY) {
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
> +		    "If no other git process is currently running, this probably means\n"
> +		    "another git process crashed earlier. Make sure no other git process\n"
> +		    "is running and remove the file manually to continue.",
> +			    absolute_path(path), strerror(err));
> +	} else {
> +		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));
> +	}
> +}
> +
>  static int lock_file(struct lock_file *lk, const char *path,
>  		     int flags, struct strbuf *err)
>  {
> @@ -149,27 +170,6 @@ static int lock_file(struct lock_file *lk, const char *path,
>  	return lk->fd;
>  }
>  
> -void unable_to_lock_message(const char *path, int flags, int err,
> -			    struct strbuf *buf)
> -{
> -	if (err != EEXIST) {
> -		strbuf_addf(buf, "Unable to create '%s.lock': %s",
> -			    absolute_path(path), strerror(err));
> -	} else if (flags & LOCK_OUTSIDE_REPOSITORY) {
> -		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
> -		    "If no other git process is currently running, this probably means\n"
> -		    "another git process crashed earlier. Make sure no other git process\n"
> -		    "is running and remove the file manually to continue.",
> -			    absolute_path(path), strerror(err));
> -	} else {
> -		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));
> -	}
> -}
> -
>  int hold_lock_file_for_update(struct lock_file *lk, const char *path,
>  			      int flags, struct strbuf *err)
>  {
> diff --git a/lockfile.h b/lockfile.h
> index b4d29a3..02e26fe 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -71,8 +71,6 @@ struct lock_file {
>  #define LOCK_NO_DEREF 1
>  #define LOCK_OUTSIDE_REPOSITORY 2
>  
> -extern void unable_to_lock_message(const char *path, int, int err,
> -				   struct strbuf *buf);
>  extern int hold_lock_file_for_update(struct lock_file *, const char *path,
>  				     int, struct strbuf *err);
>  extern int hold_lock_file_for_append(struct lock_file *, const char *path,
Junio C Hamano· Nov 18, 2014, 16:32 UTC · re: Jonathan Nieder · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> Do any callers care about errno?  Does the function's API
> documentation say it will make errno meaningful on error, so people
> making changes to copy_fd in the future know to maintain that
> property?
>
> *searches*
>
> Looks like callers are:
>
>  convert.c::filter_buffer_or_fd, which doesn't care
>
>  copy.c::copy_file, which also doesn't care
>
>  lockfile.c::hold_lock_file_for_append, which started caring
>  in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
>  before returning, 2014-10-01).  But no callers of that function
>  care yet.
>
> So this is about fixing a bug-waiting-to-happen in
> hold_lock_file_for_append.  That would be enough to motivate the
> change.
OK.  Perhaps convert.c wants to be fixed?
Show 14 quoted lines
>> +			int save_errno = errno;
>> +			error("copy-fd: read returned %s", strerror(errno));
>> +			errno = save_errno;
>> +			return -1;
>
> Any caller is presumably going to turn around and print strerror(errno)
> again, producing repetitive output.
>
> Can we do better?  E.g., if the signature were
>
> 	int copy_fd(int ifd, int ofd, struct strbuf *err);
>
> then we could write the error message to the err strbuf for the
> caller to print.

The problem you are solving is "because the caller may want to do its own error message, stop the callee from emitting the error message unconditionally", but if we are addressing "the caller may want to...", I think we should find a single solution that addresses other kind fo things the caller may want to do.

For example, two callers may want to phrase the same error condition differently, e.g. depending on what the user asked to do. We'd want something better than the ERRORMSG trick used in unpack-trees.c, which does not scale, and I think passing some data that represents "here is how the caller wants the errors to be handled and presented" is probably a part of the solution, but strbuf *err is not that.

In short I am not a very big fan of passing around strbuf *err to low level helper API functions myself.

But the approach does not make things much worse than it currently is, other than code churns to pass an extra pointer around.

Junio C Hamano· Nov 18, 2014, 17:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> In short I am not a very big fan of passing around strbuf *err to
> low level helper API functions myself.
>
> But the approach does not make things much worse than it currently
> is, other than code churns to pass an extra pointer around.
Sorry I left the conclusion out of the message.

As it does not make things much worse, and does give slightly better flexibility on error message emission to the callers, let's go with the "strbuf *err" arpporach for now.

Until we hit a wall we cannot climb over, at which time we may need to redo it, but let's first see how it goes.

Michael Haggerty· Nov 21, 2014, 09:14 UTC · re: Stefan Beller · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

On 11/17/2014 11:14 PM, Stefan Beller wrote:
Show 51 quoted lines
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Update copy_fd to return a meaningful errno on failure and also 
> preserve the existing errno variable.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> As announced in [1], I'm going to take over the 
> ref-transactions-reflog series by Ronnie Sahlberg.
> 
> This patch was sent previously to the list as part of
> that series[2], but it seems to be unrelated to me.
> 
> Thanks,
> Stefan
> 
> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
> [2] http://www.spinics.net/lists/git/msg240784.html
>  copy.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/copy.c b/copy.c
> index f2970ec..a8d366e 100644
> --- a/copy.c
> +++ b/copy.c
> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
>  		if (!len)
>  			break;
>  		if (len < 0) {
> -			return error("copy-fd: read returned %s",
> -				     strerror(errno));
> +			int save_errno = errno;
> +			error("copy-fd: read returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
> +		}
> +		if (write_in_full(ofd, buffer, len) < 0) {
> +			int save_errno = errno;
> +			error("copy-fd: write returned %s", strerror(errno));
> +			errno = save_errno;
> +			return -1;
>  		}
> -		if (write_in_full(ofd, buffer, len) < 0)
> -			return error("copy-fd: write returned %s",
> -				     strerror(errno));
>  	}
>  	return 0;
>  }
>

Couldn't we save ourselves a lot of this "save_errno" boilerplate by making error() and warning() preserve errno? They don't do any meaningful internal error checking anyway, so even if they overwrite errno, that value is useless to callers (who undoubtedly wouldn't check such a value anyway). Something like

 int error(const char *err, ...)
 {
 	va_list params;
+	int save_errno = errno;
 	va_start(params, err);
 	error_routine(err, params);
 	va_end(params);
+	errno = save_errno;
 	return -1;
 }
and the same for warning()?
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Michael Haggerty· Nov 21, 2014, 09:17 UTC · re: Michael Haggerty · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

On 11/21/2014 10:14 AM, Michael Haggerty wrote:
> Couldn't we save ourselves a lot of this "save_errno" boilerplate by
> making error() and warning() preserve errno? [...]
Never mind; I see that Peff already submitted a patch to this effect.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Junio C Hamano· Nov 21, 2014, 17:48 UTC · re: Michael Haggerty · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 5 quoted lines
> On 11/21/2014 10:14 AM, Michael Haggerty wrote:
>> Couldn't we save ourselves a lot of this "save_errno" boilerplate by
>> making error() and warning() preserve errno? [...]
>
> Never mind; I see that Peff already submitted a patch to this effect.

My understanding of the conclusion of those four patches was that only a single updated one is needed, and "moving save/restore inside error()" did not have to survive.

  http://article.gmane.org/gmane.comp.version-control.git/259911
Jeff King· Nov 21, 2014, 17:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > On 11/21/2014 10:14 AM, Michael Haggerty wrote:
> >> Couldn't we save ourselves a lot of this "save_errno" boilerplate by
> >> making error() and warning() preserve errno? [...]
> >
> > Never mind; I see that Peff already submitted a patch to this effect.
> 
> My understanding of the conclusion of those four patches was that
> only a single updated one is needed, and "moving save/restore inside
> error()" did not have to survive.
> 
>   http://article.gmane.org/gmane.comp.version-control.git/259911

Yeah, the callsite I had intended ended up needing to save it across more than just error(). And I think that is probably why we have never done any errno-handling inside error() before (it is certainly not the first time I thought of doing such a thing): real-world cases tend to be a little more complicated.

That being said, if this copy() case is one that could benefit, I do not have any problem with picking up (or just reusing the concept of) my 1/4 from that series. It probably does not _hurt_ anything, and if it can help in even a few cases, it may be worth it.

-Peff
Junio C Hamano· Nov 21, 2014, 18:31 UTC · re: Jeff King · lore

Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>> > On 11/21/2014 10:14 AM, Michael Haggerty wrote:
>> >> Couldn't we save ourselves a lot of this "save_errno" boilerplate by
>> >> making error() and warning() preserve errno? [...]
>> >
>> > Never mind; I see that Peff already submitted a patch to this effect.
>> 
>> My understanding of the conclusion of those four patches was that
>> only a single updated one is needed, and "moving save/restore inside
>> error()" did not have to survive.
>> 
>>   http://article.gmane.org/gmane.comp.version-control.git/259911
>
> Yeah, the callsite I had intended ended up needing to save it across
> more than just error(). And I think that is probably why we have never
> done any errno-handling inside error() before (it is certainly not the
> first time I thought of doing such a thing): real-world cases tend to be
> a little more complicated.
>
> That being said, if this copy() case is one that could benefit, I do not
> have any problem with picking up (or just reusing the concept of) my
> 1/4 from that series. It probably does not _hurt_ anything, and if it
> can help in even a few cases, it may be worth it.

Yeah, I agree. My summary was "did not have to survive", not "had to die". Like you, I do not mind the change as long as other code paths benefit from it.

← back to recent threads