On Wed, Apr 24, 2024 at 01:58:56PM +1000, James Liu wrote:
> advise_if_enabled() has a special branch to handle
> backwards-compatibility with the `pushUpdateRejected` and
> `pushNonFastForward` advice types, which went untested.
>
> Modify the `test-tool advise` command so the advice type can be changed
> between nestedTag (the previous behaviour) and pushUpdateRejected.
>
> Signed-off-by: James Liu <james@xxxxxxxxxxx>
> ---
> t/helper/test-advise.c | 20 ++++++++++++++------
> t/t0018-advice.sh | 30 +++++++++++++++++++++++++++---
> 2 files changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
> index 8a3fd0009a..c18b18e059 100644
> --- a/t/helper/test-advise.c
> +++ b/t/helper/test-advise.c
> @@ -5,18 +5,26 @@
>
> int cmd__advise_if_enabled(int argc, const char **argv)
> {
> - if (argc != 2)
> - die("usage: %s <advice>", argv[0]);
> + if (argc != 3)
> + die("usage: %s nestedTag|pushUpdateRejected <advice>", argv[0]);
We could retain the old behaviour here so that we don't have to update
all tests. So in case `argc == 2` we implicitly use `ADVICE_NESTED_TAG`,
if `argc == 3` we look up the advice passed by the caller. The usage
would thus essentially become something like this:
usage: test-advise <msg> [<key>]
> - if (argc != 2)
> - die("usage: %s <advice>", argv[0]);
> + if (argc != 3)
> + die("usage: %s nestedTag|pushUpdateRejected <advice>", argv[0]);
> setup_git_directory();
> git_config(git_default_config, NULL);
>
> /*
> - * Any advice type can be used for testing, but NESTED_TAG was
> - * selected here and in t0018 where this command is being
> - * executed.
> + * Any advice type can be used for testing, but ADVICE_NESTED_TAG and
> + * ADVICE_PUSH_UPDATE_REJECTED were selected here and used in t0018
> + * where this command is being executed.
> + *
> + * This allows test cases to exercise the normal and special branch
> + * within advice_enabled().
> */
> - advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
> + if (!strcmp(argv[1], "nestedTag"))
> + advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[2]);
> + else if (!strcmp(argv[1], "pushUpdateRejected"))
> + advise_if_enabled(ADVICE_PUSH_UPDATE_REJECTED, "%s", argv[2]);
> + else
> + die("advice type should be nestedTag|pushUpdateRejected");
Instead of singling out these specific advices, can we maybe expose the
`advice_setting[]` array and make this generic? We could for example add
a new `lookup_advice_by_name()` function that you pass the advice key,
which then walks through the array to look up the `enum advice_type`.
Patrick
Attachment:
signature.asc
Description: PGP signature
