Re: [GSoC][PATCH] t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 7, 2024, 08:31 UTC
Message-ID: <CAP8UFD0r+YYxAvN2Ej1mGa2Kt5M2dQgQEGLraB3iQ30cPWuA6Q@mail.gmail.com>
In-Reply-To: <dohbd64jxuahelut63esztozdozqrhx5rgv5m4t3wt5gz6v6kv@6q2aivlcvxcq>

On Fri, Jun 7, 2024 at 1:35 AM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:

Show 16 quoted lines

> > Come to think of it, how is your check_each_cb() ensuring that it is
> > only called once with "123" when queried with "12300"?  If the
> > callback is made with "123" 100 times with the single query with
> > "12300", would it even notice?  I would imagine that the original
> > would (simply because it dumps each and every callback to a file to
> > be compared with the golden copy).
>
> That's true! I did not think of that. What do you think about something
> like this then? I will clean it up to send in v2.
>
> ---
>
> struct cb_data {
>         int *i;
>         struct strvec *expected_hexes;
> };

It might be better to use a more meaningful name for the struct, like perhaps 'expected_hex_iter'. Also I think 'i' could be just 'size_t i' instead of 'int *i', and 'expected_hexes' could be just 'hexes'.

> static enum cb_next check_each_cb(const struct object_id *oid, void *data)
> {
>         struct cb_data *cb_data = data;

Maybe: `struct expected_hex_iter *hex_iter = data;`

>         struct object_id expected;
>
>         if(!check_int(*cb_data->i, <, cb_data->hexes->nr)) {

A space character is missing between 'if' and '('. And by the way you use 'hexes' instead of 'expected_hexes' here.

Show 30 quoted lines

>                 test_msg("error: extraneous callback. found oid: %s", oid_to_hex(oid));
>                 return CB_BREAK;
>         }
>
>         if (!check_int(get_oid_arbitrary_hex(cb_data->expected_hexes->v[*cb_data->i], &expected), ==, 0))
>                 return CB_BREAK;
>         if (!check(oideq(oid, &expected)))
>                 test_msg("expected: %s\n       got: %s",
>                          hash_to_hex(expected.hash), hash_to_hex(oid->hash));
>
>         *cb_data->i += 1;
>         return CB_CONTINUE;
> }
>
> static void check_each(struct oidtree *ot, char *query, ...)
> {
>         struct object_id oid;
>         struct strvec hexes = STRVEC_INIT;
>         struct cb_data cb_data;
>         const char *arg;
>         int i = 0;
>
>         va_list expected;
>         va_start(expected, query);
>
>         while ((arg = va_arg(expected, const char *)))
>                 strvec_push(&hexes, arg);
>
>         cb_data.i = &i;
>         cb_data.expected_hexes = &hexes;

Can't we just have something like:

        struct expected_hex_iter hex_iter = { .i = 0, .hexes = &hexes };

above when 'hex_iter' is declared?

Show 7 quoted lines

>         if (!check_int(get_oid_arbitrary_hex(query, &oid), ==, 0))
>                 return;
>         oidtree_each(ot, &oid, strlen(query), check_each_cb, &cb_data);
>
>         if (!check_int(*cb_data.i, ==, cb_data.expected_hexes->nr))
>                 test_msg("error: could not find some oids");
> }

Thanks.