Hoyoung LeeRefactor the `test-tool delta` implementation to improve clarity and robustness:
- Replace raw pointer/length buffer handling with `strbuf` for `from` and `data` inputs. This simplifies the code and avoids potential issues such as: - off_t to size_t truncation when allocating large buffers - reading fewer bytes than expected without notice
- Add an explicit `close(fd)` after writing the output file to avoid leaking file descriptors and to properly detect and report close() errors.
- Use `die()`/`die_errno()` consistently to handle all failure paths, simplifying error handling.
This change not only cleans up the code but also improves safety and sets a better example for writing robust file-handling logic.
Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com> ---
Thank you very much for your detailed and thoughtful feedback.
I've taken your suggestions seriously and reflected them fully in the updated patch. Switching to strbuf_read_file() not only simplified the code but also helped me better understand subtle bugs related to buffer allocation and file reading. In particular, I learned a lot about the risks of integer truncation between off_t and size_t, as well as the importance of handling early EOF conditions correctly.
Also, your note on leaking file descriptors and the explanation around why we should avoid combining write_in_full() and close() in a single if condition was incredibly insightful. It deepened my understanding of how seemingly small patterns can lead to subtle bugs or bad practices, even in short-lived test helpers.
I sincerely appreciate your time and guidance — it helped me not only improve the patch but also grow as a contributor.
t/helper/test-delta.c | 95 +++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 62 deletions(-)
Show changes to t/helper/test-delta.c +33 −62
diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c index f5811e96ad..1c4322b7c0 100644 --- a/t/helper/test-delta.c +++ b/t/helper/test-delta.c @@ -11,76 +11,47 @@ #include "test-tool.h" #include "git-compat-util.h" #include "delta.h" +#include "strbuf.h" static const char usage_str[] = "test-tool delta (-d|-p) <from_file> <data_file> <out_file>"; int cmd__delta(int argc, const char **argv) { - int fd = -1; - struct stat st; - void *from_buf = NULL, *data_buf = NULL, *out_buf = NULL; - unsigned long from_size, data_size, out_size; - int ret = 1; + int fd; + struct strbuf from = STRBUF_INIT, data = STRBUF_INIT; + char *out_buf; + unsigned long out_size; - if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) { - fprintf(stderr, "usage: %s\n", usage_str); - return 1; - } + if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) + usage(usage_str); - fd = open(argv[2], O_RDONLY); - if (fd < 0 || fstat(fd, &st)) { - perror(argv[2]); - goto cleanup; - } - from_size = st.st_size; - from_buf = xmalloc(from_size); - if (read_in_full(fd, from_buf, from_size) < 0) { - perror(argv[2]); - goto cleanup; - } - close(fd); - - fd = open(argv[3], O_RDONLY); - if (fd < 0 || fstat(fd, &st)) { - perror(argv[3]); - goto cleanup; - } - data_size = st.st_size; - data_buf = xmalloc(data_size); - if (read_in_full(fd, data_buf, data_size) < 0) { - perror(argv[3]); - goto cleanup; - } - close(fd); + if (strbuf_read_file(&from, argv[2], 0) < 0) + die_errno("unable to read '%s'", argv[2]); + if (strbuf_read_file(&data, argv[3], 0) < 0) + die_errno("unable to read '%s'", argv[3]); if (argv[1][1] == 'd') - out_buf = diff_delta(from_buf, from_size, - data_buf, data_size, - &out_size, 0); - else - 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; - } - - 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; - } - - ret = 0; -cleanup: - free(from_buf); - free(data_buf); - free(out_buf); - - if (fd >= 0) - close(fd); - - return ret; + out_buf = diff_delta(from.buf, from.len, + data.buf, data.len, + &out_size, 0); + else + out_buf = patch_delta(from.buf, from.len, + data.buf, data.len, + &out_size); + + if (!out_buf) + die("delta operation failed (returned NULL)"); + + 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]); + if (close(fd) < 0) + die_errno("close(%s)", argv[4]); + + strbuf_release(&from); + strbuf_release(&data); + free(out_buf); + + return 0; }
-- 2.34.1
