Refactor 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@xxxxxxxxx>
---
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(-)
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
