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

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

[Stefan Beller]

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;
 }
-- 
2.2.0.rc1.24.g562add4

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

[Junio C Hamano]

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;
>  }

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

[Stefan Beller]

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:
> 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;
>>  }

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

[Jonathan Nieder]

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.

[...]
> --- 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>

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)

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

[Stefan Beller]

On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.

>
> 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?

>
> 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)

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

[Jonathan Nieder]

(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:

>>> 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).

[...]
>> 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

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

[Stefan Beller]

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

>
> 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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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?

>> +			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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

[Michael Haggerty]

On 11/17/2014 11:14 PM, Stefan Beller wrote:
> 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

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

[Michael Haggerty]

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

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

[Junio C Hamano]

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

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Jonathan Nieder]

Stefan Beller wrote:
> On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> 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(-)

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

[Jonathan Nieder]

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(+)

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.

[PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY

[Jonathan Nieder]

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(-)

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");

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

[Jonathan Nieder]

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(-)

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;
 }
 

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

[Jonathan Nieder]

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

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

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

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

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

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

[Jonathan Nieder]

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(-)

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

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

[Jonathan Nieder]

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(-)

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;
 	}
 

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

[Jonathan Nieder]

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(-)

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;
 	}
 

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

[Jonathan Nieder]

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(-)

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);

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

[Jonathan Nieder]

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(-)

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;
 	}
 

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

[Jonathan Nieder]

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(-)

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)

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

[Jonathan Nieder]

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(-)

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;
 }

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

[Jonathan Nieder]

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(-)

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);

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

[Jonathan Nieder]

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(-)

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,

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

[Jonathan Nieder]

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(-)

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,

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

[Torsten Bögershausen]

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

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

[Jonathan Nieder]

Torsten Bögershausen wrote:
> On 12/03/2014 06:14 AM, Jonathan Nieder wrote:

>> --- 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

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

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- 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(-)

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);

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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?

>  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");

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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?

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

[Eric Sunshine]

On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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?

> +}
> +
>  static void add_lines(struct strbuf *out,
>                         const char *prefix1,
>                         const char *prefix2,

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

[Jonathan Nieder]

Eric Sunshine wrote:
> On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> +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

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

[Jonathan Nieder]

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

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

[Jonathan Nieder]

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

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

[Stefan Beller]

On Wed, Dec 3, 2014 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Jonathan Nieder]

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?

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

Thanks,
Jonathan

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

[Stefan Beller]

On Tue, Dec 02, 2014 at 09:27:34PM -0800, Jonathan Nieder wrote:
> 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.

> 
>  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,

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

[Jeff King]

On Wed, Dec 03, 2014 at 11:01:08AM -0800, 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?
> 
> 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

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

[Jonathan Nieder]

Hi,

Jeff King wrote:

> 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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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).

> +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.

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder writes:
>> 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

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

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

[Jonathan Nieder]

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?

> 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

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

[Jonathan Nieder]

Jonathan Nieder wrote:
> Junio C Hamano wrote:

>> 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(-)

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)

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

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

[Junio C Hamano]

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.

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

[Jonathan Nieder]

Junio C Hamano wrote:
> 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).

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

Thanks,
Jonathan

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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-<.

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

[Jonathan Nieder]

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.  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.)

> 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

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

[Junio C Hamano]

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".

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.

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

[Jonathan Nieder]

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).

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

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

[Jeff King]

On Wed, Dec 03, 2014 at 03:29:51PM -0800, Jonathan Nieder wrote:

> 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

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

[Jonathan Nieder]

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.

 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);

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

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

[Jeff King]

On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:

> > 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.

> 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.

> > 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

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

[Stefan Beller]

> 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?

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

[Jeff King]

On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote:

> > 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

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

[Jonathan Nieder]

Junio C Hamano wrote:

> 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

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]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.




>  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);

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]

Hi,

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.

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

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]

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)?

Sorry for the confusion,
Jonathan

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]

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

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]

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:
> 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

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]

On Thu, Dec 04, 2014 at 03:52:45PM -0800, Junio C Hamano wrote:

> 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

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]

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.

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]

On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:

> 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;
}

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]

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.
>
> 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.

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]

On Tue, Dec 09, 2014 at 10:43:52AM -0800, Junio C Hamano wrote:

> > 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

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

[Michael Haggerty]

On 12/04/2014 08:59 AM, Jeff King wrote:
> 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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> 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);
	}

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

[Jeff King]

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.

>  - 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.

> 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.

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]

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?

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]

On 02/13/2015 12:08 AM, Junio C Hamano wrote:
> 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

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]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> 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.

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]

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.

-Peff

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]

Jeff King <peff@peff.net> writes:

> 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.