Re: [PATCH v2 4/4] config: add '--show-scope' to print the scope of a config value

From: Matt Rogers <mattr94@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Matthew Rogers via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 4/4] config: add '--show-scope' to print the scope of a config value
Date: Thu, 9 Jan 2020 18:47:36 -0500	[thread overview]
Message-ID: <CAOjrSZviSf+b6-MMavyAntcp0MfK91N-cZEyJYiwJ8OqEbgXYQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqd0bsz9xm.fsf@gitster-ct.c.googlers.com>

> > +     switch (scope) {
> > +     case CONFIG_SCOPE_LOCAL:
> > +             return "local";
> > +     case CONFIG_SCOPE_GLOBAL:
> > +             return "global";
> > +     case CONFIG_SCOPE_SYSTEM:
> > +             return "system";
> > +     case CONFIG_SCOPE_WORKTREE:
> > +             return "worktree";
> > +     case CONFIG_SCOPE_COMMAND:
> > +             return "command";
> > +     case CONFIG_SCOPE_SUBMODULE:
> > +             return "submodule";
> > +     default:
> > +             return "unknown";
>
> Makes me wonder if this wants to be done via a table, which would
> later allow a parser to map in the other direction, taking a string
> and returning the corresponding enum, more easily (imagine a yet to
> be invented feature to "list config settings for only this scope").

I was thinking the same but then I realized such an option already exists,
--local and company.

>
> > +     }
> > +}
> > +
> > +static void show_config_scope(struct strbuf *buf) {
> > +     const char term = end_nul ? '\0' : '\t';
> > +     const char *scope = scope_to_string(current_config_scope());
> > +
> > +     strbuf_addstr(buf, N_(scope));
> > +     strbuf_addch(buf, term);
> > +}
> > +
> >  static int show_all_config(const char *key_, const char *value_, void *cb)
> >  {
> > -     if (show_origin) {
> > +     if (show_origin || show_scope) {
> >               struct strbuf buf = STRBUF_INIT;
> > -             show_config_origin(&buf);
> > +             if (show_scope)
> > +                     show_config_scope(&buf);
> > +             if (show_origin)
> > +                     show_config_origin(&buf);
> >               /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> >               fwrite(buf.buf, 1, buf.len, stdout);
> >               strbuf_release(&buf);
> > @@ -213,6 +245,8 @@ struct strbuf_list {
> >
> >  static int format_config(struct strbuf *buf, const char *key_, const char *value_)
> >  {
> > +     if (show_scope)
> > +             show_config_origin(buf);
> >       if (show_origin)
> >               show_config_origin(buf);
>
> Shouldn't one of these show scope instead of origin?  I wonder how
> the tests I see later in the patch could have passed with this.
>

Oof, that was a blunder... I think this passes the tests because none of them
actually hits `format_config()` only `show_all_config()` so I'll definitely add
a test case for this. (and fix it while I'm at it)

> > diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> > index 78bbb9eb98..aae2c6fc9e 100644
> > --- a/t/helper/test-config.c
> > +++ b/t/helper/test-config.c
> > @@ -48,8 +48,10 @@ static const char *scope_name(enum config_scope scope)
> >               return "repo";
> >       case CONFIG_SCOPE_WORKTREE:
> >               return "worktree";
> > +     case CONFIG_SCOPE_SUBMODULE:
> > +             return "submodule";
> >       case CONFIG_SCOPE_COMMAND:
> > -             return "command";
> > +             return "cmdline";
>
> Oh???
>

I definitely got a little too excited to get this patch in, so I
definitely apologize for that,
but thanks for the review.

-- 
Matthew Rogers