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@xxxxxxxxxx>
Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
---
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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html