On Wed, Jul 23, 2025 at 03:55:13AM -0400, Jeff King wrote:
> I dunno. We are reaching diminishing returns spending brainpower on a
> function that is meant to be somewhat quick-and-dirty.
OK, I clearly could not resist spending more brainpower on it. If we are
doing quick-and-dirty, why not just die()? The end result is the same,
but per my argument in the earlier iteration of the series, that means
we do not have to worry about cleaning up at all.
Like:
t/helper/test-delta.c | 49 ++++++++++++-----------------------
1 file changed, 16 insertions(+), 33 deletions(-)
diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 6bc787a474..769b68839d 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -21,39 +21,28 @@ int cmd__delta(int argc, const char **argv)
struct stat st;
void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;
unsigned long from_size, data_size, out_size;
- int ret = 1;
if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
fprintf(stderr, "usage: %s\n", usage_str);
return 1;
}
- fd = open(argv[2], O_RDONLY);
- if (fd < 0 || fstat(fd, &st)) {
- perror(argv[2]);
- return 1;
- }
+ fd = xopen(argv[2], O_RDONLY);
+ if (fstat(fd, &st) < 0)
+ die_errno("fstat(%s)", argv[2]);
from_size = st.st_size;
from_buf = xmalloc(from_size);
- if (read_in_full(fd, from_buf, from_size) < 0) {
- perror(argv[2]);
- close(fd);
- goto cleanup;
- }
+ if (read_in_full(fd, from_buf, from_size) < 0)
+ die_errno("read(%s)", argv[2]);
close(fd);
- fd = open(argv[3], O_RDONLY);
- if (fd < 0 || fstat(fd, &st)) {
- perror(argv[3]);
- goto cleanup;
- }
+ fd = xopen(argv[3], O_RDONLY);
+ if (fstat(fd, &st) < 0)
+ die_errno("fstat(%s)", argv[3]);
data_size = st.st_size;
data_buf = xmalloc(data_size);
- if (read_in_full(fd, data_buf, data_size) < 0) {
- perror(argv[3]);
- close(fd);
- goto cleanup;
- }
+ if (read_in_full(fd, data_buf, data_size) < 0)
+ die_errno("read(%s)", argv[3]);
close(fd);
if (argv[1][1] == 'd')
@@ -64,22 +53,16 @@ int cmd__delta(int argc, const char **argv)
out_buf = patch_delta(from_buf, from_size,
data_buf, data_size,
&out_size);
- if (!out_buf) {
- fprintf(stderr, "delta operation failed (returned NULL)\n");
- goto cleanup;
- }
+ if (!out_buf)
+ die("delta operation failed (returned NULL)");
- fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
- if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
- perror(argv[4]);
- goto cleanup;
- }
+ fd = xopen(argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
+ if (write_in_full(fd, out_buf, out_size) < 0)
+ die_errno("write(%s)", argv[4]);
- ret = 0;
-cleanup:
free(from_buf);
free(data_buf);
free(out_buf);
- return ret;
+ return 0;
}
I'd guess that one could probably go even further by replacing the bare
pointers with strbufs. And then you could use strbuf_read_file().
Incidentally that would also fix two minor bugs I noticed:
- passing st.st_size directly to xmalloc() is wrong, because of
truncation from off_t to size_t. This should use the xsize_t helper.
This is even a potential security vulnerability, but probably not
important in a test helper.
- likewise read_in_full() might return a non-negative value smaller
than the requested size (if the file racily changes and we get an
early EOF). But we only check whether we got a negative error value.
So we may read fewer bytes than expected and feed uninitialized
garbage to the delta code.
-Peff
