From 00b7cbfcb36b6679c64ad0566c0fc251f7e979fe Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 19 May 2015 10:55:16 -0700 Subject: [PATCH 1/2] copy.c: make copy_fd() report its status silently When copy_fd() function encounters errors, it emits error messages itself, which makes it impossible for callers to take responsibility for reporting errors, especially when they want to ignore certain errors. Move the error reporting to its callers in preparation. - copy_file() and copy_file_with_time() by indirection get their own calls to error(). - hold_lock_file_for_append(), when told to die on error, used to exit(128) relying on the error message from copy_fd(), but now it does its own die() instead. Note that the callers that do not pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but fortunately there is none ;-) - filter_buffer_or_fd() has its own error() already, in addition to the message from copy_fd(), so this will change the output but arguably in a better way. Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ copy.c | 17 +++++++++++------ lockfile.c | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/cache.h b/cache.h index 22b7b81290..2981eec7f8 100644 --- a/cache.h +++ b/cache.h @@ -1482,9 +1482,13 @@ 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, ...); + +#define COPY_READ_ERROR (-2) +#define COPY_WRITE_ERROR (-3) extern int copy_fd(int ifd, int ofd); 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); extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg); extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg); diff --git a/copy.c b/copy.c index f2970ec462..574fa1f09d 100644 --- a/copy.c +++ b/copy.c @@ -7,13 +7,10 @@ int copy_fd(int ifd, int ofd) ssize_t len = xread(ifd, buffer, sizeof(buffer)); if (!len) break; - if (len < 0) { - return error("copy-fd: read returned %s", - strerror(errno)); - } + if (len < 0) + return COPY_READ_ERROR; if (write_in_full(ofd, buffer, len) < 0) - return error("copy-fd: write returned %s", - strerror(errno)); + return COPY_WRITE_ERROR; } return 0; } @@ -43,6 +40,14 @@ int copy_file(const char *dst, const char *src, int mode) return fdo; } status = copy_fd(fdi, fdo); + switch (status) { + case COPY_READ_ERROR: + error("copy-fd: read returned %s", strerror(errno)); + break; + case COPY_WRITE_ERROR: + error("copy-fd: write returned %s", strerror(errno)); + break; + } close(fdi); if (close(fdo) != 0) return error("%s: close error: %s", dst, strerror(errno)); diff --git a/lockfile.c b/lockfile.c index 4f16ee78ce..beba0ed480 100644 --- a/lockfile.c +++ b/lockfile.c @@ -206,7 +206,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) int save_errno = errno; if (flags & LOCK_DIE_ON_ERROR) - exit(128); + die("failed to prepare '%s' for appending", path); close(orig_fd); rollback_lock_file(lk); errno = save_errno; From 0c4dd67a048b39470b9b95912e4912fecc405a85 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 19 May 2015 11:08:23 -0700 Subject: [PATCH 2/2] filter_buffer_or_fd(): ignore EPIPE We are explicitly ignoring SIGPIPE, as we fully expect that the filter program may not read our output fully. Ignore EPIPE that may come from writing to it as well. A new test was stolen from Jeff's suggestion. Helped-by: Jeff King Signed-off-by: Junio C Hamano --- convert.c | 7 ++++++- t/t0021-conversion.sh | 10 ++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/convert.c b/convert.c index 9a5612e93d..f3bd3e93fb 100644 --- a/convert.c +++ b/convert.c @@ -356,9 +356,14 @@ static int filter_buffer_or_fd(int in, int out, void *data) sigchain_push(SIGPIPE, SIG_IGN); if (params->src) { - write_err = (write_in_full(child_process.in, params->src, params->size) < 0); + write_err = (write_in_full(child_process.in, + params->src, params->size) < 0); + if (errno == EPIPE) + write_err = 0; } else { write_err = copy_fd(params->fd, child_process.in); + if (write_err == COPY_WRITE_ERROR && errno == EPIPE) + write_err = 0; } if (close(child_process.in)) diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index ca7d2a630a..e0200b9f33 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -204,6 +204,16 @@ test_expect_success 'filtering large input to small output should use little mem GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB ' +test_expect_success 'filter that does not read is fine' ' + test-genrandom foo $((128 * 1024 + 1)) >big && + echo "big filter=epipe" >.gitattributes && + git config filter.epipe.clean "echo xyzzy" && + git add big && + git cat-file blob :big >actual && + echo xyzzy >expect && + test_cmp expect actual +' + test_expect_success EXPENSIVE 'filter large file' ' git config filter.largefile.smudge cat && git config filter.largefile.clean cat &&