On Fri, Feb 23, 2024 at 8:33 PM Ghanshyam Thakkar
<shyamthakkar001@xxxxxxxxx> wrote:
>
> Migrate t/helper/test-oid-array.c and t/t0064-oid-array.sh to the
> recently added unit testing framework. This would improve runtime
> performance and provide better debugging via displaying array content,
> index at which the test failed etc. directly to stdout.
It might not be a good idea to start working on a GSoC project we
propose (Move existing tests to a unit testing framework) right now.
You can work on it as part of your GSoC application, to show an
example of how you would do it, and we might review that as part of
reviewing your application. But for such a project if many candidates
started working on it and sent patches to the mailing list before they
get selected, then the project might be nearly finished before the
GSoC even starts.
So I think it would be better to work on other things instead, like
perhaps reviewing other people's work or working on other bug fixes or
features. Anyway now that this is on the mailing list, I might as well
review it as it could help with your application. But please consider
working on other things.
> There is only one change in the new testing approach. In the previous
> testing method, a new repo gets initialized for the test according to
> GIT_TEST_DEFAULT_HASH algorithm.
It looks like this happens in "t/test-lib-functions.sh", right?
Telling a bit more about how and where that happens might help
reviewers who would like to take a look.
> In unit testing however, we do not
> need to initialize the repo. We can set the length of the hexadecimal
> strbuf according to the algorithm used directly.
So is your patch doing that or not? It might be better to be explicit.
Also if 'strbuf's are used, then is it really worth it to set their
length in advance, instead of just letting them grow to the right
length as we add hex to them?
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx>
> ---
> [RFC]: I recently saw a series by Eric W. Biederman [1] which enables
> the use of oid's with different hash algorithms into the same
> oid_array safely. However, there were no tests added for this. So, I
> am wondering if we should have a input format which allows us to
> specify hash algo for each oid with its hex value. i.e. "sha1:55" or
> "sha256:55", instead of just "55" and relying on GIT_TEST_DEFAULT_HASH
> for algo. So far, I tried to imitate the existing tests but I suppose
> this may be useful in the future if that series gets merged.
The fact that there is a series touching the same area might also hint
that it might not be the right time to work on this.
> diff --git a/t/unit-tests/t-oid-array.c b/t/unit-tests/t-oid-array.c
> new file mode 100644
> index 0000000000..b4f43c025d
> --- /dev/null
> +++ b/t/unit-tests/t-oid-array.c
> @@ -0,0 +1,222 @@
> +#include "test-lib.h"
> +#include "hex.h"
> +#include "oid-array.h"
> +#include "strbuf.h"
> +
> +#define INPUT "88", "44", "aa", "55"
> +#define INPUT_DUP \
> + "88", "44", "aa", "55", "88", "44", "aa", "55", "88", "44", "aa", "55"
Can you reuse INPUT in INPUT_DUP?
> +#define INPUT_ONLY_DUP "55", "55"
> +#define ENUMERATION_RESULT_SORTED "44", "55", "88", "aa"
> +
> +/*
> + * allocates the memory based on the hash algorithm used and sets the length to
> + * it.
> + */
> +static void hex_strbuf_init(struct strbuf *hex)
> +{
> + static int sz = -1;
> +
> + if (sz == -1) {
> + char *algo_env = getenv("GIT_TEST_DEFAULT_HASH");
> + if (algo_env && !strcmp(algo_env, "sha256"))
> + sz = GIT_SHA256_HEXSZ;
> + else
> + sz = GIT_SHA1_HEXSZ;
> + }
> +
> + strbuf_init(hex, sz);
> + strbuf_setlen(hex, sz);
> +}
A strbuf can grow when we add stuff to it. We don't need to know its
size in advance. So I am not sure this function is actually useful.
> +/* fills the hex strbuf with alternating characters from 'c' */
> +static void fill_hex_strbuf(struct strbuf *hex, char *c)
> +{
> + size_t i;
> + for (i = 0; i < hex->len; i++)
> + hex->buf[i] = (i & 1) ? c[1] : c[0];
There is strbuf_addch() to add a single char to a strbuf, or
strbuf_add() and strbuf_addstr() to add many chars at once.
> +}
