[Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Achu Luma]

In a following commit we are going to port code from
"t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
a new "t/unit-tests/t-hash.c" file using the recently added unit test
framework.

To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
we are going to need a new strbuf_addstrings() function that repeatedly
adds the same string a number of times to a buffer.

Such a strbuf_addstrings() function would already be useful in
"json-writer.c" and "builtin/submodule-helper.c" as both of these files
already have code that repeatedly adds the same string. So let's
introduce such a strbuf_addstrings() function in "strbuf.{c,h}" and use
it in both "json-writer.c" and "builtin/submodule-helper.c".

We use the "strbuf_addstrings" name as this way strbuf_addstr() and
strbuf_addstrings() would be similar for strings as strbuf_addch() and
strbuf_addchars() for characters.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 builtin/submodule--helper.c |  4 +---
 json-writer.c               |  5 +----
 strbuf.c                    | 11 +++++++++++
 strbuf.h                    |  5 +++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1..bed08af410 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -257,11 +257,9 @@ static void module_list_active(struct module_list *list)

 static char *get_up_path(const char *path)
 {
-	int i;
 	struct strbuf sb = STRBUF_INIT;

-	for (i = count_slashes(path); i; i--)
-		strbuf_addstr(&sb, "../");
+	strbuf_addstrings(&sb, "../", count_slashes(path));

 	/*
 	 * Check if 'path' ends with slash or not
diff --git a/json-writer.c b/json-writer.c
index 005c820aa4..25b9201f9c 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -46,10 +46,7 @@ static void append_quoted_string(struct strbuf *out, const char *in)

 static void indent_pretty(struct json_writer *jw)
 {
-	int k;
-
-	for (k = 0; k < jw->open_stack.len; k++)
-		strbuf_addstr(&jw->json, "  ");
+	strbuf_addstrings(&jw->json, "  ", jw->open_stack.len);
 }

 /*
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..eb2b3299ce 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }

+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
+{
+	size_t len = strlen(s);
+	if (unsigned_mult_overflows(len, n))
+		die("you want to use way too much memory");
+	strbuf_grow(sb, len * n);
+	for (size_t i = 0; i < n; i++)
+		memcpy(sb->buf + sb->len + len * i, s, len);
+	strbuf_setlen(sb, sb->len + len * n);
+}
+
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
 {
 	strbuf_grow(sb, sb2->len);
diff --git a/strbuf.h b/strbuf.h
index e959caca87..0fb1b5e81e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
 	strbuf_add(sb, s, strlen(s));
 }

+/**
+ * Add a NUL-terminated string the specified number of times to the buffer.
+ */
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
+
 /**
  * Copy the contents of another buffer at the end of the current one.
  */
--
2.43.0.windows.1

[Outreachy][PATCH 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Achu Luma]

In the recent codebase update (8bf6fbd (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.

This commit migrates the unit tests for hash functionality from the
legacy approach using the test-tool command `test-tool sha1`and
`test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
new unit testing framework (t/unit-tests/test-lib.h). Porting
t0013-sha1dc.sh is left for later.

The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 Makefile              |  2 +-
 t/helper/test-tool.c  |  1 -
 t/helper/test-tool.h  |  1 -
 t/t0015-hash.sh       | 56 --------------------------------
 t/unit-tests/t-hash.c | 75 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 76 insertions(+), 59 deletions(-)
 delete mode 100755 t/t0015-hash.sh
 create mode 100644 t/unit-tests/t-hash.c

diff --git a/Makefile b/Makefile
index 78e874099d..b73d2270f9 100644
--- a/Makefile
+++ b/Makefile
@@ -847,7 +847,6 @@ TEST_BUILTINS_OBJS += test-run-command.o
 TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
 TEST_BUILTINS_OBJS += test-serve-v2.o
 TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
 TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-simple-ipc.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
@@ -1346,6 +1345,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-hash
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4..7bfbb75c9b 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
 	{ "serve-v2", cmd__serve_v2 },
 	{ "sha1", cmd__sha1 },
 	{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
-	{ "sha256", cmd__sha256 },
 	{ "sigchain", cmd__sigchain },
 	{ "simple-ipc", cmd__simple_ipc },
 	{ "strcmp-offset", cmd__strcmp_offset },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf5..8139c9664d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
 int cmd__sha1(int argc, const char **argv);
 int cmd__sha1_is_sha1dc(int argc, const char **argv);
 int cmd__oid_array(int argc, const char **argv);
-int cmd__sha256(int argc, const char **argv);
 int cmd__sigchain(int argc, const char **argv);
 int cmd__simple_ipc(int argc, const char **argv);
 int cmd__strcmp_offset(int argc, const char **argv);
diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 0a087a1983..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,56 +0,0 @@
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-1 hash values' '
-	test-tool sha1 </dev/null >actual &&
-	grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
-	printf "a" | test-tool sha1 >actual &&
-	grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
-	printf "abc" | test-tool sha1 >actual &&
-	grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
-	printf "message digest" | test-tool sha1 >actual &&
-	grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
-	grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha1 >actual &&
-	grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
-	printf "blob 0\0" | test-tool sha1 >actual &&
-	grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
-	printf "blob 3\0abc" | test-tool sha1 >actual &&
-	grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
-	printf "tree 0\0" | test-tool sha1 >actual &&
-	grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
-'
-
-test_expect_success 'test basic SHA-256 hash values' '
-	test-tool sha256 </dev/null >actual &&
-	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
-	printf "a" | test-tool sha256 >actual &&
-	grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
-	printf "abc" | test-tool sha256 >actual &&
-	grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
-	printf "message digest" | test-tool sha256 >actual &&
-	grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
-	grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
-	# Try to exercise the chunking code by turning autoflush on.
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
-	perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
-	printf "blob 0\0" | test-tool sha256 >actual &&
-	grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
-	printf "blob 3\0abc" | test-tool sha256 >actual &&
-	grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
-	printf "tree 0\0" | test-tool sha256 >actual &&
-	grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
new file mode 100644
index 0000000000..b512e64bf1
--- /dev/null
+++ b/t/unit-tests/t-hash.c
@@ -0,0 +1,75 @@
+#include "test-lib.h"
+#include "hash-ll.h"
+#include "hex.h"
+#include "strbuf.h"
+
+static void check_hash_data(const void *data, size_t data_length, const char *expected, int algo) {
+	git_hash_ctx ctx;
+	unsigned char hash[GIT_MAX_HEXSZ];
+	const struct git_hash_algo *algop = &hash_algos[algo];
+
+	if (!check(!!data)) {
+    		test_msg("Error: No data provided when expecting: %s", expected);
+    		return;
+	}
+
+	algop->init_fn(&ctx);
+	algop->update_fn(&ctx, data, data_length);
+	algop->final_fn(hash, &ctx);
+
+	check_str(hash_to_hex_algop(hash, algop), expected);
+}
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA1_STR(data, expected) \
+    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
+        	"SHA1 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA1_LITERAL(literal, expected) \
+    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
+        	"SHA1 (%s) works", #literal)
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA256_STR(data, expected) \
+    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+        	"SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+        	"SHA256 (%s) works", #literal)
+
+int cmd_main(int argc, const char **argv) {
+	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+	struct strbuf alphabet_100000 = STRBUF_INIT;
+
+	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
+
+	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
+	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
+	TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
+	TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
+	TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
+	TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
+	TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
+	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
+	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
+
+	TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+	TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+	TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+	TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+	TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+	TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+	TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+	TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+	TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+	TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
+	strbuf_release(&aaaaaaaaaa_100000);
+	strbuf_release(&alphabet_100000);
+
+	return test_done();
+}
--
2.43.0.windows.1

Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Junio C Hamano]

Achu Luma <ach.lumap@gmail.com> writes:

> In a following commit we are going to port code from
> "t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
> a new "t/unit-tests/t-hash.c" file using the recently added unit test
> framework.
>
> To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
> we are going to need a new strbuf_addstrings() function that repeatedly
> adds the same string a number of times to a buffer.

We do not need to call such a function "addstrings", though.  The
name on the subject line made me expect a varargs function:

 (bad)	strbuf_addstrings(&sb, "foo", "bar", "baz", NULL);

It would have been clearer if the name hinted what it does, clearer
than just a single "s" that says it is talking about plural.  What
would be a good name that hints "n times add a single same string"?
I dunno.

I also would have expected that the order of parameters are
repeat-count followed by what gets repeated.

Having said all of the above, we already have "addchars" that is
equally strange, so let's let it pass ;-).

> diff --git a/strbuf.c b/strbuf.c
> index 7827178d8e..eb2b3299ce 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
>  	strbuf_setlen(sb, sb->len + len);
>  }
>
> +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
> +{
> +	size_t len = strlen(s);

Let's have a blank line here to separate decls from the first
statement.

> +	if (unsigned_mult_overflows(len, n))
> +		die("you want to use way too much memory");
> +	strbuf_grow(sb, len * n);

The error message given by

	strbuf_grow(sb, st_mult(len, n));

would be equally informative and takes only a single line.

> +	for (size_t i = 0; i < n; i++)
> +		memcpy(sb->buf + sb->len + len * i, s, len);

Wouldn't it be sufficient to run strbuf_add() n times at this point,
as we have already called strbuf_grow() to avoid repeated
reallocation?  Repeated manual memcpy() that involves manual offset
computation makes me nervous.

> +	strbuf_setlen(sb, sb->len + len * n);
> +}
> +
>  void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
>  {
>  	strbuf_grow(sb, sb2->len);
> diff --git a/strbuf.h b/strbuf.h
> index e959caca87..0fb1b5e81e 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
>  	strbuf_add(sb, s, strlen(s));
>  }
>
> +/**
> + * Add a NUL-terminated string the specified number of times to the buffer.
> + */
> +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
> +
>  /**
>   * Copy the contents of another buffer at the end of the current one.
>   */
> --
> 2.43.0.windows.1

Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Christian Couder]

On Mon, Feb 26, 2024 at 5:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In a following commit we are going to port code from
> > "t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
> > a new "t/unit-tests/t-hash.c" file using the recently added unit test
> > framework.
> >
> > To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
> > we are going to need a new strbuf_addstrings() function that repeatedly
> > adds the same string a number of times to a buffer.
>
> We do not need to call such a function "addstrings", though.  The
> name on the subject line made me expect a varargs function:
>
>  (bad)  strbuf_addstrings(&sb, "foo", "bar", "baz", NULL);
>
> It would have been clearer if the name hinted what it does, clearer
> than just a single "s" that says it is talking about plural.  What
> would be a good name that hints "n times add a single same string"?
> I dunno.
>
> I also would have expected that the order of parameters are
> repeat-count followed by what gets repeated.
>
> Having said all of the above, we already have "addchars" that is
> equally strange, so let's let it pass ;-).

Yeah, we thought about naming it strbuf_repeatstr() first, but then
seeing addchars() I thought it would be better to imitate it.

> > diff --git a/strbuf.c b/strbuf.c
> > index 7827178d8e..eb2b3299ce 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -302,6 +302,17 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
> >       strbuf_setlen(sb, sb->len + len);
> >  }
> >
> > +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
> > +{
> > +     size_t len = strlen(s);
>
> Let's have a blank line here to separate decls from the first
> statement.

Yeah, right.

> > +     if (unsigned_mult_overflows(len, n))
> > +             die("you want to use way too much memory");
> > +     strbuf_grow(sb, len * n);
>
> The error message given by
>
>         strbuf_grow(sb, st_mult(len, n));
>
> would be equally informative and takes only a single line.

It seems that the pattern in strbuf.c, for example in strbuf_splice()
and strbuf_vinsertf(), is to do a check first using an *_overflows()
function, die if the check fails, and only then call strbuf_grow(). So
we did the same. I am fine with using your suggestion though.

> > +     for (size_t i = 0; i < n; i++)
> > +             memcpy(sb->buf + sb->len + len * i, s, len);
>
> Wouldn't it be sufficient to run strbuf_add() n times at this point,
> as we have already called strbuf_grow() to avoid repeated
> reallocation?

Unfortunately strbuf_add() calls strbuf_grow() itself which is not
needed as we have already called strbuf_grow() to avoid repeated
reallocation.

>  Repeated manual memcpy() that involves manual offset
> computation makes me nervous.

I would have prefered to avoid it too, but didn't find a good way to do it.

> > +     strbuf_setlen(sb, sb->len + len * n);
> > +}

Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Unfortunately strbuf_add() calls strbuf_grow() itself which is not
> needed as we have already called strbuf_grow() to avoid repeated
> reallocation.

Why is it unfortunate?  If the current allocation is sufficient, it
is a cheap no-op, isn't it (if not, we should make it so)?

Re: [Outreachy][PATCH 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Christian Couder]

On Mon, Feb 26, 2024 at 7:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > Unfortunately strbuf_add() calls strbuf_grow() itself which is not
> > needed as we have already called strbuf_grow() to avoid repeated
> > reallocation.
>
> Why is it unfortunate?  If the current allocation is sufficient, it
> is a cheap no-op, isn't it (if not, we should make it so)?

Yeah, I agree that we don't need to be extra efficient and calling
strbuf_grow() doesn't add much overhead. So let's just use
strbuf_add() repeatedly after calling it once.

[Outreachy][PATCH v2 1/2] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Achu Luma]

In a following commit we are going to port code from
"t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
a new "t/unit-tests/t-hash.c" file using the recently added unit test
framework.

To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
we are going to need a new strbuf_addstrings() function that repeatedly
adds the same string a number of times to a buffer.

Such a strbuf_addstrings() function would already be useful in
"json-writer.c" and "builtin/submodule-helper.c" as both of these files
already have code that repeatedly adds the same string. So let's
introduce such a strbuf_addstrings() function in "strbuf.{c,h}" and use
it in both "json-writer.c" and "builtin/submodule-helper.c".

We use the "strbuf_addstrings" name as this way strbuf_addstr() and
strbuf_addstrings() would be similar for strings as strbuf_addch() and
strbuf_addchars() for characters.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The changes between version 1 and version 2 are the following:
 - Remove memcpy() that involves manual offset computation
 - Use strbuf_add() repeatedly after calling strbuf_grow() once to avoid
   repeated allocation.

 Thanks to Junio for pointing this out.
 Here is a diff between v1 and v2:

 -       size_t len = strlen(s);
 -       if (unsigned_mult_overflows(len, n))
 -               die("you want to use way too much memory");
 -       strbuf_grow(sb, len * n);
 -       for (size_t i = 0; i < n; i++)
 -               memcpy(sb->buf + sb->len + len * i, s, len);
 -       strbuf_setlen(sb, sb->len + len * n);
 +       size_t len = strlen(s);
 +
 +       if (unsigned_mult_overflows(len, n))
 +               die("you want to use way too much memory");
 +       strbuf_grow(sb, len * n);
 +       for (size_t i = 0; i < n; i++)
 +               strbuf_add(sb, s, len);

 builtin/submodule--helper.c |  4 +---
 json-writer.c               |  5 +----
 strbuf.c                    | 10 ++++++++++
 strbuf.h                    |  5 +++++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1..bed08af410 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -257,11 +257,9 @@ static void module_list_active(struct module_list *list)

 static char *get_up_path(const char *path)
 {
-	int i;
 	struct strbuf sb = STRBUF_INIT;

-	for (i = count_slashes(path); i; i--)
-		strbuf_addstr(&sb, "../");
+	strbuf_addstrings(&sb, "../", count_slashes(path));

 	/*
 	 * Check if 'path' ends with slash or not
diff --git a/json-writer.c b/json-writer.c
index 005c820aa4..25b9201f9c 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -46,10 +46,7 @@ static void append_quoted_string(struct strbuf *out, const char *in)

 static void indent_pretty(struct json_writer *jw)
 {
-	int k;
-
-	for (k = 0; k < jw->open_stack.len; k++)
-		strbuf_addstr(&jw->json, "  ");
+	strbuf_addstrings(&jw->json, "  ", jw->open_stack.len);
 }

 /*
diff --git a/strbuf.c b/strbuf.c
index 7827178d8e..f4282b70ad 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -302,6 +302,16 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }

+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
+{
+       size_t len = strlen(s);
+
+       if (unsigned_mult_overflows(len, n))
+               die("you want to use way too much memory");
+       strbuf_grow(sb, len * n);
+       for (size_t i = 0; i < n; i++)
+               strbuf_add(sb, s, len);
+}
+
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
 {
 	strbuf_grow(sb, sb2->len);
diff --git a/strbuf.h b/strbuf.h
index e959caca87..0fb1b5e81e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
 	strbuf_add(sb, s, strlen(s));
 }

+/**
+ * Add a NUL-terminated string the specified number of times to the buffer.
+ */
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
+
 /**
  * Copy the contents of another buffer at the end of the current one.
  */
--
2.43.0.windows.1

[Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Achu Luma]

In the recent codebase update (8bf6fbd (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.

This commit migrates the unit tests for hash functionality from the
legacy approach using the test-tool command `test-tool sha1`and
`test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
new unit testing framework (t/unit-tests/test-lib.h). Porting
t0013-sha1dc.sh is left for later.

The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The change between version 1 and version 2 is:
 - Deleted t/helper/test-sha256.c

 Here is a diff between v1 and v2:

 -#include "test-tool.h"
 -#include "hash-ll.h"
 -
 -int cmd__sha256(int ac, const char **av)
 -{
 -       return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
 -}

 Makefile               |  2 +-
 t/helper/test-sha256.c |  7 ----
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0015-hash.sh        | 56 -------------------------------
 t/unit-tests/t-hash.c  | 75 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 66 deletions(-)
 delete mode 100644 t/helper/test-sha256.c
 delete mode 100755 t/t0015-hash.sh
 create mode 100644 t/unit-tests/t-hash.c

diff --git a/Makefile b/Makefile
index 4e255c81f2..c85f24f813 100644
--- a/Makefile
+++ b/Makefile
@@ -847,7 +847,6 @@ TEST_BUILTINS_OBJS += test-run-command.o
 TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
 TEST_BUILTINS_OBJS += test-serve-v2.o
 TEST_BUILTINS_OBJS += test-sha1.o
-TEST_BUILTINS_OBJS += test-sha256.o
 TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-simple-ipc.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
@@ -1347,6 +1346,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-prio-queue
+UNIT_TEST_PROGRAMS += t-hash
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-sha256.c b/t/helper/test-sha256.c
deleted file mode 100644
index 08cf149001..0000000000
--- a/t/helper/test-sha256.c
+++ /dev/null
@@ -1,7 +0,0 @@
-#include "test-tool.h"
-#include "hash-ll.h"
-
-int cmd__sha256(int ac, const char **av)
-{
-	return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 482a1e58a4..7bfbb75c9b 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
 	{ "serve-v2", cmd__serve_v2 },
 	{ "sha1", cmd__sha1 },
 	{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
-	{ "sha256", cmd__sha256 },
 	{ "sigchain", cmd__sigchain },
 	{ "simple-ipc", cmd__simple_ipc },
 	{ "strcmp-offset", cmd__strcmp_offset },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index b1be7cfcf5..8139c9664d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
 int cmd__sha1(int argc, const char **argv);
 int cmd__sha1_is_sha1dc(int argc, const char **argv);
 int cmd__oid_array(int argc, const char **argv);
-int cmd__sha256(int argc, const char **argv);
 int cmd__sigchain(int argc, const char **argv);
 int cmd__simple_ipc(int argc, const char **argv);
 int cmd__strcmp_offset(int argc, const char **argv);
diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 0a087a1983..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,56 +0,0 @@
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-1 hash values' '
-	test-tool sha1 </dev/null >actual &&
-	grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
-	printf "a" | test-tool sha1 >actual &&
-	grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
-	printf "abc" | test-tool sha1 >actual &&
-	grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
-	printf "message digest" | test-tool sha1 >actual &&
-	grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
-	grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha1 >actual &&
-	grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
-	printf "blob 0\0" | test-tool sha1 >actual &&
-	grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
-	printf "blob 3\0abc" | test-tool sha1 >actual &&
-	grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
-	printf "tree 0\0" | test-tool sha1 >actual &&
-	grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
-'
-
-test_expect_success 'test basic SHA-256 hash values' '
-	test-tool sha256 </dev/null >actual &&
-	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
-	printf "a" | test-tool sha256 >actual &&
-	grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
-	printf "abc" | test-tool sha256 >actual &&
-	grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
-	printf "message digest" | test-tool sha256 >actual &&
-	grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
-	grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
-	# Try to exercise the chunking code by turning autoflush on.
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
-	perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
-	printf "blob 0\0" | test-tool sha256 >actual &&
-	grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
-	printf "blob 3\0abc" | test-tool sha256 >actual &&
-	grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
-	printf "tree 0\0" | test-tool sha256 >actual &&
-	grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
new file mode 100644
index 0000000000..b512e64bf1
--- /dev/null
+++ b/t/unit-tests/t-hash.c
@@ -0,0 +1,75 @@
+#include "test-lib.h"
+#include "hash-ll.h"
+#include "hex.h"
+#include "strbuf.h"
+
+static void check_hash_data(const void *data, size_t data_length, const char *expected, int algo) {
+	git_hash_ctx ctx;
+	unsigned char hash[GIT_MAX_HEXSZ];
+	const struct git_hash_algo *algop = &hash_algos[algo];
+
+	if (!check(!!data)) {
+    		test_msg("Error: No data provided when expecting: %s", expected);
+    		return;
+	}
+
+	algop->init_fn(&ctx);
+	algop->update_fn(&ctx, data, data_length);
+	algop->final_fn(hash, &ctx);
+
+	check_str(hash_to_hex_algop(hash, algop), expected);
+}
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA1_STR(data, expected) \
+    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
+        	"SHA1 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA1_LITERAL(literal, expected) \
+    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
+        	"SHA1 (%s) works", #literal)
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA256_STR(data, expected) \
+    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+        	"SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+        	"SHA256 (%s) works", #literal)
+
+int cmd_main(int argc, const char **argv) {
+	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+	struct strbuf alphabet_100000 = STRBUF_INIT;
+
+	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
+
+	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
+	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
+	TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
+	TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
+	TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
+	TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
+	TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
+	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
+	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
+
+	TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+	TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+	TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+	TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+	TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+	TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+	TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+	TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+	TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+	TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
+	strbuf_release(&aaaaaaaaaa_100000);
+	strbuf_release(&alphabet_100000);
+
+	return test_done();
+}
--
2.43.0.windows.1

Re: [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Christian Couder]

On Thu, Feb 29, 2024 at 6:41 AM Achu Luma <ach.lumap@gmail.com> wrote:
>
> In the recent codebase update (8bf6fbd (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for hash functionality from the
> legacy approach using the test-tool command `test-tool sha1`and
> `test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
> new unit testing framework (t/unit-tests/test-lib.h). Porting
> t0013-sha1dc.sh is left for later.
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>  The change between version 1 and version 2 is:
>  - Deleted t/helper/test-sha256.c
>
>  Here is a diff between v1 and v2:

When sending a patch series, even a small one, I think it's better to
have a cover letter (using for example `git format-patch
--cover-letter ...`) and list all the changes from the previous
version in the cover letter, rather than listing some changes in each
patch. Also, instead of `git diff`, `git range-diff` can be used and
its output can be put into the cover letter too.

I don't think it's worth resending the series just for this though.

Maybe I missed it but it seems that this patch and patch 1/2
(https://lore.kernel.org/git/20240229054004.3807-1-ach.lumap@gmail.com/)
in the series fell into the cracks.

Re: [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 11717 bytes --]

On Thu, Feb 29, 2024 at 06:40:03AM +0100, Achu Luma wrote:
> In the recent codebase update (8bf6fbd (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
> 
> This commit migrates the unit tests for hash functionality from the
> legacy approach using the test-tool command `test-tool sha1`and

Nit: s/sha1`and/sha1` and/.

> `test-tool sha256` in helper/test-sha256.c and helper/test-sha1.c to the
> new unit testing framework (t/unit-tests/test-lib.h). Porting
> t0013-sha1dc.sh is left for later.
> 
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>  The change between version 1 and version 2 is:
>  - Deleted t/helper/test-sha256.c
> 
>  Here is a diff between v1 and v2:
> 
>  -#include "test-tool.h"
>  -#include "hash-ll.h"
>  -
>  -int cmd__sha256(int ac, const char **av)
>  -{
>  -       return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
>  -}
> 
>  Makefile               |  2 +-
>  t/helper/test-sha256.c |  7 ----
>  t/helper/test-tool.c   |  1 -
>  t/helper/test-tool.h   |  1 -
>  t/t0015-hash.sh        | 56 -------------------------------
>  t/unit-tests/t-hash.c  | 75 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 76 insertions(+), 66 deletions(-)
>  delete mode 100644 t/helper/test-sha256.c
>  delete mode 100755 t/t0015-hash.sh
>  create mode 100644 t/unit-tests/t-hash.c
> 
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..c85f24f813 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -847,7 +847,6 @@ TEST_BUILTINS_OBJS += test-run-command.o
>  TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
>  TEST_BUILTINS_OBJS += test-serve-v2.o
>  TEST_BUILTINS_OBJS += test-sha1.o
> -TEST_BUILTINS_OBJS += test-sha256.o
>  TEST_BUILTINS_OBJS += test-sigchain.o
>  TEST_BUILTINS_OBJS += test-simple-ipc.o
>  TEST_BUILTINS_OBJS += test-strcmp-offset.o
> @@ -1347,6 +1346,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool
>  UNIT_TEST_PROGRAMS += t-strbuf
>  UNIT_TEST_PROGRAMS += t-ctype
>  UNIT_TEST_PROGRAMS += t-prio-queue
> +UNIT_TEST_PROGRAMS += t-hash
>  UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
>  UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
>  UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-sha256.c b/t/helper/test-sha256.c
> deleted file mode 100644
> index 08cf149001..0000000000
> --- a/t/helper/test-sha256.c
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -#include "test-tool.h"
> -#include "hash-ll.h"
> -
> -int cmd__sha256(int ac, const char **av)
> -{
> -	return cmd_hash_impl(ac, av, GIT_HASH_SHA256);
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 482a1e58a4..7bfbb75c9b 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
>  	{ "serve-v2", cmd__serve_v2 },
>  	{ "sha1", cmd__sha1 },

I'm a bit surprised that the `cmd_sha1` subcommand still exists. I would
have expected it to be removed as part of this commit, as well, judging
by the commit message.

In any case I think it would be easier to review if the "sha1" and
"sha256" refactorings were split out into two separate patches.

Patrick

>  	{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
> -	{ "sha256", cmd__sha256 },
>  	{ "sigchain", cmd__sigchain },
>  	{ "simple-ipc", cmd__simple_ipc },
>  	{ "strcmp-offset", cmd__strcmp_offset },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index b1be7cfcf5..8139c9664d 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
>  int cmd__sha1(int argc, const char **argv);
>  int cmd__sha1_is_sha1dc(int argc, const char **argv);
>  int cmd__oid_array(int argc, const char **argv);
> -int cmd__sha256(int argc, const char **argv);
>  int cmd__sigchain(int argc, const char **argv);
>  int cmd__simple_ipc(int argc, const char **argv);
>  int cmd__strcmp_offset(int argc, const char **argv);
> diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
> deleted file mode 100755
> index 0a087a1983..0000000000
> --- a/t/t0015-hash.sh
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -#!/bin/sh
> -
> -test_description='test basic hash implementation'
> -
> -TEST_PASSES_SANITIZE_LEAK=true
> -. ./test-lib.sh
> -
> -test_expect_success 'test basic SHA-1 hash values' '
> -	test-tool sha1 </dev/null >actual &&
> -	grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
> -	printf "a" | test-tool sha1 >actual &&
> -	grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
> -	printf "abc" | test-tool sha1 >actual &&
> -	grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
> -	printf "message digest" | test-tool sha1 >actual &&
> -	grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
> -	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
> -	grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
> -	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
> -		test-tool sha1 >actual &&
> -	grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
> -	printf "blob 0\0" | test-tool sha1 >actual &&
> -	grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
> -	printf "blob 3\0abc" | test-tool sha1 >actual &&
> -	grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
> -	printf "tree 0\0" | test-tool sha1 >actual &&
> -	grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
> -'
> -
> -test_expect_success 'test basic SHA-256 hash values' '
> -	test-tool sha256 </dev/null >actual &&
> -	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
> -	printf "a" | test-tool sha256 >actual &&
> -	grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
> -	printf "abc" | test-tool sha256 >actual &&
> -	grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
> -	printf "message digest" | test-tool sha256 >actual &&
> -	grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
> -	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
> -	grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
> -	# Try to exercise the chunking code by turning autoflush on.
> -	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
> -		test-tool sha256 >actual &&
> -	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
> -	perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
> -		test-tool sha256 >actual &&
> -	grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
> -	printf "blob 0\0" | test-tool sha256 >actual &&
> -	grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
> -	printf "blob 3\0abc" | test-tool sha256 >actual &&
> -	grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
> -	printf "tree 0\0" | test-tool sha256 >actual &&
> -	grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
> -'
> -
> -test_done
> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> new file mode 100644
> index 0000000000..b512e64bf1
> --- /dev/null
> +++ b/t/unit-tests/t-hash.c
> @@ -0,0 +1,75 @@
> +#include "test-lib.h"
> +#include "hash-ll.h"
> +#include "hex.h"
> +#include "strbuf.h"
> +
> +static void check_hash_data(const void *data, size_t data_length, const char *expected, int algo) {
> +	git_hash_ctx ctx;
> +	unsigned char hash[GIT_MAX_HEXSZ];
> +	const struct git_hash_algo *algop = &hash_algos[algo];
> +
> +	if (!check(!!data)) {
> +    		test_msg("Error: No data provided when expecting: %s", expected);
> +    		return;
> +	}
> +
> +	algop->init_fn(&ctx);
> +	algop->update_fn(&ctx, data, data_length);
> +	algop->final_fn(hash, &ctx);
> +
> +	check_str(hash_to_hex_algop(hash, algop), expected);
> +}
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> +#define TEST_SHA1_STR(data, expected) \
> +    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
> +        	"SHA1 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA1_LITERAL(literal, expected) \
> +    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> +        	"SHA1 (%s) works", #literal)
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> +#define TEST_SHA256_STR(data, expected) \
> +    	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> +        	"SHA256 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA256_LITERAL(literal, expected) \
> +    	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> +        	"SHA256 (%s) works", #literal)
> +
> +int cmd_main(int argc, const char **argv) {
> +	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
> +	struct strbuf alphabet_100000 = STRBUF_INIT;
> +
> +	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
> +	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
> +
> +	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
> +	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
> +	TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
> +	TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
> +	TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
> +	TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
> +	TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
> +	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
> +	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
> +
> +	TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
> +	TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
> +	TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
> +	TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
> +	TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
> +	TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
> +	TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
> +	TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
> +	TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
> +	TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
> +
> +	strbuf_release(&aaaaaaaaaa_100000);
> +	strbuf_release(&alphabet_100000);
> +
> +	return test_done();
> +}
> --
> 2.43.0.windows.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Christian Couder]

On Tue, Mar 26, 2024 at 12:39 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Feb 29, 2024 at 06:40:03AM +0100, Achu Luma wrote:

> > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> > index 482a1e58a4..7bfbb75c9b 100644
> > --- a/t/helper/test-tool.c
> > +++ b/t/helper/test-tool.c
> > @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
> >       { "serve-v2", cmd__serve_v2 },
> >       { "sha1", cmd__sha1 },
>
> I'm a bit surprised that the `cmd_sha1` subcommand still exists. I would
> have expected it to be removed as part of this commit, as well, judging
> by the commit message.

I think that's because it is used in t0013-sha1dc.sh not just
t0015-hash.sh, and porting t0013-sha1dc.sh is complex because it
relies on `test-tool sha1` dying when it is used on a file created to
contain the known sha1 attack.

But I agree this should be better explained in the commit message.

> In any case I think it would be easier to review if the "sha1" and
> "sha256" refactorings were split out into two separate patches.

Thanks for your review!

Re: [Outreachy][PATCH v2 2/2] Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c

[Ghanshyam Thakkar]

On Thu, 29 Feb 2024, Achu Luma <ach.lumap@gmail.com> wrote:
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 482a1e58a4..7bfbb75c9b 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -73,7 +73,6 @@ static struct test_cmd cmds[] = {
>  	{ "serve-v2", cmd__serve_v2 },
>  	{ "sha1", cmd__sha1 },
>  	{ "sha1-is-sha1dc", cmd__sha1_is_sha1dc },
> -	{ "sha256", cmd__sha256 },
>  	{ "sigchain", cmd__sigchain },
>  	{ "simple-ipc", cmd__simple_ipc },
>  	{ "strcmp-offset", cmd__strcmp_offset },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index b1be7cfcf5..8139c9664d 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -66,7 +66,6 @@ int cmd__serve_v2(int argc, const char **argv);
>  int cmd__sha1(int argc, const char **argv);
>  int cmd__sha1_is_sha1dc(int argc, const char **argv);
>  int cmd__oid_array(int argc, const char **argv);
> -int cmd__sha256(int argc, const char **argv);

Removing 'test-tool sha256' would break many tests in t53** when
GIT_TEST_DEFAULT_HASH=sha256, because it is used in pack_trailer():t/lib-pack.sh.

```
    pack_trailer () {
	    test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
	    cat trailer.tmp >>"$1" &&
	    rm -f trailer.tmp
    }
```

Also, I am planning to take over this series, so I will take care of this.

Thanks.

[PATCH v3 0/3] Port t0015-hash to the unit testing framework

[Ghanshyam Thakkar]

Changes in v3:
- use st_mult() instead of unsigned_mult_overflows(), as suggested by
  Junio.
- split migration of SHA-1 and SHA-256 in different patches, as
  suggested by Patrick.
- Revert removal of 'sha256' subcommand of test-tool due to existing
  dependencies on it.
- Fix some formatting

CI for v3: https://github.com/spectre10/git/actions/runs/9216124753

Ghanshyam Thakkar (3):
  strbuf: introduce strbuf_addstrings() to repeatedly add a string
  t/: port helper/test-sha1.c to unit-tests/t-hash.c
  t/: port helper/test-sha256.c to unit-tests/t-hash.c

 Makefile                    |  1 +
 builtin/submodule--helper.c |  4 +-
 json-writer.c               |  5 +--
 strbuf.c                    |  9 +++++
 strbuf.h                    |  5 +++
 t/t0015-hash.sh             | 56 --------------------------
 t/unit-tests/t-hash.c       | 79 +++++++++++++++++++++++++++++++++++++
 7 files changed, 96 insertions(+), 63 deletions(-)
 delete mode 100755 t/t0015-hash.sh
 create mode 100644 t/unit-tests/t-hash.c

Range-diff against v2:
1:  b8c4ab31c6 ! 1:  cd831fabf5 strbuf: introduce strbuf_addstrings() to repeatedly add a string
    @@
      ## Metadata ##
    -Author: Achu Luma <ach.lumap@gmail.com>
    +Author: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
     
      ## Commit message ##
         strbuf: introduce strbuf_addstrings() to repeatedly add a string
    @@ Commit message
         strbuf_addstrings() would be similar for strings as strbuf_addch() and
         strbuf_addchars() for characters.
     
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
    +    Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
    +    Co-authored-by: Achu Luma <ach.lumap@gmail.com>
         Signed-off-by: Achu Luma <ach.lumap@gmail.com>
    +    Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
     
      ## builtin/submodule--helper.c ##
     @@ builtin/submodule--helper.c: static void module_list_active(struct module_list *list)
    @@ strbuf.c: void strbuf_add(struct strbuf *sb, const void *data, size_t len)
      
     +void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
     +{
    -+       size_t len = strlen(s);
    -+       if (unsigned_mult_overflows(len, n))
    -+               die("you want to use way too much memory");
    -+       strbuf_grow(sb, len * n);
    -+       for (size_t i = 0; i < n; i++)
    -+               strbuf_add(sb, s, len);
    ++	size_t len = strlen(s);
    ++
    ++	strbuf_grow(sb, st_mult(len, n));
    ++	for (size_t i = 0; i < n; i++)
    ++		strbuf_add(sb, s, len);
     +}
     +
      void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
2:  20a7ff8e24 < -:  ---------- Port helper/test-sha256.c and helper/test-sha1.c to unit-tests/t-hash.c
-:  ---------- > 2:  2c4471ce37 t/: port helper/test-sha1.c to unit-tests/t-hash.c
-:  ---------- > 3:  68dcf388d8 t/: port helper/test-sha256.c to unit-tests/t-hash.c
-- 
2.45.1

[PATCH v3 1/3] strbuf: introduce strbuf_addstrings() to repeatedly add a string

[Ghanshyam Thakkar]

In a following commit we are going to port code from
"t/helper/test-sha256.c", t/helper/test-hash.c and "t/t0015-hash.sh" to
a new "t/unit-tests/t-hash.c" file using the recently added unit test
framework.

To port code like: perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;"
we are going to need a new strbuf_addstrings() function that repeatedly
adds the same string a number of times to a buffer.

Such a strbuf_addstrings() function would already be useful in
"json-writer.c" and "builtin/submodule-helper.c" as both of these files
already have code that repeatedly adds the same string. So let's
introduce such a strbuf_addstrings() function in "strbuf.{c,h}" and use
it in both "json-writer.c" and "builtin/submodule-helper.c".

We use the "strbuf_addstrings" name as this way strbuf_addstr() and
strbuf_addstrings() would be similar for strings as strbuf_addch() and
strbuf_addchars() for characters.

Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Co-authored-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 builtin/submodule--helper.c | 4 +---
 json-writer.c               | 5 +----
 strbuf.c                    | 9 +++++++++
 strbuf.h                    | 5 +++++
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e604cb5ddb..5620a5bf67 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -256,11 +256,9 @@ static void module_list_active(struct module_list *list)
 
 static char *get_up_path(const char *path)
 {
-	int i;
 	struct strbuf sb = STRBUF_INIT;
 
-	for (i = count_slashes(path); i; i--)
-		strbuf_addstr(&sb, "../");
+	strbuf_addstrings(&sb, "../", count_slashes(path));
 
 	/*
 	 * Check if 'path' ends with slash or not
diff --git a/json-writer.c b/json-writer.c
index 005c820aa4..25b9201f9c 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -46,10 +46,7 @@ static void append_quoted_string(struct strbuf *out, const char *in)
 
 static void indent_pretty(struct json_writer *jw)
 {
-	int k;
-
-	for (k = 0; k < jw->open_stack.len; k++)
-		strbuf_addstr(&jw->json, "  ");
+	strbuf_addstrings(&jw->json, "  ", jw->open_stack.len);
 }
 
 /*
diff --git a/strbuf.c b/strbuf.c
index 0d929e4e19..e3ca9b1ee9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -313,6 +313,15 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n)
+{
+	size_t len = strlen(s);
+
+	strbuf_grow(sb, st_mult(len, n));
+	for (size_t i = 0; i < n; i++)
+		strbuf_add(sb, s, len);
+}
+
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
 {
 	strbuf_grow(sb, sb2->len);
diff --git a/strbuf.h b/strbuf.h
index 97fa4a3d01..003f880ff7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -310,6 +310,11 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
 	strbuf_add(sb, s, strlen(s));
 }
 
+/**
+ * Add a NUL-terminated string the specified number of times to the buffer.
+ */
+void strbuf_addstrings(struct strbuf *sb, const char *s, size_t n);
+
 /**
  * Copy the contents of another buffer at the end of the current one.
  */
-- 
2.45.1

[PATCH v3 2/3] t/: port helper/test-sha1.c to unit-tests/t-hash.c

[Ghanshyam Thakkar]

t/helper/test-sha1 and t/t0015-hash.sh test the hash implementation of
SHA-1 in Git with basic SHA-1 hash values. Migrate them to the new unit
testing framework for better debugging and runtime performance.

The sha1 subcommand from test-tool is still not removed because it is
relied upon by t0013-sha1dc (which requires 'test-tool sha1' dying
when it is used on a file created to contain the known sha1 attack)
and pack_trailer():lib-pack.sh.

Helped-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Co-authored-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 Makefile              |  1 +
 t/t0015-hash.sh       | 22 ------------------
 t/unit-tests/t-hash.c | 54 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 22 deletions(-)
 create mode 100644 t/unit-tests/t-hash.c

diff --git a/Makefile b/Makefile
index 8f4432ae57..2b19fdf6ae 100644
--- a/Makefile
+++ b/Makefile
@@ -1335,6 +1335,7 @@ THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
 UNIT_TEST_PROGRAMS += t-ctype
+UNIT_TEST_PROGRAMS += t-hash
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-strbuf
diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
index 0a087a1983..2264e702d5 100755
--- a/t/t0015-hash.sh
+++ b/t/t0015-hash.sh
@@ -5,28 +5,6 @@ test_description='test basic hash implementation'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'test basic SHA-1 hash values' '
-	test-tool sha1 </dev/null >actual &&
-	grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
-	printf "a" | test-tool sha1 >actual &&
-	grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
-	printf "abc" | test-tool sha1 >actual &&
-	grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
-	printf "message digest" | test-tool sha1 >actual &&
-	grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
-	grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha1 >actual &&
-	grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
-	printf "blob 0\0" | test-tool sha1 >actual &&
-	grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
-	printf "blob 3\0abc" | test-tool sha1 >actual &&
-	grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
-	printf "tree 0\0" | test-tool sha1 >actual &&
-	grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
-'
-
 test_expect_success 'test basic SHA-256 hash values' '
 	test-tool sha256 </dev/null >actual &&
 	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
new file mode 100644
index 0000000000..89dfea9cc1
--- /dev/null
+++ b/t/unit-tests/t-hash.c
@@ -0,0 +1,54 @@
+#include "test-lib.h"
+#include "hash-ll.h"
+#include "hex.h"
+#include "strbuf.h"
+
+static void check_hash_data(const void *data, size_t data_length,
+			    const char *expected, int algo)
+{
+	git_hash_ctx ctx;
+	unsigned char hash[GIT_MAX_HEXSZ];
+	const struct git_hash_algo *algop = &hash_algos[algo];
+
+	if (!check(!!data)) {
+		test_msg("Error: No data provided when expecting: %s", expected);
+		return;
+	}
+
+	algop->init_fn(&ctx);
+	algop->update_fn(&ctx, data, data_length);
+	algop->final_fn(hash, &ctx);
+
+	check_str(hash_to_hex_algop(hash, algop), expected);
+}
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
+#define TEST_SHA1_STR(data, expected) \
+	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
+	     "SHA1 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA1_LITERAL(literal, expected) \
+	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
+	     "SHA1 (%s) works", #literal)
+
+int cmd_main(int argc, const char **argv)
+{
+	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+
+	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+
+	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
+	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
+	TEST_SHA1_STR("abc", "a9993e364706816aba3e25717850c26c9cd0d89d");
+	TEST_SHA1_STR("message digest", "c12252ceda8be8994d5fa0290a47231c1d16aae3");
+	TEST_SHA1_STR("abcdefghijklmnopqrstuvwxyz", "32d10c7b8cf96570ca04ce37f2a19d84240d3a89");
+	TEST_SHA1_STR(aaaaaaaaaa_100000.buf, "34aa973cd4c4daa4f61eeb2bdbad27316534016f");
+	TEST_SHA1_LITERAL("blob 0\0", "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
+	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
+	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
+
+	strbuf_release(&aaaaaaaaaa_100000);
+
+	return test_done();
+}
-- 
2.45.1

[PATCH v3 3/3] t/: port helper/test-sha256.c to unit-tests/t-hash.c

[Ghanshyam Thakkar]

t/helper/test-sha256 and t/t0015-hash test the hash implementation of
SHA-256 in Git with basic SHA-256 hash values. Port them to the new
unit testing framework for better debugging, simplicity and faster
runtime. The necessary building blocks are already implemented in
t-hash in the previous commit which ported test-sha1.

The 'sha256' subcommand of test-tool is still not removed, because it
is used by pack_trailer() in lib-pack.sh, which is used in many tests
of the t53** series.

Helped-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Co-authored-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t0015-hash.sh       | 34 ----------------------------------
 t/unit-tests/t-hash.c | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 34 deletions(-)
 delete mode 100755 t/t0015-hash.sh

diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 2264e702d5..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,34 +0,0 @@
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-256 hash values' '
-	test-tool sha256 </dev/null >actual &&
-	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
-	printf "a" | test-tool sha256 >actual &&
-	grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
-	printf "abc" | test-tool sha256 >actual &&
-	grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
-	printf "message digest" | test-tool sha256 >actual &&
-	grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
-	grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
-	# Try to exercise the chunking code by turning autoflush on.
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
-	perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
-	printf "blob 0\0" | test-tool sha256 >actual &&
-	grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
-	printf "blob 3\0abc" | test-tool sha256 >actual &&
-	grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
-	printf "tree 0\0" | test-tool sha256 >actual &&
-	grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
index 89dfea9cc1..0f86cd3730 100644
--- a/t/unit-tests/t-hash.c
+++ b/t/unit-tests/t-hash.c
@@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
 	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
 	     "SHA1 (%s) works", #literal)
 
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
+#define TEST_SHA256_STR(data, expected) \
+	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+	     "SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+	     "SHA256 (%s) works", #literal)
+
 int cmd_main(int argc, const char **argv)
 {
 	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+	struct strbuf alphabet_100000 = STRBUF_INIT;
 
 	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
 
 	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
 	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
@@ -48,7 +61,19 @@ int cmd_main(int argc, const char **argv)
 	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
 	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
 
+	TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+	TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+	TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+	TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+	TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+	TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+	TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+	TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+	TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+	TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
 	strbuf_release(&aaaaaaaaaa_100000);
+	strbuf_release(&alphabet_100000);
 
 	return test_done();
 }
-- 
2.45.1

Re: [PATCH v3 2/3] t/: port helper/test-sha1.c to unit-tests/t-hash.c

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

On Fri, May 24, 2024 at 05:29:44AM +0530, Ghanshyam Thakkar wrote:
> t/helper/test-sha1 and t/t0015-hash.sh test the hash implementation of
> SHA-1 in Git with basic SHA-1 hash values. Migrate them to the new unit
> testing framework for better debugging and runtime performance.
> 
> The sha1 subcommand from test-tool is still not removed because it is
> relied upon by t0013-sha1dc (which requires 'test-tool sha1' dying
> when it is used on a file created to contain the known sha1 attack)
> and pack_trailer():lib-pack.sh.

Can we refactor this test to stop doing that? E.g., would it work if we
used git-hash-object(1) to check that SHA1DC does its thing? Then we
could get rid of the helper altogether, as far as I understand.

> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> new file mode 100644
> index 0000000000..89dfea9cc1
> --- /dev/null
> +++ b/t/unit-tests/t-hash.c
> @@ -0,0 +1,54 @@
> +#include "test-lib.h"
> +#include "hash-ll.h"
> +#include "hex.h"
> +#include "strbuf.h"
> +
> +static void check_hash_data(const void *data, size_t data_length,
> +			    const char *expected, int algo)
> +{
> +	git_hash_ctx ctx;
> +	unsigned char hash[GIT_MAX_HEXSZ];
> +	const struct git_hash_algo *algop = &hash_algos[algo];
> +
> +	if (!check(!!data)) {

Is this double negation needed? Can't we just `if (!check(data))`?

> +		test_msg("Error: No data provided when expecting: %s", expected);

This error message is a bit atypical compared to the other callers of
this function. We could say something like "BUG: test has no data",
which would match something we have in "t/unit-tests/test-lib.c".

> +		return;
> +	}
> +
> +	algop->init_fn(&ctx);
> +	algop->update_fn(&ctx, data, data_length);
> +	algop->final_fn(hash, &ctx);
> +
> +	check_str(hash_to_hex_algop(hash, algop), expected);
> +}
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> +#define TEST_SHA1_STR(data, expected) \
> +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
> +	     "SHA1 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA1_LITERAL(literal, expected) \
> +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> +	     "SHA1 (%s) works", #literal)
> 

This macro also works for `TEST_SHA1_STR()`, right? Is there a
partiuclar reason why we don't unify them?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] t/: port helper/test-sha256.c to unit-tests/t-hash.c

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

On Fri, May 24, 2024 at 05:29:45AM +0530, Ghanshyam Thakkar wrote:
> t/helper/test-sha256 and t/t0015-hash test the hash implementation of
> SHA-256 in Git with basic SHA-256 hash values. Port them to the new
> unit testing framework for better debugging, simplicity and faster
> runtime. The necessary building blocks are already implemented in
> t-hash in the previous commit which ported test-sha1.
> 
> The 'sha256' subcommand of test-tool is still not removed, because it
> is used by pack_trailer() in lib-pack.sh, which is used in many tests
> of the t53** series.

Similar question here, are there replacements we can use for it? I also
couldn't see it being used in any test other than t0015 when searing for
"test-tool sha256". Maybe I'm looking for the wrong thing?

[snip]
> -test_done
> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> index 89dfea9cc1..0f86cd3730 100644
> --- a/t/unit-tests/t-hash.c
> +++ b/t/unit-tests/t-hash.c
> @@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
>  	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
>  	     "SHA1 (%s) works", #literal)
>  
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
> +#define TEST_SHA256_STR(data, expected) \
> +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA256_LITERAL(literal, expected) \
> +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #literal)
> +

Same question here regarding the macros and whether we can merge them.

Also, we do have the same test data for both hashes, and if we ever grow
another hash it's likely that we'll also want to check for the same
inputs there. Would it make sense to have a generic `TAST_HASHES()`
macro where you give the input and then both the expected SHA1 and
SHA256 to avoid some duplication?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/3] t/: port helper/test-sha1.c to unit-tests/t-hash.c

[Christian Couder]

On Fri, May 24, 2024 at 3:30 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, May 24, 2024 at 05:29:44AM +0530, Ghanshyam Thakkar wrote:
> > t/helper/test-sha1 and t/t0015-hash.sh test the hash implementation of
> > SHA-1 in Git with basic SHA-1 hash values. Migrate them to the new unit
> > testing framework for better debugging and runtime performance.
> >
> > The sha1 subcommand from test-tool is still not removed because it is
> > relied upon by t0013-sha1dc (which requires 'test-tool sha1' dying
> > when it is used on a file created to contain the known sha1 attack)
> > and pack_trailer():lib-pack.sh.
>
> Can we refactor this test to stop doing that? E.g., would it work if we
> used git-hash-object(1) to check that SHA1DC does its thing? Then we
> could get rid of the helper altogether, as far as I understand.

It could perhaps work if we used git-hash-object(1) instead of
`test-tool sha1` in t0013-sha1dc to check that SHA1DC does its thing,
but we could do that in a separate patch or patch series.

> > diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> > new file mode 100644
> > index 0000000000..89dfea9cc1
> > --- /dev/null
> > +++ b/t/unit-tests/t-hash.c
> > @@ -0,0 +1,54 @@
> > +#include "test-lib.h"
> > +#include "hash-ll.h"
> > +#include "hex.h"
> > +#include "strbuf.h"
> > +
> > +static void check_hash_data(const void *data, size_t data_length,
> > +                         const char *expected, int algo)
> > +{
> > +     git_hash_ctx ctx;
> > +     unsigned char hash[GIT_MAX_HEXSZ];
> > +     const struct git_hash_algo *algop = &hash_algos[algo];
> > +
> > +     if (!check(!!data)) {
>
> Is this double negation needed? Can't we just `if (!check(data))`?

As far as I remember it is needed as check() is expecting an 'int'
while 'data' is a 'void *'.

> > +             test_msg("Error: No data provided when expecting: %s", expected);
>
> This error message is a bit atypical compared to the other callers of
> this function. We could say something like "BUG: test has no data",
> which would match something we have in "t/unit-tests/test-lib.c".

Actually I think something like "BUG: Null data pointer provided"
would be even better.

> > +             return;
> > +     }
> > +
> > +     algop->init_fn(&ctx);
> > +     algop->update_fn(&ctx, data, data_length);
> > +     algop->final_fn(hash, &ctx);
> > +
> > +     check_str(hash_to_hex_algop(hash, algop), expected);
> > +}
> > +
> > +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> > +#define TEST_SHA1_STR(data, expected) \
> > +     TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA1), \
> > +          "SHA1 (%s) works", #data)
> > +
> > +/* Only works with a literal string, useful when it contains a NUL character. */
> > +#define TEST_SHA1_LITERAL(literal, expected) \
> > +     TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> > +          "SHA1 (%s) works", #literal)
> >
>
> This macro also works for `TEST_SHA1_STR()`, right?

No, it uses 'sizeof(literal)' which works only for string literals.

> Is there a
> partiuclar reason why we don't unify them?

The comments above them try to explain that the first one doesn't work
when the data contains a NUL char as it uses strlen() while the second
one works only for string literals including those which contain NUL
characters.

Thanks for your review.

Re: [PATCH v3 2/3] t/: port helper/test-sha1.c to unit-tests/t-hash.c

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> Can we refactor this test to stop doing that? E.g., would it work if we
>> used git-hash-object(1) to check that SHA1DC does its thing? Then we
>> could get rid of the helper altogether, as far as I understand.
>
> It could perhaps work if we used git-hash-object(1) instead of
> `test-tool sha1` in t0013-sha1dc to check that SHA1DC does its thing,
> but we could do that in a separate patch or patch series.

Yeah, I think such a plan to make preliminary refactoring as a
separate series, and then have another series to get rid of
"test-tool sha1" (and "test-tool sha256" as well?) on top of it
would work well.

>> > +     if (!check(!!data)) {
>>
>> Is this double negation needed? Can't we just `if (!check(data))`?
>
> As far as I remember it is needed as check() is expecting an 'int'
> while 'data' is a 'void *'.

It might be easier to read by being more explicit, "data != NULL",
if that is the case?  check() is like assert(), i.e., "we expect
data is not NULL", and if (!check("expected condition")) { guards an
error handling block for the case in which the expectation is not
met, right?

Re: [PATCH v3 3/3] t/: port helper/test-sha256.c to unit-tests/t-hash.c

[Ghanshyam Thakkar]

On Fri, 24 May 2024, Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, May 24, 2024 at 05:29:45AM +0530, Ghanshyam Thakkar wrote:
> > t/helper/test-sha256 and t/t0015-hash test the hash implementation of
> > SHA-256 in Git with basic SHA-256 hash values. Port them to the new
> > unit testing framework for better debugging, simplicity and faster
> > runtime. The necessary building blocks are already implemented in
> > t-hash in the previous commit which ported test-sha1.
> > 
> > The 'sha256' subcommand of test-tool is still not removed, because it
> > is used by pack_trailer() in lib-pack.sh, which is used in many tests
> > of the t53** series.
> 
> Similar question here, are there replacements we can use for it? I also
> couldn't see it being used in any test other than t0015 when searing for
> "test-tool sha256". Maybe I'm looking for the wrong thing?

It is used indirectly and not explicitly like 'test-tool sha256'. e.g.
in t/lib-pack.sh when GIT_TEST_DEFUALT_HASH=sha256, ...

  # Compute and append pack trailer to "$1"
  pack_trailer () {
	  test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
	  cat trailer.tmp >>"$1" &&
	  rm -f trailer.tmp
  }

...it will use 'test-tool sha256' on the first line of pack_trailer().
And the pack_trailer() is used in t7450, t5308, t5309, t5321.

And I will consult with Christian and Kaartic on the replacements but as
Christian and Junio said, doing it in another series would be a good
idea.

> [snip]
> > -test_done
> > diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> > index 89dfea9cc1..0f86cd3730 100644
> > --- a/t/unit-tests/t-hash.c
> > +++ b/t/unit-tests/t-hash.c
> > @@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
> >  	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> >  	     "SHA1 (%s) works", #literal)
> >  
> > +
> > +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
> > +#define TEST_SHA256_STR(data, expected) \
> > +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> > +	     "SHA256 (%s) works", #data)
> > +
> > +/* Only works with a literal string, useful when it contains a NUL character. */
> > +#define TEST_SHA256_LITERAL(literal, expected) \
> > +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> > +	     "SHA256 (%s) works", #literal)
> > +
> 
> Same question here regarding the macros and whether we can merge them.
> 
> Also, we do have the same test data for both hashes, and if we ever grow
> another hash it's likely that we'll also want to check for the same
> inputs there. Would it make sense to have a generic `TAST_HASHES()`
> macro where you give the input and then both the expected SHA1 and
> SHA256 to avoid some duplication?

Yeah, that can be done as the inputs are same for both hashes minus one
extra for sha256 (though I think it can be easily obtained). It looks
like a good idea to me for v4.

Thank you for the review!