Re: [PATCH v2 4/4] config: add '--show-scope' to print the scope of a config value
- From
Matt Rogers <mattr94@gmail.com>
- Date
- Jan 9, 2020, 23:47 UTC
- Message-ID
- <CAOjrSZviSf+b6-MMavyAntcp0MfK91N-cZEyJYiwJ8OqEbgXYQ@mail.gmail.com>
- In-Reply-To
- <xmqqd0bsz9xm.fsf@gitster-ct.c.googlers.com>
Show 20 quoted lines
> > + 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.
Show 37 quoted lines
>
> > + }
> > +}
> > +
> > +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)
Show 16 quoted lines
> > 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