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

[Antonio Ospite <ao2@ao2.it>]

On Mon, 14 May 2018 18:33:44 -0700
Stefan Beller <sbeller@google.com> wrote:

> On Mon, May 14, 2018 at 3:58 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>
> > ---
> >  builtin/submodule--helper.c | 39 +++++++++++++++++++++++++++++++++++++
> >  t/t7411-submodule-config.sh | 26 +++++++++++++++++++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 9e8f2acd5..b32110e3b 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -1825,6 +1825,44 @@ static int is_active(int argc, const char **argv, const char *prefix)
> >         return !is_submodule_active(the_repository, argv[1]);
> >  }
> >
> > +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> > +{
> > +       char *key = cb_data;
> > +
> > +       if (!strcmp(key, key_))
> > +               printf("%s\n", value_);
> > +
> > +       return 0;
> > +}
> > +
> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +       int ret;
> > +
> > +       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) {
> > +               char *key;
> > +
> > +               ret = git_config_parse_key(argv[1], &key, NULL);
> > +               if (ret < 0)
> > +                       return CONFIG_INVALID_KEY;
> > +
> > +               config_from_gitmodules(config_print_callback, the_repository, key);
> > +
> > +               free(key);
> > +               return 0;
> > +       }
> > +
> > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > +       if (argc == 3)
> > +               return config_gitmodules_set(argv[1], argv[2]);
> 
> Ah, here we definitely want to set it in the .gitmodules file?
> (Or does that change later in this series?)
> 
> > +
> > +       return 0;
> > +}
> > +
> >  #define SUPPORT_SUPER_PREFIX (1<<0)
> >
> >  struct cmd_struct {
> > @@ -1850,6 +1888,7 @@ static struct cmd_struct commands[] = {
> >         {"push-check", push_check, 0},
> >         {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> >         {"is-active", is_active, 0},
> > +       {"config", module_config, 0},
> >  };
> >
> >  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index a648de6a9..dfe019f05 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -139,4 +139,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >         )
> >  '
> >
> > +test_expect_success 'reading submodules config with "submodule--helper config"' '
> > +       (cd super &&
> 
> I think the project prefers a style
> of the cd at the same level of the echo and the following commands.
>

There is mixed style about that, so for new tests in existing files I'd
stick to the predominant style in the file.

For new test files I'll use the recommended style of cd on the same
level of the following commands.

> However we might not need the (cd super && ...) via
> 
>   echo "../submodule"  >expected
>   git -C super ubmodule--helper config submodule.submodule.url >../actual
>   test_cmp expected actual
> 
> Our friends developing git on Windows will thank us for saving
> to spawn a shell as spawning processes is expensive on Windows. :)
> Also we have fewer lines of code.
>

I'll see how that looks, thanks for the suggestion.

Again, I'd use a subshell if that's what the other tests in a file do,
and use "git -C" in new files.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?