Re: [PATCH 05/20] userdiff tests: list builtin drivers via test-tool

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 15, 2021, 01:24 UTC
Message-ID: <CAPig+cQbPkfh7f2VpeCitRwRMothYqUvUqQk0HiZnUGUpvWTvw@mail.gmail.com>
In-Reply-To: <20210215005236.11313-6-avarab@gmail.com>

On Sun, Feb 14, 2021 at 7:56 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 17 quoted lines

> Change the userdiff test to list the builtin drivers via the
> test-tool, using the new for_each_userdiff_driver() API function.
> [...]
> I only need the "list-builtin-drivers "argument here, but let's add
> "list-custom-drivers" and "list-drivers" too, just because it's easy.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/helper/test-userdiff.c b/t/helper/test-userdiff.c
> @@ -0,0 +1,32 @@
> +static int userdiff_list_builtin_drivers_cb(struct userdiff_driver *driver,
> +                                           enum userdiff_driver_type type,
> +                                           void *priv)
> +{
> +       puts(driver->name);
> +       return 0;
> +}

Nit: The word "builtin" in the name of this callback function made me
search the patch for its cousin function which would be used for
custom drivers. It was only after reading on that I discovered that
this one function is used for both builtin and custom drivers. (Caught
me off guard, but not itself worth a re-roll.)

Show 6 quoted lines

> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> @@ -8,6 +8,10 @@ test_description='Test custom diff function name patterns'
>  test_expect_success 'setup' '
> +       test-tool userdiff list-builtin-drivers >builtin-drivers &&
> +       test_file_not_empty builtin-drivers &&
> +       builtin_drivers=$(cat builtin-drivers) &&

Nit: I get why you are using test_file_not_empty() for its diagnostic
value, but it does confuse the reader a bit to see the file
"builtin-drivers" created for no particularly good reason. This could
easily have been:

    builtin_drivers=$(test-tool userdiff list-builtin-drivers) &&
    test -n "$builtin_drivers" &&

Subjective and not worth a re-roll.