{"thread":{"id":"63836","subject":"[PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","startedAt":"2025-07-24T09:33:45Z","lastAt":"2025-07-25T14:42:35Z","messageCount":5,"participants":["Hoyoung Lee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":1},"messages":[{"id":"522647","messageId":"20250724093327.2261615-1-lhywkd22@gmail.com","threadId":"63836","inReplyTo":null,"subject":"[PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-24T09:33:27Z","receivedAt":"2025-07-24T09:33:45Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"Refactor the `test-tool delta` implementation to improve clarity and robustness:\n\n- Replace raw pointer/length buffer handling with `strbuf` for `from` and `data` inputs.\n This simplifies the code and avoids potential issues such as:\n - off_t to size_t truncation when allocating large buffers\n - reading fewer bytes than expected without notice\n\n- Add an explicit `close(fd)` after writing the output file to avoid leaking file descriptors\n and to properly detect and report close() errors.\n\n- Use `die()`/`die_errno()` consistently to handle all failure paths, simplifying error handling.\n\nThis change not only cleans up the code but also improves safety and sets a better example\nfor writing robust file-handling logic.\n\nSigned-off-by: Hoyoung Lee \n---\n\nThank you very much for your detailed and thoughtful feedback.\n\nI've taken your suggestions seriously and reflected them fully in the updated patch.\nSwitching 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.\n\nAlso, 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.\n\nI sincerely appreciate your time and guidance — it helped me not only improve the patch but also grow as a contributor.\n\n t/helper/test-delta.c | 95 +++++++++++++++----------------------------\n 1 file changed, 33 insertions(+), 62 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex f5811e96ad..1c4322b7c0 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -11,76 +11,47 @@\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n #include \"delta.h\"\n+#include \"strbuf.h\"\n \n static const char usage_str[] =\n \t\"test-tool delta (-d|-p) \";\n \n int cmd__delta(int argc, const char **argv)\n {\n-\tint fd = -1;\n-\tstruct stat st;\n-\tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n-\tunsigned long from_size, data_size, out_size;\n-\tint ret = 1;\n+\tint fd;\n+ struct strbuf from = STRBUF_INIT, data = STRBUF_INIT;\n+ char *out_buf;\n+ unsigned long out_size;\n \n-\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n-\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n-\t\treturn 1;\n-\t}\n+\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\")))\n+ usage(usage_str);\n \n-\tfd = open(argv[2], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[2]);\n-\t\tgoto cleanup;\n-\t}\n-\tfrom_size = st.st_size;\n-\tfrom_buf = xmalloc(from_size);\n-\tif (read_in_full(fd, from_buf, from_size) < 0) {\n-\t\tperror(argv[2]);\n-\t\tgoto cleanup;\n-\t}\n-\tclose(fd);\n-\n-\tfd = open(argv[3], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[3]);\n-\t\tgoto cleanup;\n-\t}\n-\tdata_size = st.st_size;\n-\tdata_buf = xmalloc(data_size);\n-\tif (read_in_full(fd, data_buf, data_size) < 0) {\n-\t\tperror(argv[3]);\n-\t\tgoto cleanup;\n-\t}\n-\tclose(fd);\n+\tif (strbuf_read_file(&from, argv[2], 0) < 0)\n+ die_errno(\"unable to read '%s'\", argv[2]);\n+ if (strbuf_read_file(&data, argv[3], 0) < 0)\n+ die_errno(\"unable to read '%s'\", argv[3]);\n \n \tif (argv[1][1] == 'd')\n-\t\tout_buf = diff_delta(from_buf, from_size,\n-\t\t\t\t data_buf, data_size,\n-\t\t\t\t &out_size, 0);\n-\telse\n-\t\tout_buf = patch_delta(from_buf, from_size,\n-\t\t\t\t data_buf, data_size,\n-\t\t\t\t &out_size);\n-\tif (!out_buf) {\n-\t\tfprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n-\t\tgoto cleanup;\n-\t}\n-\n-\tfd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n-\tif (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n-\t\tperror(argv[4]);\n-\t\tgoto cleanup;\n-\t}\n-\n-\tret = 0;\n-cleanup:\n-\tfree(from_buf);\n-\tfree(data_buf);\n-\tfree(out_buf);\n-\n-\tif (fd >= 0)\n-\t\tclose(fd);\n-\n-\treturn ret;\n+ out_buf = diff_delta(from.buf, from.len,\n+ data.buf, data.len,\n+ &out_size, 0);\n+ else\n+ out_buf = patch_delta(from.buf, from.len,\n+ data.buf, data.len,\n+ &out_size);\n+\n+\tif (!out_buf)\n+ die(\"delta operation failed (returned NULL)\");\n+\n+\tfd = xopen(argv[4], O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+ if (write_in_full(fd, out_buf, out_size) < 0)\n+ die_errno(\"write(%s)\", argv[4]);\n+ if (close(fd) < 0)\n+ die_errno(\"close(%s)\", argv[4]);\n+\n+\tstrbuf_release(&from);\n+ strbuf_release(&data);\n+ free(out_buf);\n+\n+ return 0;\n }\n-- \n2.34.1\n\n"},{"id":"522696","messageId":"xmqqjz3xh34c.fsf@gitster.g","threadId":"63836","inReplyTo":"20250724093327.2261615-1-lhywkd22@gmail.com","subject":"Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-24T18:09:55Z","receivedAt":"2025-07-24T18:09:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hoyoung Lee writes:\n\n> diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\n> index f5811e96ad..1c4322b7c0 100644\n> --- a/t/helper/test-delta.c\n> +++ b/t/helper/test-delta.c\n> @@ -11,76 +11,47 @@\n> #include \"test-tool.h\"\n> #include \"git-compat-util.h\"\n> #include \"delta.h\"\n> +#include \"strbuf.h\"\n> \n> static const char usage_str[] =\n> \t\"test-tool delta (-d|-p) \";\n> \n> int cmd__delta(int argc, const char **argv)\n> {\n> -\tint fd = -1;\n> -\tstruct stat st;\n> -\tvoid *from_buf = NULL, *data_buf = NULL, *out_buf = NULL;\n> -\tunsigned long from_size, data_size, out_size;\n> -\tint ret = 1;\n> +\tint fd;\n> + struct strbuf from = STRBUF_INIT, data = STRBUF_INIT;\n> + char *out_buf;\n> + unsigned long out_size;\n\nMixed indentation. Make sure you indent with tabs, with tab-width\nset to 8. Not limited to the above code block.\n\n> -\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n> -\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n> -\t\treturn 1;\n> -\t}\n> +\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\")))\n> + usage(usage_str);\n\nNice.\n\n> -\tfd = open(argv[2], O_RDONLY);\n> -\tif (fd < 0 || fstat(fd, &st)) {\n> -\t\tperror(argv[2]);\n> -\t\tgoto cleanup;\n> -\t}\n> -\tfrom_size = st.st_size;\n> -\tfrom_buf = xmalloc(from_size);\n> -\tif (read_in_full(fd, from_buf, from_size) < 0) {\n> -\t\tperror(argv[2]);\n> -\t\tgoto cleanup;\n> -\t}\n> -\tclose(fd);\n> -\n> -\tfd = open(argv[3], O_RDONLY);\n> -\tif (fd < 0 || fstat(fd, &st)) {\n> -\t\tperror(argv[3]);\n> -\t\tgoto cleanup;\n> -\t}\n> -\tdata_size = st.st_size;\n> -\tdata_buf = xmalloc(data_size);\n> -\tif (read_in_full(fd, data_buf, data_size) < 0) {\n> -\t\tperror(argv[3]);\n> -\t\tgoto cleanup;\n> -\t}\n> -\tclose(fd);\n> +\tif (strbuf_read_file(&from, argv[2], 0) < 0)\n> + die_errno(\"unable to read '%s'\", argv[2]);\n> + if (strbuf_read_file(&data, argv[3], 0) < 0)\n> + die_errno(\"unable to read '%s'\", argv[3]);\n\nOK. from_buf/from_size has become strbuf from; data_buf/data_size\nhas become strbuf data. Very straight-forward and understandable.\n\n> \tif (argv[1][1] == 'd')\n> -\t\tout_buf = diff_delta(from_buf, from_size,\n> -\t\t\t\t data_buf, data_size,\n> -\t\t\t\t &out_size, 0);\n> -\telse\n> -\t\tout_buf = patch_delta(from_buf, from_size,\n> -\t\t\t\t data_buf, data_size,\n> -\t\t\t\t &out_size);\n> -\tif (!out_buf) {\n> -\t\tfprintf(stderr, \"delta operation failed (returned NULL)\\n\");\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tfd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);\n> -\tif (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {\n> -\t\tperror(argv[4]);\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tret = 0;\n> -cleanup:\n> -\tfree(from_buf);\n> -\tfree(data_buf);\n> -\tfree(out_buf);\n> -\n> -\tif (fd >= 0)\n> -\t\tclose(fd);\n> -\n> -\treturn ret;\n> + out_buf = diff_delta(from.buf, from.len,\n> + data.buf, data.len,\n> + &out_size, 0);\n> + else\n> + out_buf = patch_delta(from.buf, from.len,\n> + data.buf, data.len,\n> + &out_size);\n\nOK, quite straight-forward again.\n\n> +\tif (!out_buf)\n> + die(\"delta operation failed (returned NULL)\");\n\nNice again.\n\n> +\tfd = xopen(argv[4], O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> + if (write_in_full(fd, out_buf, out_size) < 0)\n> + die_errno(\"write(%s)\", argv[4]);\n> + if (close(fd) < 0)\n> + die_errno(\"close(%s)\", argv[4]);\n> +\n> +\tstrbuf_release(&from);\n> + strbuf_release(&data);\n> + free(out_buf);\n> +\n> + return 0;\n> }\n\nOK. Except for the whitespace breakage, I didn't spot anything\nglaringly wrong in the patch. Looking good.\n\nThanks.\n"},{"id":"522742","messageId":"20250725111530.GA3015021@coredump.intra.peff.net","threadId":"63836","inReplyTo":"xmqqjz3xh34c.fsf@gitster.g","subject":"Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-25T11:15:30Z","receivedAt":"2025-07-25T11:15:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:\n\n> OK. Except for the whitespace breakage, I didn't spot anything\n> glaringly wrong in the patch. Looking good.\n\nHmm. This looks like just a squash of the 3-patch series I sent earlier?\n\n(Sorry, it was in another part of the thread and you weren't on the cc).\n\nIf we are going to go in this direction, I'd prefer to leave it split\ninto the 3 more obvious patches with explanations.\n\n-Peff\n"},{"id":"522743","messageId":"20250725111634.GA3015176@coredump.intra.peff.net","threadId":"63836","inReplyTo":"20250725111530.GA3015021@coredump.intra.peff.net","subject":"Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-25T11:16:34Z","receivedAt":"2025-07-25T11:16:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 25, 2025 at 07:15:31AM -0400, Jeff King wrote:\n\n> On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:\n> \n> > OK. Except for the whitespace breakage, I didn't spot anything\n> > glaringly wrong in the patch. Looking good.\n> \n> Hmm. This looks like just a squash of the 3-patch series I sent earlier?\n> \n> (Sorry, it was in another part of the thread and you weren't on the cc).\n> \n> If we are going to go in this direction, I'd prefer to leave it split\n> into the 3 more obvious patches with explanations.\n\nProbably would have been helpful to actually provide a link:\n\n https://lore.kernel.org/git/20250723235929.GB592873@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"522749","messageId":"xmqq1pq45o2u.fsf@gitster.g","threadId":"63836","inReplyTo":"20250725111530.GA3015021@coredump.intra.peff.net","subject":"Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-25T14:42:33Z","receivedAt":"2025-07-25T14:42:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:\n>\n>> OK. Except for the whitespace breakage, I didn't spot anything\n>> glaringly wrong in the patch. Looking good.\n>\n> Hmm. This looks like just a squash of the 3-patch series I sent earlier?\n\nI think I have your variant in my tree, and the results from both\nvariants do match.\n"}]}