[PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[Hoyoung Lee]

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

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

Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[Junio C Hamano]

Hoyoung Lee <lhywkd22@gmail.com> writes:

> 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;

Mixed indentation.  Make sure you indent with tabs, with tab-width
set to 8.  Not limited to the above code block.

> -	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);

Nice.

> -	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]);

OK.  from_buf/from_size has become strbuf from; data_buf/data_size
has become strbuf data.  Very straight-forward and understandable.

>  	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);

OK, quite straight-forward again.

> +	if (!out_buf)
> +                die("delta operation failed (returned NULL)");

Nice again.

> +	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;
>  }

OK.  Except for the whitespace breakage, I didn't spot anything
glaringly wrong in the patch.  Looking good.

Thanks.

Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[Jeff King]

On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:

> OK.  Except for the whitespace breakage, I didn't spot anything
> glaringly wrong in the patch.  Looking good.

Hmm. This looks like just a squash of the 3-patch series I sent earlier?

(Sorry, it was in another part of the thread and you weren't on the cc).

If we are going to go in this direction, I'd prefer to leave it split
into the 3 more obvious patches with explanations.

-Peff

Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[Jeff King]

On Fri, Jul 25, 2025 at 07:15:31AM -0400, Jeff King wrote:

> On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:
> 
> > OK.  Except for the whitespace breakage, I didn't spot anything
> > glaringly wrong in the patch.  Looking good.
> 
> Hmm. This looks like just a squash of the 3-patch series I sent earlier?
> 
> (Sorry, it was in another part of the thread and you weren't on the cc).
> 
> If we are going to go in this direction, I'd prefer to leave it split
> into the 3 more obvious patches with explanations.

Probably would have been helpful to actually provide a link:

  https://lore.kernel.org/git/20250723235929.GB592873@coredump.intra.peff.net/

-Peff

Re: [PATCH v4 1/1] test-delta: simplify delta helper with strbuf and better cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jul 24, 2025 at 11:09:55AM -0700, Junio C Hamano wrote:
>
>> OK.  Except for the whitespace breakage, I didn't spot anything
>> glaringly wrong in the patch.  Looking good.
>
> Hmm. This looks like just a squash of the 3-patch series I sent earlier?

I think I have your variant in my tree, and the results from both
variants do match.