git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 24, 2025, 18:09 UTC
Message-ID
<xmqqjz3xh34c.fsf@gitster.g>
In-Reply-To
<20250724093327.2261615-1-lhywkd22@gmail.com>
Hoyoung Lee <lhywkd22@gmail.com> writes:
Show 24 quoted lines
> 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.

Show 6 quoted lines
> -	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.
Show 29 quoted lines
> -	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.

Show 36 quoted lines
>  	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.
Show 12 quoted lines
> +	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.
Previous: Hoyoung LeeNext: Jeff King
Message 2 of 5 in “test-delta: simplify delta helper with strbuf and better cleanup”
  1. 1/1 test-delta: simplify delta helper with strbuf and better cleanupHoyoung Lee, Jul 24, 2025
  2. Junio C HamanoJul 24, 2025
  3. Jeff KingJul 25, 2025
  4. Jeff KingJul 25, 2025
  5. Junio C HamanoJul 25, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.