Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

[Stefan Beller <sbeller@google.com>]

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Note that the tests follow the predominant style in the file: subshell and cd
> right at the start of the sub-shell opening.
>
>  builtin/submodule--helper.c | 17 +++++++++++++++++
>  t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..14f0845d30 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
>         return 0;
>  }
>
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +       if (argc < 2 || argc > 3)
> +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +
> +       /* Equivalent to ACTION_GET in builtin/config.c */
> +       if (argc == 2)
> +               return print_config_from_gitmodules(argv[1]);
> +
> +       /* Equivalent to ACTION_SET in builtin/config.c */
> +       if (argc == 3)
> +               return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +
> +       return 0;

Technically we cannot reach this point here?
Maybe it would be more defensive to

    BUG("How did we get here?");

or at least return something !=0 ?

>
> +test_expect_success 'reading submodules config with "submodule--helper config"' '

I shortly wondered if it would make sense to put these tests at the
beginning of either
this or a new file, as the functionality is rather fundamental. (I
never thought about
it, but actually it is better to go from common basic to more exotic
tests as you scroll
down the file), but this place is ok, if you choose to do so.

Thanks,
Stefan